Skip to content

Allow ability to plug in custom (de)serializers for cirq_google protos #7059

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.

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

Merged
merged 6 commits into from
Feb 16, 2025

Conversation

dstrain115
Copy link
Collaborator

  • This will allow users to plug in custom serializers and deserializers, which can parse gates before falling back to the default.
  • This enables internal libraries to parse and deserialize non-public gates, tags, and operations.

- This will allow users to plug in custom serializers and deserializers,
which can parse gates before falling back to the default.
- This enables internal libraries to parse and deserialize non-public
gates, tags, and operations.
@dstrain115 dstrain115 requested review from wcourtney, vtomole, verult and a team as code owners February 11, 2025 23:41
@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 11, 2025
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (34b9c3b) to head (1122030).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7059   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files        1089     1089           
  Lines       95208    95237   +29     
=======================================
+ Hits        93478    93507   +29     
  Misses       1730     1730           

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

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please see comment on temporary predicate lambda objects. Otherwise LGTM.

On a bit unrelated note - can you please apply the patch below to suppress intentional warnings in pytest cirq-google/cirq_google/serialization/circuit_serializer_test.py.
Note it flips the test names so they are in sync with the warnings produced.

diff --git a/cirq-google/cirq_google/serialization/circuit_serializer_test.py b/cirq-google/cirq_google/serialization/circuit_serializer_test.py
index a09de744..a1d1cfc3 100644
--- a/cirq-google/cirq_google/serialization/circuit_serializer_test.py
+++ b/cirq-google/cirq_google/serialization/circuit_serializer_test.py
@@ -859,5 +859,6 @@ def test_circuit_with_tag(tag):
 
 
-def test_unknown_tag_is_ignored():
+@pytest.mark.filterwarnings('ignore:Unrecognized Tag .*DingDongTag')
+def test_unrecognized_tag_is_ignored():
     class DingDongTag:
         pass
@@ -869,5 +870,6 @@ def test_unknown_tag_is_ignored():
 
 
-def test_unrecognized_tag_is_ignored():
+@pytest.mark.filterwarnings('ignore:Unknown tag msg=phase_match')
+def test_unknown_tag_is_ignored():
     op_tag = v2.program_pb2.Operation()
     op_tag.xpowgate.exponent.float_value = 1.0

Comment on lines 34 to 35
def can_deserialize_predicate(self) -> Callable[[Any], bool]:
"""The method used to determine if this can deserialize a proto."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a regular method returning a bool instead? Or a staticmethod if you want to have it decoupled from OpDeserializer object?

As it is, each call of CircuitOpDeserializer.can_deserialize_proto creates lambda object, calls it once, and discards it after the call.

(Same for OpSerializer.can_serialize_predicate.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I don't remember why this was done originally, but it is no longer needed.

@@ -31,13 +31,12 @@ class OpDeserializer(abc.ABC):

@property
@abc.abstractmethod
def serialized_id(self) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update class docstring which refers to serializer_id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Also warnings suppressed.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM, but the warning filters need to be flipped so that
check/pytest -n0 cirq-google/cirq_google/serialization/circuit_serializer_test.py
has no warnings to report.

Currently the test_unknown_tag_is_ignored test gives warning about unrecognized tag
and test_unrecognized_tag_is_ignored about unknown tag. (My previous patch had test names flipped for test-name / warning consistency).

diff --git a/cirq-google/cirq_google/serialization/circuit_serializer_test.py b/cirq-google/cirq_google/serialization/circuit_serializer_test.py
index cd067ffb..d2c06e78 100644
--- a/cirq-google/cirq_google/serialization/circuit_serializer_test.py
+++ b/cirq-google/cirq_google/serialization/circuit_serializer_test.py
@@ -860,3 +860,3 @@ def test_circuit_with_tag(tag):
 
-@pytest.mark.filterwarnings('ignore:Unknown tag msg=phase_match')
+@pytest.mark.filterwarnings('ignore:Unrecognized Tag .*DingDongTag')
 def test_unknown_tag_is_ignored():
@@ -871,3 +871,3 @@ def test_unknown_tag_is_ignored():
 
-@pytest.mark.filterwarnings('ignore:Unrecognized Tag .*DingDongTag')
+@pytest.mark.filterwarnings('ignore:Unknown tag msg=phase_match')
 def test_unrecognized_tag_is_ignored():

@dstrain115 dstrain115 added this pull request to the merge queue Feb 16, 2025
Merged via the queue into quantumlib:main with commit ca6ceb3 Feb 16, 2025
38 checks passed
@dstrain115 dstrain115 deleted the add_custom_serializers branch February 16, 2025 23:07
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
quantumlib#7059)

* Allow ability to plug in custom (de)serializers for cirq_google protos

- This will allow users to plug in custom serializers and deserializers,
which can parse gates before falling back to the default.
- This enables internal libraries to parse and deserialize non-public
gates, tags, and operations.

* Fix coverage and get rid of unneeded junk.

* Address comments.

* Flip warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants