dataclass_json_dict #4391

mpharrigan · 2021-08-06T18:54:19Z

Dataclasses keep track of their relevant fields, so we can automatically generate these.
Dataclasses are implemented with somewhat complex metaprogramming, and tooling (PyCharm, mypy)
have special cases for dealing with classes decorated with @dataclass. There is very little
support (and no plans for support) for decorators that wrap @dataclass (like
@cirq.json_serializable_dataclass) or combining additional decorators with @dataclass.
Although not as elegant, you may want to consider explicitly defining _json_dict_ on your
dataclasses which simply return dataclass_json_dict(self).

maffoo

I like this a lot. Can we deprecate json_serializable_dataclass?

mpharrigan · 2021-08-13T22:34:58Z

I wouldn't be opposed

maffoo

Minor comments, then LGTM.

maffoo · 2021-08-13T22:35:58Z

cirq-core/cirq/__init__.py

@@ -500,6 +500,7 @@
    is_parameterized,
    JsonResolver,
    json_serializable_dataclass,
+    dataclass_json_dict,


nit: sort imports

maffoo · 2021-08-13T22:37:19Z

cirq-core/cirq/experiments/xeb_fitting.py

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 """Estimation of fidelity associated with experimental circuit executions."""
+import dataclasses
 from abc import abstractmethod, ABC
 from dataclasses import dataclass


nit: remove this import and use @dataclasses.dataclass throughout.

maffoo · 2021-08-13T22:41:56Z

cirq-core/cirq/protocols/__init__.py

@@ -88,6 +88,7 @@
    to_json,
    read_json,
    obj_to_dict_helper,
+    dataclass_json_dict,


I was going to say sort imports, but it looks like we haven't been doing that consistently. Really needs to be automated...

Craig and I are in solidarity that we won't manually sort imports

maffoo · 2021-08-13T22:43:12Z

cirq-core/cirq/protocols/json_serialization.py

+        if TYPE_CHECKING:
+            from dataclasses import dataclass as json_serializable_dataclass
+        else:
+            from cirq.protocols import json_serializable_dataclass


I would vote for removing this from the docs and deprecating json_serializable_dataclass, but I guess that could be a future PR.

removed the mypy hack. Propose punting the deprecation to a follow-on PR. opened #4460

maffoo · 2021-08-13T22:44:30Z

cirq-core/cirq/protocols/json_serialization.py

+
+    Dataclasses keep track of their relevant fields, so we can automatically generate these.
+
+    Dataclasses are implemented with somewhat complex metaprogramming, and tooling (PyCharm, mypy)


This paragraph is largely duplicated in the module docstring above. I'd suggest removing it here and relying on the module docstring.

what do you mean module docstring? do you mean the docstring for json_serializable_dataclass? I think it's relevant to both functions. Especially if we remove json_serializable_dataclass

Ah sorry, I thought other occurence of this text was in the json_serialization.py module docstring. Agree it's fine to have on json_serializable_dataclass.

maffoo · 2021-08-13T22:46:17Z

cirq-core/cirq/protocols/json_serialization_test.py

@@ -782,6 +782,24 @@ def custom_resolver(name):
    assert_json_roundtrip_works(my_dc, resolvers=[custom_resolver] + cirq.DEFAULT_RESOLVERS)


+def test_json_serializable_dataclass_no_decorator():


I'd suggest naming this so it stands on its own, e.g. test_dataclass_json_dict (then no change needed if json_serializable_dataclass goes away).

cirq-core/cirq/protocols/json_serialization_test.py

google-cla · 2021-08-16T16:38:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-23T22:27:23Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mpharrigan · 2021-08-23T22:27:58Z

@googlebot i fixed it

google-cla · 2021-08-23T22:28:08Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

mpharrigan · 2021-08-23T22:28:46Z

note to self: never ever accept suggestion in the github ui or clabot will lose it

mpharrigan · 2021-08-23T22:29:02Z

@maffoo PTAL

maffoo

LGTM

maffoo · 2021-08-23T22:58:01Z

cirq-core/cirq/protocols/json_serialization.py

+
+    Dataclasses keep track of their relevant fields, so we can automatically generate these.
+
+    Dataclasses are implemented with somewhat complex metaprogramming, and tooling (PyCharm, mypy)


