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

[V2] Add Python 3.12 Support #8897

Closed
wants to merge 12 commits into from
Closed

[V2] Add Python 3.12 Support #8897

wants to merge 12 commits into from

Conversation

aemous
Copy link
Contributor

@aemous aemous commented Sep 4, 2024

Issue #, if available:

Description of changes:
This PR adds changes needed to support Python version 3.12. A highlight of the changes are:

  • Updating Github workflows to run with Python 3.12.
  • Updating pyproject.toml, tox.ini, and test files to support changes introduced in Python 3.12, and deprecation warnings added in dependencies.
  • Install setuptools in all CI builds with Python 3.12+ where it's no longer included by default (similar to Add support for Python 3.12 #8106).
  • Fixing EKS test suite (similar to Add support for Python 3.12 #8106).
  • Fixing botocore smoke test integration suite. Specifically, I removed our assertion that no warnings are emitted. PyTest already guarantees this, and which we configure with pyproject.toml.
  • Updated a botocore unit test suite against Python 3.12's update to BufferedIOBase.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aemous
Copy link
Contributor Author

aemous commented Sep 5, 2024

In reply to the only failing check, it is due to a failed assertion on the version of urllib3. For some reason, the the regenerated portable-exe-win-lock.txt file expects 1.26.19, but our lockfile has 1.26.20 pinned. Both satisfy our pyproject.toml requirement of urllib3>=1.25.4,<1.27.

Copy link
Contributor

@hssyoo hssyoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, had questions on some changes being made. Let's also remember to add a new change entry.

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@@ -1,4 +1,5 @@
# Requirements we need to run our build jobs for the installers.
# We create the separation for cases where we're doing installation
# from a local dependency directory instead of requirements.txt.
PyInstaller==5.12.0
PyInstaller==5.13.2
pip==24.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we pinning pip?

scripts/ci/install Show resolved Hide resolved
Comment on lines 276 to 280
with warnings.catch_warnings(record=True) as caught_warnings:
response = method(**kwargs)
err_msg = f"Warnings were emitted during smoke test: {caught_warnings}"
assert len(caught_warnings) == 0, err_msg
assert 'Errors' not in response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this? The PR description doesn't go into what this is fixing. It just looks like we're getting less coverage. It looks like botocore never removed the warnings assertion here. We should avoid drifting if it's not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants