-
Notifications
You must be signed in to change notification settings - Fork 167
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
Conversation
Thanks for the PR. Ideally we would need to update the CI. Do you know if that can be easily achieved with GitHub Actions? |
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. |
More specifically, for https://github.com/cloudpipe/cloudpickle/actions/runs/10183782743/job/28169551899?pr=534#step:6:168 |
Maybe this is the cause: https://github.com/cloudpipe/cloudpickle/pull/524/files#r1399015881 |
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)? |
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.
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__), |
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.
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.
After triggering the tests again with CPython 3.13.0 we get new failures on macOS. Will investigate. |
The class A:
"""Simple class definition"""
pass
A_dump = w.run(cloudpickle.dumps, A)
check_deterministic_pickle(A_dump, cloudpickle.dumps(A)) |
I did a bit more digging. In Python 3.13, setting the 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 The cloudpickle reproducibility tests fail because of the folllowing sequence of events:
There is a question to be had about whether the |
Co-authored-by: Olivier Grisel <[email protected]>
Merging, thanks @tomMoral @frenzymadness @ogrisel! |
Resolves: #533