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

[WIP] Add support for Python 3.13 #8904

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

[WIP] Add support for Python 3.13 #8904

wants to merge 19 commits into from

Conversation

phofl
Copy link
Collaborator

@phofl phofl commented Oct 24, 2024

Closes #xxxx

  • Tests added / passed
  • Passes pre-commit run --all-files

@phofl phofl requested a review from fjetter as a code owner October 24, 2024 09:48
Copy link
Contributor

github-actions bot commented Oct 24, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    29 files  +    4      29 suites  +4   12h 20m 35s ⏱️ + 2h 4m 14s
 4 130 tests ±    0   4 011 ✅  -     3    111 💤 +  1   8 ❌ +3 
55 839 runs  +8 146  53 398 ✅ +7 834  2 426 💤 +304  15 ❌ +9 

For more details on these failures, see this check.

Results for commit e3819b6. ± Comparison against base commit d7eff77.

This pull request skips 1 test.
distributed.tests.test_metrics ‑ test_monotonic

♻️ This comment has been updated with latest results.

@graingert
Copy link
Member

Basic Constraints of CA cert not marked critical

this is python/cpython#107361 I'd recommend using https://trustme.readthedocs.io/en/latest/ to generate certs

Copy link
Member

@jacobtomlinson jacobtomlinson left a 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

@jrbourbeau jrbourbeau mentioned this pull request Nov 7, 2024
6 tasks
Copy link
Member

@jrbourbeau jrbourbeau left a 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
Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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?

Suggested change
# - git+https://github.com/dask-contrib/dask-expr
- git+https://github.com/dask/dask-expr

Copy link
Collaborator Author

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

Copy link
Member

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

Copy link
Collaborator Author

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

@@ -174,6 +176,13 @@ jobs:
|| steps.cache.outputs.cache-hit != 'true'
)

- name: Install PyArrow nightlies for Python 3.13
Copy link
Member

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?

Copy link
Member

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

@jrbourbeau jrbourbeau changed the title Add ci job for python 3.13 [WIP] Add support for Python 3.13 Nov 8, 2024
@jrbourbeau jrbourbeau added the skip-caching Apply to PRs to disable CI environment caching label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-caching Apply to PRs to disable CI environment caching
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants