Page MenuHomeElementl

first run of hooks on expectations
Changes PlannedPublic

Authored by yuhan on Aug 13 2020, 4:29 PM.



adding hooks to expectations, with prep to make GE and hooks interface neatly

Test Plan

Unit +

Diff Detail

R1 dagster
gehook (branched from master)
Lint Passed
No Test Coverage

Event Timeline

this is my first try at adding hooks on expectation results, and is likely useful for GE tooling

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.

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

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'

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

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

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

This revision now requires changes to proceed.Aug 17 2020, 2:31 PM
yuhan edited reviewers, added: leoeer; removed: yuhan.
This revision now requires review to proceed.Feb 2 2021, 10:26 PM