Skip to content

verbosify cirq_type deprecation #5339

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 5 commits into from
May 20, 2022
Merged

verbosify cirq_type deprecation #5339

merged 5 commits into from
May 20, 2022

Conversation

95-martin-orion
Copy link
Collaborator

Fixes #5337.

More specifically, this message is intended to make it very clear that:

  1. cirq_type will persist as a field in JSON-serialized objects
  2. cirq_type is now auto-generated
  3. Users should remove cirq_type from their _json_dict_ methods
  4. Classes outside Cirq should define _json_namespace_ to avoid collisions

@95-martin-orion 95-martin-orion requested a review from Strilanc May 9, 2022 18:09
@95-martin-orion 95-martin-orion requested review from a team, vtomole and cduck as code owners May 9, 2022 18:09
@CirqBot CirqBot added the size: S 10< lines changed <50 label May 9, 2022
"its _json_namespace_ method. Starting in v0.15, custom 'cirq_type' values "
"will trigger an error."
"\n\n"
"To fix this, remove 'cirq_type' from the class _json_dict_ method. Classes "
Copy link
Collaborator

@tanujkhattar tanujkhattar May 9, 2022

Choose a reason for hiding this comment

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

Should we also mention that serialized objects will continue to be the same as previously serialized objects? I think cirq_type will persist as a field in JSON-serialized objects was not a 100% clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Emphasized this in the preceding paragraph. FWIW, this only preserves serialization format if a class follows the [namespace.]classname convention, but we've been exceedingly consistent about applying that convention in Cirq. Hopefully that translates to external users doing the same.

@tanujkhattar tanujkhattar self-assigned this May 9, 2022
@95-martin-orion
Copy link
Collaborator Author

@Strilanc, I'd still like your confirmation on this before merging, since you were the one affected by the previous wording here.

@Strilanc
Copy link
Contributor

Shouldn't 'how to fix this' vary depending on whether the manually assigned value agrees with the automatically assigned value or not? If I was using a custom cirq_type value, how would I override the automatic behavior? How would I guarantee cross-compatibility across versions with/without this deprecation?

@95-martin-orion
Copy link
Collaborator Author

Okay, I think I hit the points raised above.

If I was using a custom cirq_type value, how would I override the automatic behavior?

You can't - the point of this change was to enforce consistency in cirq_types. The motivation for this decision and discussion around it can be found on PR #4693.

How would I guarantee cross-compatibility across versions with/without this deprecation?

As described in #5337, you'd need to include both cirq_types in the deserialization dict. This means that external classes which opted not to use a namespace will never be totally safe from the risk of colliding with a future Cirq class name, but that was true before this change as well.

LMK if there's anything else you'd like to see covered here.

"define _json_namespace_ for the class."
"\n\n"
"For backwards compatibility, classes whose old 'cirq_type' does not match the "
"new value must appear in `json_resolver_cache.py::_class_resolver_dictionary()` "
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this instruction. My project does not have a json_resolver_cache.py file. Is this a file within cirq? Say it is within cirq, and give the full file path not the name.

Also, this seems rather extreme to require someone who wrote a different library to make a change in cirq to maintaing compatibility across versions? It sounds like this is saying I can't maintain compatibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I misunderstood how Cirq handles third-party JSON resolvers. The appropriate thing to update for this is whatever custom resolver you are using, as described here. I'll update the instructions to match.

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 20, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 20, 2022
@CirqBot CirqBot merged commit 0f4c10f into master May 20, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 20, 2022
@CirqBot CirqBot deleted the verbose-cirq-type-depr branch May 20, 2022 14:31
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 20, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Fixes quantumlib#5337.

More specifically, this message is intended to make it very clear that:

1. `cirq_type` will persist as a field in JSON-serialized objects
2. `cirq_type` is now auto-generated
3. Users should remove `cirq_type` from their `_json_dict_` methods
4. Classes outside Cirq should define `_json_namespace_` to avoid collisions
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Fixes quantumlib#5337.

More specifically, this message is intended to make it very clear that:

1. `cirq_type` will persist as a field in JSON-serialized objects
2. `cirq_type` is now auto-generated
3. Users should remove `cirq_type` from their `_json_dict_` methods
4. Classes outside Cirq should define `_json_namespace_` to avoid collisions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It's too late to deprecate others using cirq_type in their json
4 participants