[REVIEW] ToPickle - Unpickle on the Scheduler

[madsbk]

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

• Tests added / passed
• Passes pre-commit run --all-files

[github-actions]

Unit Test Results

       12 files  ±0         12 suites  ±0   5h 50m 36s ⏱️ - 1m 41s
  2 654 tests +1    2 572 ✔️ +2    80 💤 ±0  2 ❌  - 1 
13 025 runs  +4  12 384 ✔️ +4  638 💤 +1  3 ❌  - 1 

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.

[rjzamora]

@madsbk - dask#8672 is is pretty close to "review ready," but it depends on this getting in first. Let me know if you need any help getting this ready for review as well

[madsbk]

@madsbk - dask#8672 is is pretty close to "review ready," but it depends on this getting in first. Let me know if you need any help getting this ready for review as well

I think this is ready for reviews now

[rjzamora]

Thanks again for this @madsbk ! I only had minor suggestions

[rjzamora]

Should we add a note about the "distributed.scheduler.pickle" config here?

[madsbk]

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.
    """

[madsbk]

@rjzamora thanks, I have added your suggestions :)

[rjzamora]

Are the current ci failures related to this PR?

@fjetter @quasiben - Any thoughts/concerns here? My understanding is that this feature is quite safe on its own, and wont change behavior until dask/dask serialization code begins "opting in".

[quasiben]

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

[rjzamora]

@quasiben @madsbk - Is this ready to go? I'm exceited to unblock dask/dask#8672 :)

[madsbk]

@quasiben @madsbk - Is this ready to go? I'm exceited to unblock dask/dask#8672 :)

Yes :)

[quasiben]

Thanks @madsbk and @rjzamora -- merging in

[mrocklin]

Hi folks. Sorry that you didn't get any review on this. However, I'd also like to raise two concerns retroactively:

1. I'm getting pretty concerned about the complexity of the complexity of the protocol here. I would like to see us unwind some of that mess if we can (maybe something like Decouple serialization, frame splitting, and compression in protocol  #5150 ?)
2. I was sad to see that a PR that was merged without any real testing, especially something this close to the core. I wrote up Add tiny test for ToPickle #6021 just to convince myself that this worked.

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.

[rjzamora]

Thanks for following up on this @mrocklin - Totally my fault this was merged in such a rush. I agree with your comments.

[mrocklin]

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: ***@***.***>

[madsbk]

2. I was sad to see that a PR that was merged without any real testing, especially something this close to the core. I wrote up Add tiny test for ToPickle #6021 just to convince myself that this worked.

I think test_to_pickle.py is a real test. I agree, we could have added some more tests, like a test with a custom class and not just a list. But remember, basically all existing tests that use communication also tests this PR in the default case where ToPickle isn't used.

Regarding your first concern, I agree :)

[mrocklin]

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.

[madsbk]

I think that if we're adding funcitonality to the core then we should verify that functionality's correctness.

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.

You did this in the very specific case that you were curious about, high level graphs with strange serialization requirements.

I disagree, test_non_msgpack_serializable_layer tests all the new code paths through dumps() and loads().

I like test_ToPickle as an informative explicit test but the dumps() and loads() of an object within a dict is already tested by test_non_msgpack_serializable_layer implicitly (see highlevelgraph.py) -- it tests a lot implicitly by triggering a full HLG pack/unpack.

Furthermore, it was reassuring that dask/dask#8672 didn't find any bugs.

Having said that, I didn't think of the case where dumps() and loads() is never called, which happens when using the inproc protocol. In this case, we have to make nested_deserialize() remove the wrapping ToPickle class as you suggest: #6021 (comment)