-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add snapshot_callback support to pipeline.run (with agent support) #10312
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
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@mpangrazzi thanks for implementing this! If I recall a number of users also requested a way to turn off the file saving behavior and I think you mentioned you'd look at adding a something like a boolean toggle for this when working on this feature. Is that something you are still planning on doing? |
|
@sjrl yes definitely! But I would do that in a separate PR. So, with this one we introduce |
Sounds good! Especially the env var idea. |
sjrl
left a comment
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.
Looks good!
Makes me wonder if in the future if we should refactor Breakpoint to drop the snapshot_file_path field and always use the callback feature. Then we could create a default snapshot_callback that uses the current logic to save to disk.
davidsbatista
left a comment
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.
LGTM, just minor suggestions.
Co-authored-by: David S. Batista <[email protected]>
Co-authored-by: David S. Batista <[email protected]>
Co-authored-by: David S. Batista <[email protected]>
|
@sjrl @davidsbatista thanks! |
Related Issues
Proposed Changes:
NOTE: Reimplementation of #10250
Added a
snapshot_callbackparameter toPipeline.run()andAgent.run()that allows users to provide a custom handler for pipeline snapshots instead of the default file-saving behavior. Currently, when a breakpoint is triggered or an error occurs during pipeline execution, snapshots are always saved to JSON files on disk.This is limiting for users who want to e.g. save snapshots to a database or send snapshots to a remote service.
How did you test it?
unit tests, manual verification
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.