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

Improve compatibility with "nogil" Python and 3.11 #470

Merged
merged 7 commits into from May 20, 2022

Conversation

colesbury
Copy link
Contributor

This makes a number of changes to improve compatibility with the
"nogil" Python fork as well as the Python 3.11+.

  • Fix _code_reduce for 3.11b0 and nogil Python

  • Use instr.argval in _walk_global_ops. This avoids adding a special
    case for 3.11+ (and is useful for nogil Python). In 3.11+, the argval
    for LOAD_GLOBAL would need to be divided by two to access the correct
    name. The 'argval' field already stores the correct name.

  • Set __builtins__ before constructing de-pickled functions. (Useful
    for nogil Python)

  • Fix test_recursion_during_pickling in Python 3.11+. Objects now have
    a default __getstate__ method so __getattr__ was never called,
    but __getattribute__ would still be called.

This makes a number of changes to improve compatibility with the
"nogil" Python fork as well as the upcoming 3.11 release.

 - Fix _code_reduce for 3.11b0 and nogil Python

 - Use instr.argval in _walk_global_ops. This avoids adding a special
   case for 3.11+ (and is useful for nogil Python). In 3.11+, the argval
   for LOAD_GLOBAL would need to be divided by two to access the correct
   name. The 'argval' field already stores the correct name.

 - Set '__builtins__' before constructing de-pickled functions. (Useful
   for nogil Python)

 - Fix test_recursion_during_pickling in Python 3.11+. Objects now have
   a default `__getstate__` method so `__getattr__` was never called,
   but `__getattribute__` would still be called.
@colesbury
Copy link
Contributor Author

There's still a bug in Python 3.11b0 that prevents the tests from passing:

python/cpython#92932

But once that is fixed all the tests should pass. (I've run the tests locally with a patched version of 3.11b0).

Also, I've included both changes for "nogil" and CPython 3.11 because some of the changes overlapped. Let me know if you'd prefer me to split up the PRs.

@pcmoritz
Copy link
Contributor

Thanks a lot for your work on nogil Python -- I'm a big fan of this work! Last time I tried it out, I was running into CloudPickle not working so it is really great to see you are fixing it!

@ogrisel ogrisel self-requested a review May 20, 2022 07:45
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #470 (bb3c31b) into master (2fc334d) will increase coverage by 8.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   84.47%   92.60%   +8.13%     
==========================================
  Files           4        4              
  Lines         715      717       +2     
  Branches      157      158       +1     
==========================================
+ Hits          604      664      +60     
+ Misses         87       32      -55     
+ Partials       24       21       -3     
Impacted Files Coverage Δ
cloudpickle/cloudpickle.py 88.59% <100.00%> (+0.59%) ⬆️
cloudpickle/cloudpickle_fast.py 96.93% <100.00%> (+16.25%) ⬆️
cloudpickle/compat.py 100.00% <0.00%> (+30.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fc334d...bb3c31b. Read the comment docs.

@ogrisel ogrisel added the ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects. label May 20, 2022
@ogrisel
Copy link
Contributor

ogrisel commented May 20, 2022

Since we now have a nogil-specific branch in the code, we might want to have a nogil CI in the build matrix. However this will require compiling nogil-CPython from source which adds significant CI config complexity...

I tested locally with my up-to-date nogil venv and all cloudpickle tests pass though.

@ogrisel
Copy link
Contributor

ogrisel commented May 20, 2022

The dask distributed failure is unrelated and will be fixed concurrently in #471.

@ogrisel
Copy link
Contributor

ogrisel commented May 20, 2022

@pierreglaser any opinion on the changes in this PR?

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

The failures observed on the dowstream distributed CI run are the same as those observed in the master branch (see for instance the last build in #471).

So +1 for merging this on my side. But before making a release I would like to better understand what's going on with distributed, irrespective of the changes in this PR.

@ogrisel
Copy link
Contributor

ogrisel commented May 20, 2022

I can reproduce the same 19 failures for distributed main against the released cloudpickle 2.0.0, so they are definitely not related to any recent change in cloudpickle.

Let me merge this and release 2.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants