Bump the proto requirements to match the generated data. #4473

tonybruguier · 2021-08-26T03:22:28Z

It seems PR #4333 used a newer version of the proto syntax, but the requirements have not been updated, so this PR bumps them,

As a check, I decided to execute the follow commands:

python3 -m pip install protobuf~=3.17.3
python3 -m pip install grpcio-tools~=1.34.1
python3 -m pip install grpcio~=1.34.1
python3 -m pip install mypy-protobuf==1.10

./dev_tools/build-protos.sh

And the generated protos are unchanged, so maybe it's correct?

MichaelBroughton

This is actually a pain point for compatibility between TFQ and CIrq. We manually compile our .proto files using grpcio-tools (which comes with a recommended protoc inside) and keep them in the repo. Right now, the release version of TensorFlow Quantum depends on TensorFlow 2.4.1 which requires proto 3.9.2, but will work up to cirq having proto 3.13.0 or lower. Is this proto bump happening because you found something breaking or because there were warnings ?

tonybruguier · 2021-08-26T23:31:31Z

@MichaelBroughton
Thanks for the quick turn-around! So while working on bumping up the Cirq version in TFQ:
tensorflow/quantum#621

I encountered an error:

    import cirq
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq/__init__.py", line 596, in <module>
    _compat.deprecated_submodule(
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq/_compat.py", line 594, in deprecated_submodule
    new_module = importlib.import_module(new_module_name)
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq_google/__init__.py", line 17, in <module>
    from cirq_google import api
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq_google/api/__init__.py", line 16, in <module>
    from cirq_google.api import v2
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq_google/api/v2/__init__.py", line 16, in <module>
    from cirq_google.api.v2 import (
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq_google/api/v2/batch_pb2.py", line 14, in <module>
    from . import program_pb2 as cirq__google_dot_api_dot_v2_dot_program__pb2
  File "/opt/hostedtoolcache/Python/3.8.11/x64/lib/python3.8/site-packages/cirq_google/api/v2/program_pb2.py", line 21, in <module>
    create_key=_descriptor._internal_create_key,
AttributeError: module 'google.protobuf.descriptor' has no attribute '_internal_create_key'

https://github.com/tensorflow/quantum/pull/621/checks?check_run_id=3425375706

I was able to repro locally, and I was able to fix by bumping the version of protobuf. But, from what I now understand, this is a naive approach and bumping the version won't work.

Is there another way? Maybe PR #4333 was compiled with the wrong version of grpcio-tool. Is so, maybe a way out is to compile with the right version? If so, I can try it but pointers would be welcome.

…oto_version

tonybruguier · 2021-08-28T20:38:17Z

I think I was able to resolve the issue, @MichaelBroughton . Instead of bumping the requirements, I had to regenerate the files.

I also added a discussion item for next week's Cirq sync.

maffoo · 2021-08-30T18:49:36Z

We used to have a CI check that would generate protos with the expected version of the proto compiler and fail if there were any diffs compared to what was in the PR. Is that no longer running? If there's a diff like this I would have expected either a previous PR to be rejected, or this one.

MichaelBroughton · 2021-09-01T23:47:06Z

From Cirq Cync: Could you regenerate and test with the following:

grpcio-tools and grpc==1.34.1
protobuf==3.17.3

And then bump: https://github.com/quantumlib/Cirq/blob/master/cirq-google/requirements.txt
to 3.17.3 ? and see if things still work ? This might also solve #4161

…oto_version

tonybruguier · 2021-09-02T03:50:31Z

Thanks. PTAL.

MichaelBroughton · 2021-09-02T23:29:39Z

So you regnerated all the pb2.py files using these versions and nothing changed ?

tonybruguier · 2021-09-02T23:57:15Z

So you regnerated all the pb2.py files using these versions and nothing changed ?

Yes, precisely. I used the commands in the description.

MichaelBroughton · 2021-09-03T00:08:52Z

Awesome. @dstrain115 @maffoo does this work for you guys to merge ?

maffoo · 2021-09-03T16:49:20Z

I think the build-changed-protos ci step is not doing what we want. It's not clear to me that the script has been updated to reflect the latest packaging changes, and I think we are being too clever by trying to detect protos that have changed and only compile those. I think ci should instead compile all the protos (e.g. run dev_tools/build-protos.sh as recommended in the dev docs) and then look for any diffs. There aren't that many protos so this is plenty fast, and it will greatly simplify the logic.

For this PR, I think updating the protobuf library is probably ok, but I'd prefer to update the grpcio-tools version in the dev_tools proto deps to reflect the version that we actually want to use to generate protos and then update the protobuf library to match whatever is used by the grpcio-tools version we settle on (grpcio-tools bundles some version of protoc). We should also fix the ci proto step.

tonybruguier · 2021-09-03T18:31:16Z

@maffoo

Thanks for answering. I am a little bit confused though. I did run dev_tools/build-protos.sh. In fact, these are the commands that I ran:

python3 -m pip install protobuf~=3.17.3
python3 -m pip install grpcio-tools~=1.34.1
python3 -m pip install grpcio~=1.34.1
python3 -m pip install mypy-protobuf==1.10

./dev_tools/build-protos.sh

After running the commands, the proto files reverted themselves to their version at head. In other words, I did not manually revert the changes. Instead, I kept the intermediate (and incorrect) changes that I had, and by running the script, they naturally got reverted.

Sorry if I am still confused and thanks for having taken the time to review the changes.

tonybruguier · 2021-09-03T19:08:14Z

Like this:
https://www.youtube.com/watch?v=-nqJoXaS064

maffoo · 2021-09-03T21:10:24Z

@tonybruguier, I think the problem is that the .proto files we have committed at HEAD are not correct because our CI check is broken. The CI should be compiling with grpcio-tools==1.26.0 which is what is listed in the the proto deps: https://github.com/quantumlib/Cirq/blob/master/dev_tools/requirements/deps/protos.txt#L6. If you do that you'll see a big diff compared to what is committed at HEAD. But our CI check didn't detect the problem whenever the latest proto changes were committed.

tonybruguier · 2021-09-04T02:34:21Z

@maffoo Thanks. I looked into the test code and I think I might have found an issue: It seems that it was only checking changed files, which is not correct I think. When we bump the requirements, we don't modify the proto definitions, yet their compiled version might be different.

I tried the fix the unit test, and also updated the requirements. Here's the log:
https://pipelines.actions.githubusercontent.com/j17N1UnkAeIifjAO1q0K1FVeXWuBVqg7pbz7SuXJuOPU7i2PHx/_apis/pipelines/1/runs/7241/signedlogcontent/23?urlExpires=2021-09-04T02%3A32%3A03.9197508Z&urlSigningMethod=HMACV1&urlSignature=TgNiRgwl%2BTvw15oO3CCoqHLXBegyfkkw2o89hUt19wc%3D

It now seems to be finding the protos and to re-generate them.

cirq-google/requirements.txt

check/build-changed-protos

tonybruguier · 2021-09-04T21:28:24Z

Thanks, @maffoo PTAL when you have the time. I appreciate the quick turn-around and the opportunity to learn.

maffoo

This is looking much better, thanks for working through these protobuf issues! I think we should run some tests to ensure that the ci task to check generated protobuf code is working. For example, make a change to a .proto file and push this branch without regenerating proto code and verify that the CI check fails. Similarly, could make an edit directly to one of the generated _pb2.py files and again verify that the CI check fails. This will ensure that we can keep our proto code properly synced going forward.

I'll let @MichaelBroughton weigh in on what protobuf version we actually want to upgrade to, but I do think we should keep that in sync here with our grpcio-tools version.

cirq-google/requirements.txt

dev_tools/requirements/deps/protos.txt

check/build-changed-protos

tonybruguier · 2021-09-07T23:32:38Z

Thanks, @maffoo
I left some comments unresolved for Michael to give some advice. I addressed the others. As for testing the code, I purposely mangled the protos with this commit:
60123dd
And I saw this error:
https://github.com/quantumlib/Cirq/pull/4473/checks?check_run_id=3539007607

…oto_version

tonybruguier · 2021-09-08T03:45:36Z

Closed remaining comments and sync'ed to head.

maffoo

Tooling looks good, thanks for fixing/simplifying the CI check. I'd suggest changing the protobuf dependency per Michael's suggestion and a few minor doc fixes, then we should be good to go.

check/build-changed-protos

cirq-google/requirements.txt

dev_tools/requirements/deps/protos.txt

…oto_version

tonybruguier · 2021-09-10T00:04:39Z

Thanks. PTAL.

maffoo

LGTM

…4473) It seems PR quantumlib#4333 used a newer version of the proto syntax, but the requirements have not been updated, so this PR bumps them, As a check, I decided to execute the follow commands: ``` python3 -m pip install protobuf~=3.17.3 python3 -m pip install grpcio-tools~=1.34.1 python3 -m pip install grpcio~=1.34.1 python3 -m pip install mypy-protobuf==1.10 ./dev_tools/build-protos.sh ``` And the generated protos are unchanged, so maybe it's correct?

Bump proto version

c48b985

google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 26, 2021

CirqBot added the Size: XS <10 lines changed label Aug 26, 2021

tonybruguier marked this pull request as ready for review August 26, 2021 03:57

tonybruguier requested review from cduck, vtomole, wcourtney and a team as code owners August 26, 2021 03:57

tonybruguier requested a review from dstrain115 August 26, 2021 03:57

tonybruguier mentioned this pull request Aug 26, 2021

Proto serialization v25 #4333

Merged

MichaelBroughton reviewed Aug 26, 2021

View reviewed changes

MichaelBroughton self-assigned this Aug 26, 2021

tonybruguier mentioned this pull request Aug 26, 2021

Increment requirement to Cirq to 0.13.1 tensorflow/quantum#621

Merged

tonybruguier added 2 commits August 28, 2021 19:25

Merge branch 'master' of github.com:quantumlib/Cirq into cirq_bump_pr…

62ccde7

…oto_version

Re-generate proto API

56f7678

CirqBot added the size: XL lines changed >1000 label Aug 28, 2021

Try regeneration again

75f54e1

tonybruguier changed the title ~~Bump proto version~~ Regenerate the proto API with the correct version Aug 28, 2021

tonybruguier added 2 commits September 2, 2021 03:15

Merge branch 'master' of github.com:quantumlib/Cirq into cirq_bump_pr…

2bc60ef

…oto_version

Regenerate again

2f5fd76

tonybruguier changed the title ~~Regenerate the proto API with the correct version~~ Bump the proto requirements to match the generated data. Sep 2, 2021

tonybruguier added 3 commits September 3, 2021 21:31

Update dev tool deps for protos

574a589

Maybe check all protos

136c7ee

More debug code

fc7b8b5

maffoo reviewed Sep 4, 2021

View reviewed changes

cirq-google/requirements.txt Outdated Show resolved Hide resolved

check/build-changed-protos Outdated Show resolved Hide resolved

Directly call the the building script

5c437db

maffoo reviewed Sep 7, 2021

View reviewed changes

cirq-google/requirements.txt Outdated Show resolved Hide resolved

dev_tools/requirements/deps/protos.txt Show resolved Hide resolved

check/build-changed-protos Show resolved Hide resolved

check/build-changed-protos Show resolved Hide resolved

tonybruguier added 3 commits September 7, 2021 23:05

Address some comments

7d4ce89

Purposely mangle the protos to make sure the test catches this

60123dd

Re-generate protos to undo the mangling

ca6b6f0

Merge branch 'master' of github.com:quantumlib/Cirq into cirq_bump_pr…

1531442

…oto_version

maffoo reviewed Sep 9, 2021

View reviewed changes

check/build-changed-protos Outdated Show resolved Hide resolved

cirq-google/requirements.txt Outdated Show resolved Hide resolved

dev_tools/requirements/deps/protos.txt Outdated Show resolved Hide resolved

tonybruguier added 2 commits September 9, 2021 18:25

Merge branch 'master' of github.com:quantumlib/Cirq into cirq_bump_pr…

96ff28b

…oto_version

Address comments

a23ea74

maffoo approved these changes Sep 10, 2021

View reviewed changes

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 10, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Sep 10, 2021

CirqBot merged commit 578c956 into quantumlib:master Sep 10, 2021

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 Sep 10, 2021

tonybruguier deleted the cirq_bump_proto_version branch September 10, 2021 23:21

Bump the proto requirements to match the generated data. #4473

Bump the proto requirements to match the generated data. #4473

Uh oh!

Conversation

tonybruguier commented Aug 26, 2021 • edited by maffoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelBroughton left a comment

Choose a reason for hiding this comment

Uh oh!

tonybruguier commented Aug 26, 2021 • edited by maffoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonybruguier commented Aug 28, 2021

Uh oh!

maffoo commented Aug 30, 2021

Uh oh!

MichaelBroughton commented Sep 1, 2021

Uh oh!

tonybruguier commented Sep 2, 2021

Uh oh!

MichaelBroughton commented Sep 2, 2021

Uh oh!

tonybruguier commented Sep 2, 2021

Uh oh!

MichaelBroughton commented Sep 3, 2021

Uh oh!

maffoo commented Sep 3, 2021

Uh oh!

tonybruguier commented Sep 3, 2021 • edited by maffoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonybruguier commented Sep 3, 2021

Uh oh!

maffoo commented Sep 3, 2021

Uh oh!

tonybruguier commented Sep 4, 2021

Uh oh!

Uh oh!

Uh oh!

tonybruguier commented Sep 4, 2021

Uh oh!

maffoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonybruguier commented Sep 7, 2021

Uh oh!

tonybruguier commented Sep 8, 2021

Uh oh!

maffoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonybruguier commented Sep 10, 2021

Uh oh!

maffoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonybruguier commented Aug 26, 2021 •

edited by maffoo

Loading

tonybruguier commented Aug 26, 2021 •

edited by maffoo

Loading

tonybruguier commented Sep 3, 2021 •

edited by maffoo

Loading