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

Switching to pre-releases instead of cirq-unstable #3527

Merged
merged 5 commits into from
Dec 3, 2020

Conversation

balopat
Copy link
Contributor

@balopat balopat commented Nov 23, 2020

Switch to PyPi pre-releases instead of cirq-unstable.

Two major reasons for this change:

  1. in preparation for modules - for which we would have to duplicate all of them if we followed this pattern.
  2. As raised in Prevent installing both cirq and cirq-unstable. #3267, cirq-unstable doesn't play well with cirq installations

Fixes #3267.

A resulting state after a test-run for this looks like this on the Test PyPi:

image

The dev releases will still get the "this is an unstable version" notice for their description.

@balopat balopat requested review from cduck, vtomole, wcourtney and a team as code owners November 23, 2020 23:50
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 23, 2020
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

README.rst Outdated Show resolved Hide resolved
dev_tools/packaging/verify-published-package.sh Outdated Show resolved Hide resolved
setup.py Outdated
# If CIRQ_UNSTABLE_VERSION is set then we use cirq-unstable as the name of the package
# and update the version to this value.
# If CIRQ_UNSTABLE_VERSION is set then we update the version to this value.
# As it contains "dev" it will be a pre-release version on PyPi. See
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, this should be changed: "As it contains" -> "If it contains"

("As" implies we know it does, which I don't think we do.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can clarify that - but it is assumed that it contains dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it can contain whatever, but it is assumed that it contains devN, aN, bN, rcN hence the long description that changes it to the warning text below

@balopat balopat marked this pull request as draft November 24, 2020 01:44
@balopat
Copy link
Contributor Author

balopat commented Nov 24, 2020

Thanks @viathor - I'm going to have to put a pause on this as I discovered Craig's comment here, that pointed out that tensorflow and tfq-nightly have the same issue as cirq and cirq-unstable: #2431 (comment).
Also I need a bit more evidence that in a multi-module setting the pre-releases will play nicely together - i.e. whether I can setup for example pip install --pre cirq-pasqal should bring in the correct latest cirq... I'll convert this back to live if I'm convinced that this is the right way to go.

@mpharrigan
Copy link
Collaborator

Isn't Craig's comment further justification for using pre-releases?

@balopat
Copy link
Contributor Author

balopat commented Nov 25, 2020

Isn't Craig's comment further justification for using pre-releases?

I interpreted it as "if this is okay with tensorflow, it should be okay for us too to live with this" based on the context that he seemed to be "pro separate package".

@balopat
Copy link
Contributor Author

balopat commented Nov 25, 2020

Oh by the way some more updates on this:

  1. https://www.python.org/dev/peps/pep-0440/#developmental-releases:

Note

While they may be useful for continuous integration purposes, publishing developmental releases of pre-releases to general purpose public index servers is strongly discouraged, as it makes the version identifier difficult to parse for human readers. If such a release needs to be published, it is substantially clearer to instead create a new pre-release by incrementing the numeric component.

Developmental releases of post-releases are also strongly discouraged, but they may be appropriate for projects which use the post-release notation for full maintenance releases which may include code changes.

  1. There is no impact on dependency management whether we use the same package or separate ("unstable") packages. It is straightforward to generate requirements to the given version in the setup.py for each module.

I'm more concerned about the 1st point and playing nice in the python community. More motivating conversations around this:

The only issue I have with anaconda.org is that this is how we'd install modules:

pip install --pre --extra-index https://pypi.anaconda.org/cirq-unstable/simple cirq
pip install --pre --extra-index https://pypi.anaconda.org/cirq-unstable/simple cirq-google

@balopat
Copy link
Contributor Author

balopat commented Nov 25, 2020

Oh by the way some more updates on this:

  1. https://www.python.org/dev/peps/pep-0440/#developmental-releases:

Note
While they may be useful for continuous integration purposes, publishing developmental releases of pre-releases to general purpose public index servers is strongly discouraged, as it makes the version identifier difficult to parse for human readers. If such a release needs to be published, it is substantially clearer to instead create a new pre-release by incrementing the numeric component.
Developmental releases of post-releases are also strongly discouraged, but they may be appropriate for projects which use the post-release notation for full maintenance releases which may include code changes.

  1. There is no impact on dependency management whether we use the same package or separate ("unstable") packages. It is straightforward to generate requirements to the given version in the setup.py for each module.

I'm more concerned about the 1st point and playing nice in the python community. More motivating conversations around this:

The only issue I have with anaconda.org is that this is how we'd install modules:

pip install --pre --extra-index https://pypi.anaconda.org/cirq-unstable/simple cirq
pip install --pre --extra-index https://pypi.anaconda.org/cirq-unstable/simple cirq-google

Oh hold on - the first point is irrelevant, it is just a strong recommendation around non-human-readable version numbers. Ignore that.

I was for a second also worried about "littering" the main package with dev releases for readability - but maybe it's not that big of a deal. Also, pre-releases like v1.0.0.rc1, v1.0.0.rc2 can still work, just pip install --pre cirq will be shadowed by the dev releases, so one will have to use the specific versions there instead explicitly.
2. In terms of space, I am actually not that worried about it. With 1.6MB per release and ~800 commits per year we are below a 1GB per year.

So the only question remains is readability of different versions on the cirq pypi page...

@balopat balopat marked this pull request as ready for review December 2, 2020 22:36
@balopat
Copy link
Contributor Author

balopat commented Dec 2, 2020

In the end I am ready to merge this as it will make our (less packages to deal with) and our users lives easier (no weird namespace collisions).

As I understand the per project limit is 10GB which we'll get to in 5+ years if we never clean up a release.
Cleaning up releases is manual at this point - but there is an issue for PyPi that pushes for automated cleanups (pypi/warehouse#8792).

@quantumlib/cirq-maintainers this is a final chance to object :) Please comment, otherwise I'm ready to merge tomorrow!

@mpharrigan
Copy link
Collaborator

All in all, I think this is a good idea. We did this for a previous open source project I maintained and it worked well iirc

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

Minor comment about release docs, otherwise this LGTM!

release.md Outdated Show resolved Hide resolved
@balopat
Copy link
Contributor Author

balopat commented Dec 3, 2020

As we have quorum, I'm going to go ahead and merge it. Thanks all for chiming in!

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 3, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 3, 2020
@CirqBot CirqBot merged commit 2219ade into quantumlib:master Dec 3, 2020
@CirqBot 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 Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent installing both cirq and cirq-unstable.
5 participants