Use CIRCUIT_SERIALIZER as default serializer for quantum engine#4983
Use CIRCUIT_SERIALIZER as default serializer for quantum engine#4983
Conversation
CIRCUIT_SERIALIZER can serialize gates from any other gateset, uses a more compact proto serialization that doesn't embed redundant string literals, and can serialize more complicated constructs such as CircuitOperation. It is meant to provide serialization separate from the notion of which gates are supported by a particular device, two concepts which were previously conflated. Having this as a default is a big UX improvement when using quantum engine, as the gate_set argument can be omitted on most calls. The gate_set argument could be removed in the future, as specifying the serializer once on EngineContext, along with the protobuf version, is sufficient to customize the serialization when needed.
maffoo
requested review from
a team,
cduck,
verult,
vtomole and
wcourtney
as code owners
| verbose: Optional[bool] = None, | ||
| timeout: Optional[int] = None, | ||
| context: Optional[EngineContext] = None, | ||
| serializer: Serializer = CIRCUIT_SERIALIZER, |
There was a problem hiding this comment.
In the future we'd like to remove SerializableGateset, making CIRCUIT_SERIALIZER the only serializer AFAIK. What if we don't provide an option to configure the serializer and always use CIRCUIT_SERIALIZER?
There was a problem hiding this comment.
We could certainly do that, though having it configurable gives us a way to test out CircuitSerializer2 when that comes out...
(I kid, but only slightly)
There was a problem hiding this comment.
I would lean towards keeping the code simpler now and adding this back when the use case comes up.
Another reason to keep the parameter is if we want to inject a mock to test that Engine calls the serializer with the expected circuit, but it doesn't look like we do that today. And I'm not sure if we should - if the logic for prepping the serializer call isn't complicated then such a test is probably overkill.
There was a problem hiding this comment.
Removed the option to customize this on Engine. I've kept it on EngineContext since that gets passed around to various places where needed and I'd prefer to access the serializer in one way on the context instead of hard-coding CIRCUIT_SERIALIZER everywhere it is used.
| repetitions: int = 1, | ||
| processor_ids: Sequence[str] = ('xmonsim',), | ||
| gate_set: Optional[Serializer] = None, | ||
| gate_set: Serializer = None, |
There was a problem hiding this comment.
If it's not too much hassle, could you add deprecation notices to these fields? In my discussions I haven't heard arguments against removing this field in the future.
There was a problem hiding this comment.
Could do that here, of in a separate PR; which do you prefer? I think there are probably still some usages of this parameter in tests and example notebooks that we'd want to change, so I might prefer making a separate PR to track all that down.
|
@verult, PTAL. |
maffoo
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
…tumlib#4983) `CIRCUIT_SERIALIZER` can serialize gates from any other gateset, uses a more compact proto serialization that doesn't embed redundant string literals, and can serialize more complicated constructs such as `CircuitOperation`. It is meant to provide serialization separate from the notion of which gates are supported by a particular device, two concepts which were previously conflated. Having this as a default is a big UX improvement when using quantum engine, as the `gate_set` argument can be omitted on most calls. The `gate_set` argument could be removed in the future, as specifying the serializer once on `EngineContext`, along with the protobuf version, is sufficient to customize the serialization when needed.
…tumlib#4983) `CIRCUIT_SERIALIZER` can serialize gates from any other gateset, uses a more compact proto serialization that doesn't embed redundant string literals, and can serialize more complicated constructs such as `CircuitOperation`. It is meant to provide serialization separate from the notion of which gates are supported by a particular device, two concepts which were previously conflated. Having this as a default is a big UX improvement when using quantum engine, as the `gate_set` argument can be omitted on most calls. The `gate_set` argument could be removed in the future, as specifying the serializer once on `EngineContext`, along with the protobuf version, is sufficient to customize the serialization when needed.
4 participants
CIRCUIT_SERIALIZERcan serialize gates from any other gateset, uses a more compact proto serialization that doesn't embed redundant string literals, and can serialize more complicated constructs such asCircuitOperation. It is meant to provide serialization separate from the notion of which gates are supported by a particular device, two concepts which were previously conflated. Having this as a default is a big UX improvement when using quantum engine, as thegate_setargument can be omitted on most calls. Thegate_setargument could be removed in the future, as specifying the serializer once onEngineContext, along with the protobuf version, is sufficient to customize the serialization when needed.