Skip to content
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

Fixes some pickling issues in the Cythonized Scheduler #4768

Merged
merged 5 commits into from
Apr 29, 2021

Conversation

jakirkham
Copy link
Member

A few of the test failures discovered in PR ( #4764 ) were related to pickling issues. One relating to pickling unbound methods, which Cython has a fix for in a pending release ( cython/cython#2972 ). Another related pickling to lambdas ( cython/cython#3499 ).

Here we workaround these issues by using other techniques. This fixes a couple of test failures on the Cython Scheduler. So are now re-enabled in this PR.

  • Closes #xxxx
  • Tests added / passed
  • Passes black distributed / flake8 distributed / isort distributed

Cython encounters a bug when pickling unbound class methods. This has
been fixed upstream, but not yet released. Include a link to the issue
for reference.
Cython encounters a bug when pickling unbound class methods. This has
been fixed upstream, but not yet released. To workaround this issue,
create a local wrapper function to pickle in its place that just calls
the method.
Cython seems to run into issues pickling the `lambda` in
`Scheduler.__init__`. Link to the relevant upstream issue for tracking.
Cython seems to run into issues pickling the `lambda` in
`Scheduler.__init__`. Instead of using a `lambda`, workaround this by
using `partial`. This pickles without problems bypassing the Cython
issue.
Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just had a questions to help me understand the problem. Changes look good

Comment on lines -50 to +54
"teardown": dumps_function(Scheduler.remove_plugin),
"teardown": dumps_function(remove_plugin),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe dumb question but Scheduler.remove_plugin isn't a local scope function. Why does this change anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a workaround for cython/cython#2972. An upstream fix in cython (cython/cython#2969) should be included in the cython=0.29.24 release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly

Pickling of the local remove_plugin function should only include the function and module to import. The code itself won't need to be pickled, which avoids needing to pickle Scheduler.remove_plugin

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these fixes @jakirkham! I pushed a tiny commit to add a comment about remove_plugin. Will merge after CI finishes

@jrbourbeau jrbourbeau merged commit 97c66db into dask:main Apr 29, 2021
@jakirkham jakirkham deleted the fix_some_cy_xfails branch April 29, 2021 16:12
@jakirkham
Copy link
Member Author

Thanks all! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants