-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
[REVIEW] ToPickle - Unpickle on the Scheduler #5728
Conversation
Unit Test Results 12 files ±0 12 suites ±0 5h 50m 36s ⏱️ - 1m 41s For more details on these failures, see this check. Results for commit 9734422. ± Comparison against base commit 7a69b5e. ♻️ This comment has been updated with latest results. |
This reverts commit 2906540.
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.
Thanks again for this @madsbk ! I only had minor suggestions
distributed/protocol/serialize.py
Outdated
"""Mark an object that should be pickled | ||
|
||
Both the scheduler and workers with automatically unpickle this | ||
object on arrival |
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.
Should we add a note about the "distributed.scheduler.pickle"
config here?
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.
Added:
"""Mark an object that should be pickled
Both the scheduler and workers with automatically unpickle this
object on arrival.
Notice, this requires that the scheduler is allowed to use pickle.
If the configuration option "distributed.scheduler.pickle" is set
to False, the scheduler will raise an exception instead.
"""
@rjzamora thanks, I have added your suggestions :) |
I think we are agreed on merging this PR in. I'll tentatively plan on merging Monday after the release unless there are further comments |
@quasiben @madsbk - Is this ready to go? I'm exceited to unblock dask/dask#8672 :) |
Yes :) |
Hi folks. Sorry that you didn't get any review on this. However, I'd also like to raise two concerns retroactively:
These are both things that I would expect anyone with commit access to this repository to be concerned about going forward. That critique out of the way I'm glad to see that this works. The usability of it seems good. I'm hopeful that y'all will have some time to both clean up the protocol a bit sometime in the future and also maybe help test things like this before merging future code. |
Thanks for following up on this @mrocklin - Totally my fault this was merged in such a rush. I agree with your comments. |
Yeah, I think that we should probably stop experimenting in main, at least
for parts of the code that affect the core. We should probably use
branches here on out i think.
…On Tue, Mar 29, 2022, 6:09 PM Richard (Rick) Zamora < ***@***.***> wrote:
Thanks for following up on this @mrocklin <https://github.com/mrocklin> -
Totally my fault this was merged in such a rush. I agree with your comments.
—
Reply to this email directly, view it on GitHub
<#5728 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACKZTHZ26R6GAYVO6LBSM3VCOESRANCNFSM5NBC6FSA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think Regarding your first concern, I agree :) |
I think that if we're adding funcitonality to the core then we should verify that functionality's correctness. You did this in the very specific case that you were curious about, high level graphs with strange serialization requirements. However, for anyone else trying to verify this functionality in a more common or general case that test is not that useful. Whenever we add something to the core (and I think that the protocol is very core) we should be testing that functionality with common and simple situations I think. We should also be thinking adversarially and finding cases that might not work well and verify that the behavior is sensible in those cases. |
I agree, but I am not surprised that we find bugs in new functionality. I think the highest priority when modifying the core is to make sure that existing code keeps working, which is extensively tested in this case.
I disagree, I like Furthermore, it was reassuring that dask/dask#8672 didn't find any bugs. Having said that, I didn't think of the case where |
Explore the usefulness of allowing unpickle on the scheduler.
Layers can now mark objects that should be unpickled on the scheduler (and worker) using
ToPickle
.Notice, the PR is not ready for code review.
cc. @rjzamora
pre-commit run --all-files