-
Notifications
You must be signed in to change notification settings - Fork 145
Enable testing on Python 3.12 #400
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
Conversation
Compare to: #376 |
The motivation for this PR is:
|
py{37,38,39,310,311,312}: pytest-asyncio | ||
|
||
; For pkg_resources | ||
py{37,38,39,310,311,312}: setuptools |
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 am still not very clear on why this dependency is needed. You mentioned it was added since a test imports pkg_resources
, which test are you referring to?
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.
Specifically:
from pkg_resources import resource_filename |
with open(resource_filename(__name__, 'mock_sampling_rule.json')) as f: |
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.
The pkg_resources
module is deprecated, but this test appears to be intentionally and specifically verifying compatibility with it.
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.
Probably only “necessary” on Python 3.12 though:
gh-95299: Do not pre-install setuptools in virtual environments created with venv. This means that distutils, setuptools, pkg_resources, and easy_install will no longer available by default; to access these run pip install setuptools in the activated virtual environment.
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.
Perhaps, but it’s not harmful on others. The dependency is still real; it’s just allowed to be implicit. I guess I can change to py312
only if implicit is better than explicit.
@@ -16,13 +16,14 @@ jobs: | |||
py39: 3.9 | |||
py310: '3.10' | |||
py311: '3.11' | |||
py312: '3.12' |
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.
FYI: Should have worked at the time with 3.12.0-beta - 3.12
. But it's available now, see https://github.com/actions/python-versions/blob/main/versions-manifest.json
Maybe rebase to trigger a rerun?
Required by the core tests in tests/test_local_sampling_benchmark.py.
Co-authored-by: Carol Abadeer <[email protected]>
Hmm, I’m not sure why this is failing on Python 3.12 with |
aws-xray-sdk-python/.github/workflows/UnitTesting.yaml Lines 42 to 43 in 8563afc
|
Ensure it is available for creating the sdist on Python 3.12+. aws#400 (comment)
Thanks. Hopefully 2a682c9 is correct and sufficient. |
Issue #, if available:
Description of changes:
Added an explicitly dependency on
setuptools
, since a test importspkg_resources
.Added Python 3.12 versions of all the
tox
environments that supported Python 3.11.Added Python 3.12 to GitHub unit testing workflow.
I did not add Python 3.12 to the trove classifiers, but I’m happy to add this if it’s wanted.
Additional information:
Testing this PR requires #399.
It may also be necessary to use a very recent version of
pip
:Most tests pass. Some HTTP tests seem to fail or flake in all Python versions due to transient issues at https://httpbin.org/. At a glance, regressions specific to Python 3.12 appear to be due to certain dependencies not being ready.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.