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

Bump minimum python version to 3.9 #6167

Merged
merged 26 commits into from Jun 29, 2023
Merged

Conversation

skushnir123
Copy link
Contributor

@skushnir123 skushnir123 commented Jun 27, 2023

Drops support for Python 3.7. and 3.8 For context, see issue: #6159

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tanujkhattar
Copy link
Collaborator

We should drop support for both 3.7 and 3.8 and bump up the minimum version to 3.9. Fixing #6018 would further add support for 3.11

@CirqBot CirqBot added the size: M 50< lines changed <250 label Jun 27, 2023
@skushnir123 skushnir123 marked this pull request as ready for review June 27, 2023 20:36
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Main comment is to also drop support for Python 3.8. You'll also need to update asv.conf.json to Python 3.9.

@@ -31,7 +31,7 @@ jobs:
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: '3.7'
python-version: '3.8'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update to 3.9, here and elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 3.9 everywhere, as well as dropping some of the conditional checks for 3.7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should make a follow-up ticket/issue to thoroughly search the code to ensure the cleanup form 3.7 and 3.8 is complete.

pyproject.toml Outdated
@@ -1,6 +1,6 @@
[tool.black]
line-length = 100
target_version = ['py37', 'py38', 'py39']
target_version = ['py38', 'py39']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also add py310 here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also add py310 here?

Yes, and py311 as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added both of these, and updated the black formatter because the old version didn't support 311

@pavoljuhas
Copy link
Collaborator

Please also check all instances of sys.version and sys.version_info in the code and adjust as needed.
For example, it should be possible to remove the use_async_mock decorator because it only applies to 3.7

uses_async_mock = pytest.mark.xfail(
sys.version_info < (3, 8, 0), reason='unittest.mock.AsyncMock is only available in python 3.8+'
)

Another thing to check are all python_version specifiers in the requirement *.txt files.
For example, the dependency below can be removed if we require Python >= 3.9.

# functools.cached_property was introduced in python 3.8
backports.cached_property~=1.0.1; python_version < '3.8'

@skushnir123
Copy link
Contributor Author

Main comment is to also drop support for Python 3.8. You'll also need to update asv.conf.json to Python 3.9.

Yup, made these changes

@maffoo maffoo changed the title dropped 3.7 support Bump minimum python version to 3.9 Jun 29, 2023
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Almost there, however there are few more things relevant only for abandoned Pythons. Please consider updating your PR with

git pull https://github.com/pavoljuhas/Cirq.git pr6167-amend-1

skushnir123/Cirq@drop-37...pavoljuhas:pr6167-amend-1

There are also Dockerfile-s that use old python and will need update, but that is perhaps better left for another PR - unless they block the tests here, #6175.

@skushnir123
Copy link
Contributor Author

git pull https://github.com/pavoljuhas/Cirq.git pr6167-amend-1

Thanks for adding on those changes! I just added those to my PR. Let me know if there is anything else to remove/change.

@skushnir123
Copy link
Contributor Author

Almost there, however there are few more things relevant only for abandoned Pythons. Please consider updating your PR with

git pull https://github.com/pavoljuhas/Cirq.git pr6167-amend-1

skushnir123/Cirq@drop-37...pavoljuhas:pr6167-amend-1

There are also Dockerfile-s that use old python and will need update, but that is perhaps better left for another PR - unless they block the tests here, #6175.

I don't think the Dockerfile's are failing any tests here.

Copy link
Collaborator

@pavoljuhas pavoljuhas 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 taking care of this!

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Thanks Samuel and Pavol! I've removed Python 3.7 and 3.8 from required CI checks and the PR is ready to be merged now!

@tanujkhattar tanujkhattar merged commit bc09047 into quantumlib:master Jun 29, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants