-
Notifications
You must be signed in to change notification settings - Fork 661
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
MAINT: Python 3.12 support improve #4300
Conversation
* Fixes MDAnalysisgh-4299, and allows the testsuite to have 12 failures instead of 56 with Python `3.12.0rc3` for me locally on x86_64 Linux; full suite still passing for Python `3.11.x` * the main idea is to circumvent more aggressive CPython blocks on serialization by overriding the highest-priority pickling method available, `__reduce_ex__`, which CPython had locked down, making it such that our old `__getstate__` pickling shims were ignored as indicators of pickling safety * there is probably a slightly simpler diff that allows this to work, but this took a few hours to draft so feel free to push in simplifications if you're sure you've tested on 3.11.x and 3.12 RC [skip cirrus]
Hello @tylerjereddy! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Linter Bot Results:Hi @tylerjereddy! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Thanks @tylerjereddy - should we temporarily merge my Py3.12 PR knowing it fails and then just turn off the cron shim to test here? |
We could I suppose. Do we even need a cron guard? The final CPython |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4300 +/- ##
===========================================
- Coverage 93.40% 93.36% -0.05%
===========================================
Files 170 184 +14
Lines 22257 23374 +1117
Branches 4071 4071
===========================================
+ Hits 20790 21823 +1033
- Misses 951 1035 +84
Partials 516 516
☔ View full report in Codecov by Sentry. |
* force the use of a relative path for `mkdtemp` in our source code because, from the Python `3.12` release notes: > `tempfile.mkdtemp() now always returns an absolute path, even if the argument provided to the dir parameter is a relative path.` * with this patch, the full testsuite now passes on Python `3.12.0` (which is now out) when combined with MDAnalysisgh-4301 and MDAnalysisgh-4300 (though see my cautions about thread safety/concurrency for the latter) * this fixes the 1 remaining failure with 3.12: `MDAnalysisTests/analysis/test_hole2.py::TestCheckAndFixLongFilename::test_symlink_dir` [skip cirrus]
* force the use of a relative path for `mkdtemp` in our source code because, from the Python `3.12` release notes: > `tempfile.mkdtemp() now always returns an absolute path, even if the argument provided to the dir parameter is a relative path.` * with this patch, the full testsuite now passes on Python `3.12.0` (which is now out) when combined with gh-4301 and gh-4300 (though see my cautions about thread safety/concurrency for the latter) * this fixes the 1 remaining failure with 3.12: `MDAnalysisTests/analysis/test_hole2.py::TestCheckAndFixLongFilename::test_symlink_dir` [skip cirrus]
Fixes Python 3.12 cannot pickle PickleBufferIOPicklable instances #4299, and allows the testsuite to have 12 failures instead of 56 with Python
3.12.0rc3
for me locally on x86_64 Linux; full suite still passing for Python3.11.x
the main idea is to circumvent more aggressive CPython blocks on serialization by overriding the highest-priority pickling method available,
__reduce_ex__
, which CPython had locked down, making it such that our old__getstate__
pickling shims were ignored as indicators of pickling safetythere is probably a slightly simpler diff that allows this to work, but this took a few hours to draft so feel free to push in simplifications if you're sure you've tested on 3.11.x and 3.12 RC
also, I believe this does slightly increase the chance of hanging the testsuite at very high levels of concurrency, but I needed more than 30 processes to repro that, so it may just be a feature of our testsuite in general (it was fine on 12 cores at least, but on 32 or so file locks had issues I think)
[skip cirrus]
📚 Documentation preview 📚: https://mdanalysis--4300.org.readthedocs.build/en/4300/