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.

Sign up for GitHub

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

Jump to bottom

Increment requirement to Cirq to 0.13.1 #621

Merged
merged 32 commits into from
Nov 2, 2021
Merged

Increment requirement to Cirq to 0.13.1 #621

merged 32 commits into from
Nov 2, 2021

Conversation

tonybruguier
Copy link
Contributor

For issue #357 we would want to increase the version of Cirq used to be able to use new objects.

MichaelBroughton
MichaelBroughton reviewed Aug 26, 2021
View reviewed changes
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

It looks like cirq-rigetti is hurting us here. Can we see if just moving to depend on cirq-core and cirq-google fixes this issue ?

@tonybruguier tonybruguier mentioned this pull request Aug 26, 2021
Merged
@tonybruguier
Copy link
Contributor Author

@MichaelBroughton
Thanks! Excluding the Rigetti deps indeed helped, but then I encoutered another issue with the version of protobuf. I put more detail over at:
quantumlib/Cirq#4473

@tonybruguier tonybruguier mentioned this pull request Aug 27, 2021
Merged
@tonybruguier tonybruguier changed the title Increment requirement to Cirq to 0.12.0 Increment requirement to Cirq to 0.13.0 Oct 15, 2021
@MichaelBroughton
Copy link
Collaborator

Oops, looks like: quantumlib/Cirq#4167 might have broken us. We may have to take some extra steps here to be sure and "demote back down" any CCX or CCZ into ControlledGate(sub_gate=...) so that things keep working as intended.

@tonybruguier tonybruguier marked this pull request as ready for review October 18, 2021 06:22
@tonybruguier tonybruguier mentioned this pull request Oct 21, 2021
Open
@tonybruguier
Copy link
Contributor Author

I updated the code to add the issue that we had discussed. @MichaelBroughton It's ready for review when you have the time.

@tonybruguier tonybruguier changed the title Increment requirement to Cirq to 0.13.0 Increment requirement to Cirq to 0.13.1 Oct 27, 2021
@tonybruguier
Copy link
Contributor Author

I took the liberty of bumping the requirements to v0.13.1

MichaelBroughton
MichaelBroughton reviewed Nov 1, 2021
View reviewed changes
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

This looks great! One question: Have we looked into this: https://googleapis.dev/python/protobuf/latest/google/protobuf/message.html#google.protobuf.message.Message.SerializeToString ?

Maybe instead of adding these two new functions program_from_tensor_to_ascii_proto we could just add an argument to convert_to_tensor like deterministic_proto_serialization and see if that fixes things ?

@tonybruguier
Copy link
Contributor Author

It worked indeed. Thanks! I named the parameter deterministic_proto_serialize because the style guide limits things to 30 characters and chose its default value to be False to leave the serialization unchanged by default.

PTAL.

MichaelBroughton
MichaelBroughton approved these changes Nov 2, 2021
View reviewed changes
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelBroughton MichaelBroughton merged commit 2706655 into tensorflow:master Nov 2, 2021
@tonybruguier tonybruguier deleted the tfq_bump_cirq_version branch November 2, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelBroughton MichaelBroughton MichaelBroughton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tonybruguier @MichaelBroughton