Streamline proto serialization of circuits with duplicate operations #6991
Conversation
- Some circuits, such as randomized benchmarking circuits, have many repeated operations. - Streamline the proto serialization of these circuits so that these operations are stored only once (in the constants table). Note that this PR only adds deserialization by default and hides serialization behind a feature flag. Once this is deployed everywhere, then we can enable serialization.
dstrain115
requested review from
wcourtney,
vtomole,
verult and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6991 +/- ##
=======================================
Coverage 97.89% 97.89%
=======================================
Files 1085 1085
Lines 95098 95121 +23
=======================================
+ Hits 93096 93119 +23
Misses 2002 2002 ☔ View full report in Codecov by Sentry. |
| deserialized_constants=deserialized_constants, | ||
| ) | ||
| ) | ||
| elif which_const == 'moment_value': |
There was a problem hiding this comment.
WDYT of adding a catch-all else to handle cases where a new value was added but this block wasn't updated? It looks like we would silently ignore those cases today.
There was a problem hiding this comment.
Good idea. Done.
| ], | ||
| ) | ||
| print(serializer.serialize(circuit)) |
There was a problem hiding this comment.
Did you intend to leave this print statement? I think there's a preference to avoid output noise in the CI due to its monolithic nature. If this is only intended for development, would it work to use a logger with a low severity level (e.g. debug) so that you can enable this only when needed? I'm not sure how logging is set up for cirq/clients.
There was a problem hiding this comment.
This was purely for debugging. Removed.
There was a problem hiding this comment.
No, this was just for debugging. Removed.
| @@ -389,8 +389,10 @@ def test_serialize_deserialize_circuit(): | |||
| assert serializer.deserialize(proto) == circuit | |||
|
|
|||
|
|
|||
| def test_serialize_deserialize_circuit_with_moments_table(): | |||
| serializer = cg.CircuitSerializer(USE_CONSTANTS_TABLE_FOR_MOMENTS=True) | |||
| def test_serialize_deserialize_circuit_with_constants_table(): | |||
There was a problem hiding this comment.
Nit: separately test specific behaviors when possible rather than a monolithic block demonstrating multiple behaviors. e.g., prefer tests with titles that can be phased with clear "given", "when", and "then" clauses.
https://testing.googleblog.com/2014/04/testing-on-toilet-test-behaviors-not.html
There was a problem hiding this comment.
I am not sure we want to do that in this case. Testing all combinations of feature flags is not really a viable use case, so I am not sure we want to separate them out. Both of these flags are here so that we don't serialize something that cannot be deserialized on the server-side. Once both of these changes are deployed, we will switch the defaults and get rid of the flags. Separating into two tests is just going to mean we will need to clean up more tests later.
(Arguably, we could just have one feature flag I guess, but I think it makes sense to keep them separate even if we will later likely remove both at the same time).
| elif which_const == 'moment_value': | ||
| # For now, just put in a place holder, since moments | ||
| # will be deserialized later. | ||
| deserialized_constants.append(None) |
There was a problem hiding this comment.
Is this a bug that is being resolved? If so, can you add a test that would have have failed without this fix but passes now?
There was a problem hiding this comment.
The existing tests will catch it. This was a preexisting bug, but it did not appear since the moments would only be at the end of the constants table. In one of the tests, there are multiple moments and operations, which intersperses them in the list, so this test will fail if this is removed.
…uantumlib#6991) * Streamline proto serialization of circuits with duplicate operations - Some circuits, such as randomized benchmarking circuits, have many repeated operations. - Streamline the proto serialization of these circuits so that these operations are stored only once (in the constants table). Note that this PR only adds deserialization by default and hides serialization behind a feature flag. Once this is deployed everywhere, then we can enable serialization. * fix type * Addressed some of the comments. * coverage
…uantumlib#6991) * Streamline proto serialization of circuits with duplicate operations - Some circuits, such as randomized benchmarking circuits, have many repeated operations. - Streamline the proto serialization of these circuits so that these operations are stored only once (in the constants table). Note that this PR only adds deserialization by default and hides serialization behind a feature flag. Once this is deployed everywhere, then we can enable serialization. * fix type * Addressed some of the comments. * coverage
Note that this PR only adds deserialization by default and hides serialization behind a feature flag. Once this is deployed everywhere, then we can enable serialization.
Example serialization of a randomize benchmarking circuit with 70 qubits and 500 cliffords:
Before change:
After change: