adding hooks to expectations, with prep to make GE and hooks interface neatly
Details
Details
- Reviewers
- None
Unit +
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Branch
- gehook (branched from master)
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
this is my first try at adding hooks on expectation results, and is likely useful for GE tooling
Comment Actions
This looks nice and clean to me. Will defer to others who have more familiarity with this part of the system to make the final call.
Comment Actions
how do we handle multiple ExpectationResult? is this new hook gonna be on per solid or would it also support setting up on a pipeline?
would also like to hear thoughts from @schrockn and @alangenfeld
Comment Actions
per pipeline should be supported as well as per solid is, but tbh the use case for expectation results as I see it is way more likely to be limited to a few solids.
Right now it does get called for every expectation result, but it should be easy to provide native hook supports for 'only respond to ERs with a given label'
Comment Actions
I think it's a good idea to proceed with our own use cases ("limited to a few solids") first -- I'd be comfortable with testing this as an util internally first
Comment Actions
do hooks have a concept of should_execute, like schedules? seems like having an explicit filter would be helpful once we're talking about events that may be fired n times
Comment Actions
This is a case where I think our "list" approach fails us and we may want to consider a different type of hook and processes one event at a time. For hooks of this nature I'm concerned about long-running computations where getting the events only at the end will feel like a bug. We've spent a bunch of effort making the whole core streaming so want to keep that going.
I think we should have a generic event_hook with expectation_hook built on top of it. Further filtering can just be implement as custom event_hook