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

Fix test_extract_class_dict for Python 3.13 beta 1 #534

Merged
merged 13 commits into from
Oct 11, 2024

Conversation

frenzymadness
Copy link
Contributor

Resolves: #533

@ogrisel
Copy link
Contributor

ogrisel commented Jun 19, 2024

Thanks for the PR. Ideally we would need to update the CI. Do you know if that can be easily achieved with GitHub Actions?

@frenzymadness
Copy link
Contributor Author

Python 3.13 is already available in setup-python action, see: https://github.com/actions/python-versions/blob/main/versions-manifest.json

So it should be easy to add 3.13 to the CI here.

@ogrisel
Copy link
Contributor

ogrisel commented Jul 31, 2024

The remaining failures seems to be all related to @tomMoral's tests for deterministic pickling implemented in #524.

I had a quick look but it's not easy to understand why cloudpickle with Python 3.13 no longer leads to deterministic pickling.

@ogrisel
Copy link
Contributor

ogrisel commented Jul 31, 2024

More specifically, for test_deterministic_dynamic_class_attr_ordering_for_chained_pickling, the first difference is the lack of memoization of the "CloudPickleTest.test_deterministic_dynamic_class_attr_ordering_for_chained_pickling.<locals>.A.__init__" string, which looks like the fully qualified name (__qualname__ attribute) of the __init__ method of the class being pickled in the test.

https://github.com/cloudpipe/cloudpickle/actions/runs/10183782743/job/28169551899?pr=534#step:6:168

@ogrisel
Copy link
Contributor

ogrisel commented Jul 31, 2024

@ogrisel
Copy link
Contributor

ogrisel commented Jul 31, 2024

Ok d6696c2 did the trick! I will update the changelog and wait some time to let @tomMoral review this change.

@QuLogic
Copy link
Contributor

QuLogic commented Sep 23, 2024

Python 3.13 final release is in a week, and this appears to be passing tests; is there any thing remaining to complete this (other than review, I suppose)?

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @frenzymadness and @ogrisel for the debug of this part.

Note that python 3.13 now have a sys.isinterned which might be useful to better investigate this issue.

@@ -707,7 +707,7 @@ def _function_getstate(func):
# Hack to circumvent non-predictable memoization caused by string interning.
# See the inline comment in _class_setstate for details.
"__name__": "".join(func.__name__),
"__qualname__": func.__qualname__,
"__qualname__": "".join(func.__qualname__),
Copy link
Contributor

Choose a reason for hiding this comment

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

It was previously not interned but now it is without rebuilding the string?
Not super clear why this behavior changed, I could not find any lead in the python3.13 release note.

But if it does the trick, this seems fine like this, this should not be too costly to do.

@ogrisel
Copy link
Contributor

ogrisel commented Oct 11, 2024

After triggering the tests again with CPython 3.13.0 we get new failures on macOS. Will investigate.

@ogrisel
Copy link
Contributor

ogrisel commented Oct 11, 2024

The __firstlineno__ is present in the first pickle string but missing from the second in:

            class A:
                """Simple class definition"""

                pass

            A_dump = w.run(cloudpickle.dumps, A)
            check_deterministic_pickle(A_dump, cloudpickle.dumps(A))

@pierreglaser
Copy link
Member

I did a bit more digging. In Python 3.13, setting the __module__ attribute of some class has the side effect of deleting its __firstlineno__ attribute:

In [1]: class A:
      ...:     pass
      ...:

In [2]: A.__firstlineno__
Out[2]: 1

In [3]: A.__module__ = "anymodule"

In [4]: A.__firstlineno__
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[4], line 1
----> 1 A.__firstlineno__

AttributeError: type object 'A' has no attribute '__firstlineno__'

Note that this snippet does not involve cloudpickle.

The cloudpickle reproducibility tests fail because of the folllowing sequence of events:

  • a class is created, and pickled using cloudpickle, generating a first pickle string. Since it's __module__ is left untouched in the process, the class has a __firstlineno__ attribute, which is pickled as part of the class's state by cloudpickle
  • at unpickling time, cloudpickle first restores the class's __firstlineno__ attribute, and then __module__, thus deleting the restored __firstlineno__ attribute.
  • this depickled class is then repickled another time, yielding a second pickle string, which differs from the first one as it not contain a __firstlineno__ attribute in it. Consequently, the test, which asserts the equality of the two strings, fails.

There is a question to be had about whether the __firstlineno__ attribute of a depickled dynamically created class has any valuable meaning. But from a user perspective, the least surprising behaviour that cloudpickle can have is probably to restore this argument as it was as unpickling time, which is what I propose we do.

cloudpickle/cloudpickle.py Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@pierreglaser
Copy link
Member

Merging, thanks @tomMoral @frenzymadness @ogrisel!

@pierreglaser pierreglaser merged commit b3bac2c into cloudpipe:master Oct 11, 2024
22 checks passed
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.

Test failure with Python 3.13.0b1
5 participants