-
Notifications
You must be signed in to change notification settings - Fork 241
Feat: Add signal listener and methods in scheduler #4983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
205bcff
to
00d3802
Compare
sqlmesh/core/scheduler.py
Outdated
def on_signal_register( | ||
self, | ||
snapshot_id: SnapshotId, | ||
signals: t.Dict[str, t.Dict[str, str]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing serialized arguments means that we design this interface for one use case and one use case only. I think it'd be more extendable if we just pass a Snapshot
instance instead and let the implementation figure out what to do with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, shouldn't the registration be performed once for ALL snapshots with signals? And not the individual one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, shouldn't the registration be performed once for ALL snapshots with signals? And not the individual one?
yes will revise to do it all snapshots at once and since passing snapshot instead it'd be easier
sqlmesh/core/scheduler.py
Outdated
signal_name: str, | ||
signal_index: int, | ||
intervals: Intervals, | ||
signal_kwargs: t.Dict[str, str], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: I'd rather just pass a snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revised to pass snapshot here instead. I'm not sure if you meant to also not pass the signal related args?
00d3802
to
7ae9d39
Compare
Closing and deprioritising this for now to focus on other work |
This adds the provisionally called
SignalListener
class with methods which are called when signals are registered (on_signal_register
), when a signal starts (on_signal_start
) and when it finishes with success or failure (on_signal_end
).