-
Notifications
You must be signed in to change notification settings - Fork 28
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
A0-4155: Unit saving pipeline #432
A0-4155: Unit saving pipeline #432
Conversation
Please make sure the following happened
|
5ae8704
to
0930a7d
Compare
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.
Including backup save in the pipeline is overall a good idea, although I don't support the idea of saving (unit, parents)
in the backup. If you want the DagUnit
to go through the backup, no problem with that, just when doing backup save, only save the inner unit, and pass the whole DagUnit
further in the pipeline.
If there's some hidden idea behind saving the whole DagUnit
, please explain.
In particular this means that this doesn't change the API, so only patch version bump needed.
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, nice change in the add_unit function but a little risky :P
What makes it a little risky? |
I think @kostekIV means that it will no longer remove the units from the cache immediately after they are being processed by the |
Make the unit saving into a pipeline, to simplify the semantics of units being officially added to the Dag.