Ah sorry, I thought other occurence of this text was in the json_serialization.py module docstring. Agree it's fine to have on json_serializable_dataclass.

Dataclasses keep track of their relevant fields, so we can automatically generate these. Dataclasses are implemented with somewhat complex metaprogramming, and tooling (PyCharm, mypy) have special cases for dealing with classes decorated with `@dataclass`. There is very little support (and no plans for support) for decorators that wrap `@dataclass` (like `@cirq.json_serializable_dataclass`) or combining additional decorators with `@dataclass`. Although not as elegant, you may want to consider explicitly defining `_json_dict_` on your dataclasses which simply `return dataclass_json_dict(self)`.

google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 6, 2021

CirqBot added size: XL lines changed >1000 size: S 10< lines changed <50 and removed size: XL lines changed >1000 labels Aug 12, 2021

mpharrigan marked this pull request as ready for review August 12, 2021 23:02

mpharrigan requested review from cduck, mrwojtek, vtomole and a team as code owners August 12, 2021 23:02

mpharrigan requested a review from balopat August 12, 2021 23:02

maffoo reviewed Aug 12, 2021

View reviewed changes

maffoo approved these changes Aug 13, 2021

View reviewed changes

MichaelBroughton assigned maffoo Aug 14, 2021

google-cla bot added cla: no and removed cla: yes Makes googlebot stop complaining. labels Aug 16, 2021

This was referenced Aug 23, 2021

[Obs] 4.5 - High-level API #4392

Merged

Deprecate json_serializable_dataclass #4460

Closed

mpharrigan added 2 commits August 23, 2021 15:25

dataclass_json_dict

c98728b

test, use

d768570

mpharrigan force-pushed the 2021-08-json-dataclass branch from befbb4e to 46c3bec Compare August 23, 2021 22:27

mpharrigan added 4 commits August 23, 2021 15:28

Update cirq-core/cirq/protocols/json_serialization_test.py

e083c6d

Rename test

9db9f91

Use dataclasses module instead of dataclass

e6c184d

Don't include mypy hack dock

c856b49

mpharrigan force-pushed the 2021-08-json-dataclass branch from 46c3bec to c856b49 Compare August 23, 2021 22:28

google-cla bot added cla: yes Makes googlebot stop complaining. and removed cla: no labels Aug 23, 2021

maffoo approved these changes Aug 23, 2021

View reviewed changes

mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 23, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 23, 2021

CirqBot merged commit 886fc51 into quantumlib:master Aug 23, 2021

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dataclass_json_dict #4391

dataclass_json_dict #4391

mpharrigan commented Aug 6, 2021 •

edited

Loading

maffoo left a comment

mpharrigan commented Aug 13, 2021

maffoo left a comment

maffoo Aug 13, 2021

maffoo Aug 13, 2021

mpharrigan Aug 23, 2021

maffoo Aug 13, 2021

mpharrigan Aug 16, 2021

maffoo Aug 13, 2021

mpharrigan Aug 23, 2021 •

edited

Loading

maffoo Aug 13, 2021

mpharrigan Aug 23, 2021

maffoo Aug 23, 2021

maffoo Aug 13, 2021

mpharrigan Aug 23, 2021

google-cla bot commented Aug 16, 2021

google-cla bot commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

google-cla bot commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

maffoo left a comment

maffoo Aug 23, 2021


		Dataclasses keep track of their relevant fields, so we can automatically generate these.

		Dataclasses are implemented with somewhat complex metaprogramming, and tooling (PyCharm, mypy)

		@@ -782,6 +782,24 @@ def custom_resolver(name):
		assert_json_roundtrip_works(my_dc, resolvers=[custom_resolver] + cirq.DEFAULT_RESOLVERS)


		def test_json_serializable_dataclass_no_decorator():

dataclass_json_dict #4391

dataclass_json_dict #4391

Conversation

mpharrigan commented Aug 6, 2021 • edited Loading

maffoo left a comment

Choose a reason for hiding this comment

mpharrigan commented Aug 13, 2021

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Aug 16, 2021

google-cla bot commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

google-cla bot commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

mpharrigan commented Aug 23, 2021

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan commented Aug 6, 2021 •

edited

Loading

mpharrigan Aug 23, 2021 •

edited

Loading