Preparations to remove the proto field Program.language.gate_set #5709
Conversation
verult
requested review from
wcourtney,
a team,
vtomole and
cduck
as code owners
| """ | ||
| super().__init__(gate_set_name) | ||
| def __init__(self): | ||
| """Construct the circuit serializer object.""" |
There was a problem hiding this comment.
I think you can keep the Args in here.
There was a problem hiding this comment.
Why do you prefer keeping it? I'm leaning towards removing it now because it's not currently useful, and it's easier to add a field later on than to remove one.
There was a problem hiding this comment.
Nm, ignore the last comment. I thought you got rid of the doc string but not the argument. This is fine.
| @@ -619,4 +601,4 @@ def _deserialize_circuit_op( | |||
| ) | |||
|
|
|||
|
|
|||
| CIRCUIT_SERIALIZER = CircuitSerializer('v2_5') | |||
| CIRCUIT_SERIALIZER = CircuitSerializer() | |||
There was a problem hiding this comment.
Will this barf server-side if the language is not present? I can't remember how we determine whether this is v2 or v2.5 protos currently.
There was a problem hiding this comment.
Current it does. My plan is to update this logic on server side as well. @maffoo and I confirmed this should be the order of merging:
- Remove SerializableGateSet
- Change server side to only accept CircuitSerializer, and throw an error if the
gate_setfield doesn't match CircuitSerializer - Update CircuitSerializer on Cirq side
We also agreed to not remove Program.language.gate_set in the end, so that users on old Cirq versions won't fail silently when using QCS. If we choose to remove it in the future, it would be safer to do it later after more users upgrade past Cirq 1.0.
So I'll hold off on merging this PR for now.
There was a problem hiding this comment.
Yeah, I think #2 needs to happen first, since server side needs to support the new client. (maybe throw an error if gate_set is not empty and does not equal v2_5 for now).
|
Synced with @dstrain115 and @maffoo offline. Will keep the This PR is being replaced by #5769. The |
@dstrain115