Add ability to add custom tag (de)serializers #7169

dstrain115 · 2025-03-20T17:21:43Z

This adds custom tag serializers for serializing tags.
This allows greater flexibility and a much easier way to specify a serializer to serialize and deserialize internal tags.

- This adds custom tag serializers for serializing tags. - This allows greater flexibility and a much easier way to specify a serializer to serialize and deserialize internal tags.

codecov · 2025-03-20T18:04:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.13%. Comparing base (8cde2ba) to head (24b8253).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7169      +/-   ##
==========================================
- Coverage   98.13%   98.13%   -0.01%     
==========================================
  Files        1093     1095       +2     
  Lines       95579    95649      +70     
==========================================
+ Hits        93797    93864      +67     
- Misses       1782     1785       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pavoljuhas

LGTM with some small tweaks, please see inline.

pavoljuhas · 2025-03-20T18:42:40Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        self.discount = discount
+
+    def __eq__(self, other):
+        if isinstance(other, DiscountTag) and self.discount == other.discount:


Nit - __eq__ should return NotImplemented for unhandled types (I realize this is a test class). Consider using attrs instead which define __eq__ and __hash__ with less code

@attrs.frozen class DiscountTag: discount: float

Good idea. Done.

pavoljuhas · 2025-03-20T18:56:30Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        ),
+        constants=[v2.program_pb2.Constant(qubit=v2.program_pb2.Qubit(id='1_1'))],
+    )
+    expected_circuit_no_tag = cirq.Circuit(


Nit - the _no_tag suffix is a bit confusing, because there is a tagged operation.
How about plain expected_circuit?

pavoljuhas · 2025-03-20T19:33:46Z

cirq-google/cirq_google/serialization/circuit_serializer.py

                    constant = v2.program_pb2.Constant()
                    tag_index = len(constants)


We can use constant in both if-branches. It is also safer to get tag_index after the tag_serializer.to_proto() call in case it changes the constants list:

diff --git a/cirq-google/cirq_google/serialization/circuit_serializer.py b/cirq-google/cirq_google/serialization/circuit_serializer.py index b76b23f7..ae940471 100644 --- a/cirq-google/cirq_google/serialization/circuit_serializer.py +++ b/cirq-google/cirq_google/serialization/circuit_serializer.py @@ -331,17 +331,17 @@ class CircuitSerializer(serializer.Serializer): tag.to_proto(msg=msg.tags.add()) if (tag_index := raw_constants.get(tag, None)) is None: constant = v2.program_pb2.Constant() - tag_index = len(constants) if self.tag_serializer and self.tag_serializer.can_serialize_tag(tag): - tag_proto = self.tag_serializer.to_proto( - tag, constants=constants, raw_constants=raw_constants + self.tag_serializer.to_proto( + tag, + msg=constant.tag_value, + constants=constants, + raw_constants=raw_constants, ) - constants.append(v2.program_pb2.Constant(tag_value=tag_proto)) - if raw_constants is not None: - raw_constants[tag] = tag_index - msg.tag_indices.append(tag_index) elif getattr(tag, 'to_proto', None) is not None: tag.to_proto(constant.tag_value) # type: ignore + if constant.WhichOneof('const_value'): + tag_index = len(constants) constants.append(constant) if raw_constants is not None: raw_constants[tag] = tag_index

pavoljuhas · 2025-03-20T19:35:35Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        msg: Optional[v2.program_pb2.CircuitOperation] = None,
+        *,
+        constants: List[v2.program_pb2.Constant],
+        raw_constants: Dict[Any, int],
+    ) -> v2.program_pb2.CircuitOperation:


Typo in annotation -

Suggested change

msg: Optional[v2.program_pb2.CircuitOperation] = None,

*,

constants: List[v2.program_pb2.Constant],

raw_constants: Dict[Any, int],

) -> v2.program_pb2.CircuitOperation:

msg: Optional[v2.program_pb2.Tag] = None,

*,

constants: List[v2.program_pb2.Constant],

raw_constants: Dict[Any, int],

) -> v2.program_pb2.Tag:

dstrain115

Thanks for the quick review!

dstrain115 · 2025-03-20T20:19:42Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        msg: Optional[v2.program_pb2.CircuitOperation] = None,
+        *,
+        constants: List[v2.program_pb2.Constant],
+        raw_constants: Dict[Any, int],
+    ) -> v2.program_pb2.CircuitOperation:


dstrain115 · 2025-03-20T20:19:48Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        ),
+        constants=[v2.program_pb2.Constant(qubit=v2.program_pb2.Qubit(id='1_1'))],
+    )
+    expected_circuit_no_tag = cirq.Circuit(


dstrain115 · 2025-03-20T20:20:05Z

cirq-google/cirq_google/serialization/circuit_serializer.py

                    constant = v2.program_pb2.Constant()
                    tag_index = len(constants)


dstrain115 · 2025-03-20T20:20:16Z

cirq-google/cirq_google/serialization/circuit_serializer_test.py

+        self.discount = discount
+
+    def __eq__(self, other):
+        if isinstance(other, DiscountTag) and self.discount == other.discount:


Good idea. Done.

pavoljuhas · 2025-03-20T21:04:07Z

cirq-google/cirq_google/serialization/circuit_serializer.py

+                    if constant.WhichOneof('const_value'):
                        constants.append(constant)
                        if raw_constants is not None:
                            raw_constants[tag] = tag_index


tag_index is unassigned here. nice catch by mypy.

pavoljuhas

LGTM modulo the tag_index typo

* Add ability to add custom tag (de)serializers - This adds custom tag serializers for serializing tags. - This allows greater flexibility and a much easier way to specify a serializer to serialize and deserialize internal tags. * Fix types. * Fix coverage * Address comments. * Fix unassigned variable.

Add ability to add custom tag (de)serializers

087c371

- This adds custom tag serializers for serializing tags. - This allows greater flexibility and a much easier way to specify a serializer to serialize and deserialize internal tags.

dstrain115 requested review from a team, verult, vtomole and wcourtney as code owners March 20, 2025 17:21

dstrain115 added 2 commits March 20, 2025 10:40

Fix types.

9770c34

Fix coverage

023a2f4

dstrain115 requested a review from senecameeks March 20, 2025 19:29

pavoljuhas requested changes Mar 20, 2025

View reviewed changes

Address comments.

01560e7

dstrain115 commented Mar 20, 2025

View reviewed changes

dstrain115 requested a review from pavoljuhas March 20, 2025 20:20

pavoljuhas reviewed Mar 20, 2025

View reviewed changes

pavoljuhas approved these changes Mar 20, 2025

View reviewed changes

Fix unassigned variable.

24b8253

dstrain115 added this pull request to the merge queue Mar 20, 2025

Merged via the queue into quantumlib:main with commit 43d033b Mar 20, 2025
38 checks passed

dstrain115 deleted the custom_tag_serializers branch March 20, 2025 21:52

mhucka mentioned this pull request Apr 18, 2025

Improve reporting of errors & efficiency of pr-size-labeler #7288

Merged

		constant = v2.program_pb2.Constant()
		tag_index = len(constants)

Add ability to add custom tag (de)serializers #7169

Add ability to add custom tag (de)serializers #7169

Uh oh!

Conversation

dstrain115 commented Mar 20, 2025

Uh oh!

codecov bot commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pavoljuhas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstrain115 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavoljuhas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 20, 2025 •

edited

Loading