Repository navigation
Fix the import with python -OO and a free-threading race, and other parity follow-ups - #5
Merged
Merged
Conversation
With python -OO (or PYTHONOPTIMIZE=2), importing cloudpickle_rust has raised TypeError: 'None' is not an instance of 'str' since 0.3.0: the docstrings of the Pickler methods implemented in Rust (reducer_override) are compiled when the module is imported, so that they match upstream's, and in this mode the compiler removes them, which functions::compiled_doc did not handle. compiled_doc now returns None in that case, and the method descriptor gets no docstring. Like upstream's, the functions and methods then have no docstring, and neither do the module and _DispatchTable. A new test compares the docstrings of the module, its functions and the Pickler methods with upstream's in Python processes run without flags, with -O and with -OO (where cloudpickle_rust must be importable). Co-authored-by: Isaac <no-reply@databricks.com>
Unlike upstream cloudpickle, which is pure Python, cloudpickle_rust cannot be imported in subinterpreters, for instance the ones of concurrent.interpreters (new in Python 3.14): PyO3 extension modules do not support them yet (PyO3/pyo3#576, with no opt-in in PyO3 0.28), and the import raises ImportError. README.md now lists it among the differences with upstream, and a test checks it on Python 3.14 and later. Co-authored-by: Isaac <no-reply@databricks.com>
Loading a payload of a tracked class sets the attributes of the class again (_class_setstate): on Python 3.13+, setting __module__ also deletes __firstlineno__ until it is set again. _extract_class_dict, like upstream's, read the __dict__ of the class (and of its bases) several times, so pickling the class meanwhile in another thread could raise KeyError: '__firstlineno__' or RuntimeError: dictionary changed size during iteration on free-threaded builds (upstream cloudpickle does). It now reads a snapshot, taken with mappingproxy.copy(), which copies the namespace atomically also on free-threaded builds. The result is the same as before when the class does not change. A new test pickles a class while other threads load payloads of it: it fails 10 times out of 10 with upstream cloudpickle on CPython 3.14t, and passes with cloudpickle-rust. Co-authored-by: Isaac <no-reply@databricks.com>
- On Python 3.13 and later, Pickler gets the __firstlineno__ and __static_attributes__ that the compiler sets for upstream's class statement, and _empty_cell_value is defined by upstream's code (taken from the bundled cloudpickle.py and run at the same line), so that its class also has upstream's __firstlineno__. - The type stub of _extract_code_globals_cache is the WeakKeyDictionary that it is since 0.3.0. - README.md: the pickles of tests/old_pickles were generated with cloudpickle-rust 0.1.0. - The payloads are compared with upstream's for all the pickle protocols (protocols 0 and 1 cannot pickle some of the objects: the exception must then be upstream's) and for more kinds of objects: enums with __init__, IntFlag, StrEnum, NamedTuple, TypedDict, Protocol, metaclasses, namedtuple, dataclasses with slots and KW_ONLY, PEP 695 generics, positional-only parameters, generators and coroutines. The namespace test also compares the namespace of Pickler, and the test of _empty_cell_value the __firstlineno__ of its class. Co-authored-by: Isaac <no-reply@databricks.com>
The previous commit takes the definition of _empty_cell_value from the bundled src/upstream/cloudpickle.py by looking for a two-line snippet. On Windows, the repository is checked out with CRLF line endings by default, so the source embedded by include_str! has them, the snippet was not found, and the import failed (the Windows jobs of the CI). The search now ignores the carriage returns. Checked locally with a CRLF copy of the bundled source: the import works, the line numbers are upstream's, and the test-suite passes. Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A follow-up of #3 and #4. I compared the Rust implementation line by line with upstream's
cloudpickle/cloudpickle.pyagain (4af5936, still the head of upstream's master) and reviewed the commits since 0.2.0. This found one regression and one undocumented difference with upstream, plus an upstream bug and a few smaller differences, all fixed here.import cloudpickle_rustfails withpython -OO(regression in 0.3.0)Since #3, the docstrings of the
Picklermethods implemented in Rust (reducer_override) are compiled when the module is imported (functions::compiled_doc), so that they match upstream's on Python 3.13+. With-OO(orPYTHONOPTIMIZE=2), the compiler removes them, andcompiled_docfailed to convert the resultingNone. 0.2.0 imports fine.compiled_docnow returnsNonein that case, and the method descriptor then gets no docstring. As with upstream under-OO, the functions and methods have no docstring. Neither do the module (whose docstring otherwise also states the license of cloudpickle-rust) and_DispatchTable.Subinterpreters
Upstream cloudpickle can be imported in subinterpreters, for instance the ones of
concurrent.interpreters(new in Python 3.14). There,cloudpickle_rustraisesImportError: module cloudpickle_rust.cloudpickle does not support loading in subinterpreters, like every PyO3 extension module (PyO3/pyo3#576; PyO3 0.28 has no opt-in). The README now lists this among the differences with upstream, and a test checks it on Python 3.14 and later.Pickling a class while other threads load it (free-threaded builds; also an upstream bug)
Loading a payload of a tracked class sets the attributes of the class again (
_class_setstate). On Python 3.13+, setting__module__also deletes__firstlineno__until it is set again._extract_class_dictread the__dict__of the class several times, like upstream's. So pickling the class meanwhile in another thread can raiseKeyError: '__firstlineno__'orRuntimeError: dictionary changed size during iterationon free-threaded builds, and upstream cloudpickle does too. It now reads a snapshot (mappingproxy.copy(), atomic also on free-threaded builds), with the same result when the class does not change. The new test fails in 10 runs out of 10 with upstream on 3.14t, and passes here.Smaller differences
Picklergets the__firstlineno__and__static_attributes__that the compiler sets for upstream's class statement._empty_cell_valueis defined by upstream's code itself, taken from the bundledcloudpickle.pyand run at the same line, so its class also has upstream's__firstlineno__._extract_code_globals_cacheis theweakref.WeakKeyDictionarythat it has been since 0.3.0 (mypy.stubtestnow only reports upstream's leaked loop variableskandv).tests/old_pickleswere generated with cloudpickle-rust 0.1.0.test_payloads_identical_to_upstreamcovers all the pickle protocols. Protocols 0 and 1 cannot pickle some of the objects (theInitVarof dataclasses), and the exception must then be upstream's. It also covers more kinds of objects: enums with__init__,IntFlag,StrEnum,NamedTuple,TypedDict,Protocol, metaclasses,namedtuple, dataclasses withslotsandKW_ONLY, PEP 695 generics, positional-only parameters, generators and coroutines.Verification
tests/cloudpickle_rust_test.py:test_docstrings_with_optimizations: run in a subprocess with no flag, with-Oand with-OO,cloudpickle_rustimports and pickles. The docstrings of the module, its functions and thePicklermethods are upstream's (allNonewith-OO).test_subinterpreters_are_not_supported(Python 3.14 and later).test_pickling_a_class_while_other_threads_load_it, and the extendedtest_payloads_identical_to_upstream,test_module_namespace_matches_upstreamandtest_empty_cell_value_is_an_instance_of_a_python_class.python -OO,cloudpickle_rustimports and round-trips a lambda on all of them.-OOon CPython 3.13, upstream's suite run against upstream cloudpickle and this suite give the same outcome for every test. 5 tests fail in both, because they check docstrings.Generic, PEP 695 generics andtypealiases, dataclasses (includingslotsandKW_ONLY),Enum,Flag,StrEnumand enums with__init__, modules, files, loggers, dict views, cells and code objects.__cause__and__suppress_context__for unpicklable objects and invalid arguments;Picklersubclasses that overridereducer_override,_function_reduce,_function_getnewargs,persistent_idordispatch_table; the registry; and the private helpers.cargo fmt --check,cargo clippy --all-targets -- -D warnings, black 25.9.0 and ruff 0.14.3 pass.This pull request and its description were written by Isaac.