-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
[WIP] Add support for Python 3.13 #8904
base: main
Are you sure you want to change the base?
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 29 files + 4 29 suites +4 12h 24m 17s ⏱️ + 1h 54m 11s For more details on these failures, see this check. Results for commit 85002bf. ± Comparison against base commit 0d905d5. This pull request skips 1 test.
♻️ This comment has been updated with latest results. |
this is python/cpython#107361 I'd recommend using https://trustme.readthedocs.io/en/latest/ to generate certs |
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.
🎉 thanks for handling this @phofl
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.
Thanks @phofl
@@ -326,6 +326,8 @@ def _get_tls_context(self, tls, purpose): | |||
# IP addresses rather than hostnames | |||
ctx.check_hostname = False | |||
|
|||
ctx.verify_flags &= ~ssl.VERIFY_X509_STRICT |
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.
I've gone ahead and turned this new VERIFY_X509_STRICT
flag off here (it was previously off by default in older versions of Python but was flipped on by default in Python 3.13). This seems not ideal, but also not that bad given that's what we've always done prior to Python 3.13. @jacobtomlinson (or anyone else) let me know if you have a better suggestion, I'm not verify familiar with our TLS security code paths.
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.
using https://github.com/python-trio/trustme to generate the certificates used for testing on the fly would get this to pass (as it sets the appropriate flags on the certificates and generally keeps these flags up to date)
of course, distributed users would also need to configure their certificates to meet these stronger security standards
- setuptools < 71 | ||
- pip: | ||
- git+https://github.com/dask/dask | ||
# - git+https://github.com/dask-contrib/dask-expr |
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.
I'm not sure why this is commented out. @phofl is this still needed?
# - git+https://github.com/dask-contrib/dask-expr | |
- git+https://github.com/dask/dask-expr |
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.
dask-expr pulls in pyarrow which wasn't compatible with python 3.13
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.
Ah, I see. pyarrow=18
was recently released and has Python 3.13 support, so I think we should be okay to add this back in then
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.
Yes definitely, I was waiting for that
.github/workflows/tests.yaml
Outdated
@@ -174,6 +176,13 @@ jobs: | |||
|| steps.cache.outputs.cache-hit != 'true' | |||
) | |||
|
|||
- name: Install PyArrow nightlies for Python 3.13 |
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.
Do we still need this?
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.
Nope, not anymore. Just removed it
Closes #xxxx
pre-commit run --all-files