Skip to content
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

Add version compatibility policy guidelines for Cirq #5897

Merged
merged 4 commits into from
Oct 12, 2022

Conversation

tanujkhattar
Copy link
Collaborator

Cirq is now officially at v1.0 and we need an official policy for describing guidelines for version compatibility across cirq versions.

This PR adds a document describing the new official policy. Please take a look and leave comments / give feedback. I'd be very interested to hear about use cases which are potentially unclear from the policy so we can make any necessary improvements.

cc @mpharrigan @dstrain115

@tanujkhattar tanujkhattar requested review from a team and vtomole as code owners September 28, 2022 21:32
@tanujkhattar tanujkhattar requested a review from cduck as a code owner September 28, 2022 21:32
@CirqBot CirqBot added the size: M 50< lines changed <250 label Sep 28, 2022
* For example: We can change [SVG diagrams](https://github.com/quantumlib/Cirq/issues/5689) and [text diagrams](https://github.com/quantumlib/Cirq/issues/5688) to use a different symbol for all classically controlled operations.
* **Error behavior:** We may replace input validation errors with non-error behavior. For instance, we may change a class/function to compute a result instead of raising an error for a certain set of inputs, even if that error is documented. We also reserve the right to change the text of error messages.
* For example: If we decide to extend functionality of `PauliSumExponential` to support non-commuting Pauli’s, the [value error](https://github.com/quantumlib/Cirq/blob/e00767a2ef1233e82e9089cf3801a77e4cc3aea3/cirq-core/cirq/ops/pauli_sum_exponential.py#L53) in the constructor would be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you plan to guarantee around type preservation? Should types always be conserved, or is Liskov Substitution Principle good enough?

i.e. if there's a function f()->BaseClass that returns Subclass1, then could a new version change that to return Subclass2, so long as that object behaves the same from the BaseClass interface?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to ask a similar question. If a function used to return a np.int64 can we change it to return int? This is like 99% backwards compatible but not strictly so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should types always be conserved, or is Liskov Substitution Principle good enough?
Satisfying LSP is good enough and we should guarantee type preservation. This is because ideally we would like the policies to be flexible enough to add new features while ensuring that existing code works -- and satisfying LSP guarantees this.

Of course, if the function definition has f() -> Subclass1 and you change it to return f() -> Subclass2; then it's not allowed and it doesn't satisfy LSP.

If a function used to return a np.int64 can we change it to return int

This would not satisfy LSP and should be considered a breaking the change. The inverse however is fine (i.e. return np.int64 where we currently return np.int) since with return types; substituting a more general type to replace a more restrictive type is fine (everything that works with np.int should continue to work with `np.int64) but the inverse is not true.

Added a bullet point to highlight the specific case of type preservation. Thanks for highlighting!

docs/dev/versions.md Outdated Show resolved Hide resolved
* Private symbols: any function, class, etc. whose name start with **\_**
* Experimental and `cirq.contrib` symbols, see [below](#what-is-not-covered) for details.
* Note that the code in the [examples/](https://github.com/quantumlib/Cirq/tree/master/examples) and [dev\_tools/](https://github.com/quantumlib/Cirq/tree/master/dev_tools) directories is not reachable through the `cirq` Python module and is thus not covered by the compatibility guarantee. Similarly, symbols in vendor packages, like [cirq-google](https://quantumai.google/reference/python/cirq_google/all_symbols), [cirq-aqt](https://quantumai.google/reference/python/cirq_aqt/all_symbols) are also not covered by the compatibility guarantee.
* If a symbol is available through the `cirq` Python module or its submodules, but is not documented, then it is **not** considered part of the public API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does it mean to be documented? have a docstring? be listed on the website?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean having a docstring.

docs/dev/versions.md Outdated Show resolved Hide resolved
docs/dev/versions.md Outdated Show resolved Hide resolved

Cirq currently uses JSON serialization for serializing gates, operations and circuits. Many Cirq users serialize and store their circuits so they can be loaded and executed with a later release of Cirq. In compliance with [semver](https://semver.org/), serialized Json objects written with one version of Cirq can be loaded and evaluated with a later version of Cirq with the same major release.

We make additional guarantees for _supported_ Cirq serialized objects. A serialized object which was created using **only non-deprecated, non-experimental APIs** in Cirq major version N is a _supported serialization in version N_. Any serialized object supported in Cirq major version `N` can be loaded and executed with Cirq major version `N+1`. However, the functionality required to build or modify such an object may not be available anymore, so this guarantee only applies to the unmodified serialized objects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you switch from using N to N

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the note about "unmodified serialized objects". I can't imagine I'll be modifying my serialized objects by like manually editing the json (??). Do you mean we guarantee read-only access? i.e. you may not be able to re-serialize an old object the way it used to be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, so suppose you have an object that was first serialized in version N and you load it using version N + 2; modify it and then re-serialize the new object. There is no guarantee that you would be able to load this "modified serialized object" using version N + 1; even though it might logically refer to the same thing.

@mpharrigan
Copy link
Collaborator

Is it worth highlighting backwards-compatible-breaking-changes that we won't do that aren't as obvious? Like changing a default value / default precision; inserting a new argument-with-a-default not at the end of the list unless its in a kwargs-only section; changing arguments from positional to kwargs-only (but not vice-versa(?))

or is this the responsibility of PR reviewers to keep a keen eye for these things?

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

This looks good on the whole

Copy link
Collaborator Author

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Thanks for all the great comments! I've addressed all comments and updated the PR.

I'll wait for a while to let others also take a look before merging.

docs/dev/versions.md Outdated Show resolved Hide resolved
docs/dev/versions.md Outdated Show resolved Hide resolved
docs/dev/versions.md Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Oct 12, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Oct 12, 2022
@CirqBot CirqBot merged commit 86a9017 into master Oct 12, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Oct 12, 2022
@CirqBot CirqBot deleted the backwards_compatibility branch October 12, 2022 17:07
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Oct 12, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Cirq is now officially at v1.0 and we need an official policy for describing guidelines for version compatibility across cirq versions. 

This PR adds a document describing the new official policy. Please take a look and leave comments / give feedback. I'd be very interested to hear about use cases which are potentially unclear from the policy so we can make any necessary improvements.

cc @mpharrigan @dstrain115
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.

5 participants