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

Run tests against python 3.10 #5486

Merged
merged 13 commits into from
Jul 16, 2022
Merged

Run tests against python 3.10 #5486

merged 13 commits into from
Jul 16, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Jun 10, 2022

Test against all the versions.

@maffoo maffoo requested review from a team, vtomole and cduck as code owners June 10, 2022 22:36
@maffoo maffoo requested a review from dabacon June 10, 2022 22:36
@CirqBot CirqBot added the Size: XS <10 lines changed label Jun 10, 2022
@maffoo
Copy link
Contributor Author

maffoo commented Jun 13, 2022

Looks like we will need to update the grpc library version to support 3.10.

@MichaelBroughton
Copy link
Collaborator

Looks like the least intrusive upgrade here is grpcio-tools~=1.36.0 in protos.txt. This would take the bundled protoc to 3.14 from 3.13.0. Would this interfere with any of the cirq-google gapic or proto stuff @maffoo , @verult , @wcourtney ?

@verult
Copy link
Collaborator

verult commented Jul 14, 2022

I'm not aware of any problems with device and program protos. If we can go further and move to 3.15, that'd be even better - optional fields are only supported in >=3.15 and I'd like to make the gate_duration field optional if time allows before Cirq 1.0. But this is by no means a strong requirement.

@maffoo
Copy link
Contributor Author

maffoo commented Jul 14, 2022

@MichaelBroughton, I don't think bumping the grpcio-tools version will be a problem for cirq-google; we will have to regenerate the proto files, but that's easy, and the gapic code now uses the proto library instead of google.protobuf so it doesn't care about the protoc version.

@verult, in proto3 all fields are optional by default, so I don't think we need to do anything special to treat gate_duration as optional. Do you have any more information about the change in 3.15 that you're interested in?

@verult
Copy link
Collaborator

verult commented Jul 14, 2022

The feature I'm looking for is to be able to distinguish between an unset field and a field which is set to the default value of the type. For gate_duration, it's to be able to tell whether a gate has 0 duration vs no duration information. But this isn't a strong requirement because gate_duration is mostly informational.

See: https://stackoverflow.com/questions/42622015/how-to-define-an-optional-field-in-protobuf-3

@maffoo maffoo requested review from wcourtney and verult as code owners July 14, 2022 05:29
@CirqBot CirqBot added the size: XL lines changed >1000 label Jul 14, 2022
@maffoo
Copy link
Contributor Author

maffoo commented Jul 14, 2022

Hm, we need to update mypy to get it to install on mac/windows with 3.10, but updating mypy update is not trivial. I'm inclined to disable 3.10 tests for windows and mac for now and do that as a follow-up.

@MichaelBroughton
Copy link
Collaborator

Why is updating mypy not trivial ?

@maffoo
Copy link
Contributor Author

maffoo commented Jul 14, 2022

When I tried updating mypy to ~0.800 I saw O(20) new type errors; when I updated to the latest 0.961 it was more like O(50) errors. It might be that these are easy to fix, I didn't dive too deep. But in any even it felt like that might be better as a separate PR.

@maffoo
Copy link
Contributor Author

maffoo commented Jul 15, 2022

mypy updated in #5767 so that we can now run 3.10 tests on windows and mac. @MichaelBroughton, PTAL.

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.

LGTM.

Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

me wants this

(making new cirq virtualenv and grpcio-tools fails to build, hoping this is the fix)

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 16, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 16, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 16, 2022

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 16, 2022
@viathor viathor merged commit 6cf0109 into master Jul 16, 2022
@viathor viathor deleted the u/maffoo/3.10 branch July 16, 2022 00:36
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: XL lines changed >1000 Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants