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

Move metapackage shim for combined releases #10530

Merged
merged 11 commits into from
Aug 15, 2023

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Jul 28, 2023

Summary

Now that Qiskit 0.44.0 has been released, the Qiskit project is now what was formerly qiskit-terra. However, because Python packaging lacks a clean mechanism to enable one package superseding another we're not able to stop shipping a qiskit-terra package that owns the qiskit python namespace without introducing a lot of potential user friction. So moving forward the qiskit project will release 2 packages an inner qiskit-terra which still contains all the library code and public facing qiskit package which installs that inner package only. To enable this workflow this commit migrates the metapackage setup.py into the terra repository and setups build automation to publish a qiskit package in addition to the inner terra package on each release tag.

Details and comments

Close Qiskit/qiskit-metapackage#1792.

Now that Qiskit 0.44.0 has been released, the Qiskit project is now what
was formerly qiskit-terra. However, because Python packaging lacks a
clean mechanism to enable one package superseding another we're not able
to stop shipping a qiskit-terra package that owns the qiskit python
namespace without introducing a lot of potential user friction. So
moving forward the qiskit project will release 2 packages an inner
qiskit-terra which still contains all the library code and public facing
qiskit package which installs that inner package  only. To enable this
workflow this commit migrates the metapackage setup.py into the terra
repository and setups build automation to publish a qiskit package in
addition to the inner terra package on each release tag.
@mtreinish mtreinish added the Changelog: None Do not include in changelog label Jul 28, 2023
@mtreinish mtreinish added this to the 0.45.0 milestone Jul 28, 2023
@mtreinish mtreinish requested a review from a team as a code owner July 28, 2023 19:36
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jul 28, 2023

Pull Request Test Coverage Report for Build 5824410178

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 87.263%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 91.14%
qiskit/pulse/library/waveform.py 3 93.75%
Totals Coverage Status
Change from base Build 5822048885: 0.01%
Covered Lines: 74292
Relevant Lines: 85136

💛 - Coveralls

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

This LGTM overall. Makes sense to remove the toqm extra since we don't have a code dependency on it anymore thanks to the staged pass manager plugin interface 😄.

README.md Outdated
This library is the core component of Qiskit, **Terra**, which contains the building blocks for creating
and working with quantum circuits, programs, and algorithms. It also contains a compiler that supports
different quantum computers and a common interface for running programs on different quantum computer architectures.
This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. It also
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. It also
Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. It also

qiskit_pkg/setup.py Outdated Show resolved Hide resolved
name="qiskit",
version="0.45.0",
description="Software for developing quantum computing programs",
long_description=README,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will be the string "../README.md", right? Does setup take either a file path or the contents directly and decide what to do from there? Maybe I'm missing something 😄

Copy link
Member

Choose a reason for hiding this comment

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

Those files are actually symlinks in the repo, that's just how GitHub displays them. No idea how Windows will handle them, tbh.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Did you consider maybe switching the way we publish the packages, so qiskit-terra becomes the dummy package with a simple dependency on qiskit, and what's currently Terra becomes qiskit? I wrote a little more in a comment below, but I think it works right. It feels like the kind of thing where I could easily have not thought of something, though.

name="qiskit",
version="0.45.0",
description="Software for developing quantum computing programs",
long_description=README,
Copy link
Member

Choose a reason for hiding this comment

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

Those files are actually symlinks in the repo, that's just how GitHub displays them. No idea how Windows will handle them, tbh.

qiskit_pkg/setup.py Outdated Show resolved Hide resolved
Comment on lines +35 to +38
requirements = ["qiskit-terra==0.45.0"]

setup(
name="qiskit",
Copy link
Member

@jakelishman jakelishman Jul 28, 2023

Choose a reason for hiding this comment

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

Would it be possible / better to publish our wheels as qiskit, flip the dependency order, and have qiskit-terra become the thin dependency-only package? In other words, swap the name of what's currently qiskit-terra to become qiskit, and have this "new" package be qiskit-terra which just contains the dependency on qiskit?

As far as I could think, it'll satisfy all the upgrade paths correctly, and it also has the pleasant properties:

  • the preferred install, pip install qiskit, installs directly (and the name Terra doesn't appear any more)
  • our preferred install path links directly to the wheels, so going to the PyPI page is less surprising
  • similarly, the PyPI page for qiskit-terra will much more clearly show that it's been superseded (and we could even change the README there to simply say that, rather than the symlink trick we're using here because we're still treating qiskit-terra as the main package)
  • we can drop qiskit-terra in the future, say at 1.0 or 2.0, and pip install qiskit will still work correctly
  • we effectively gain another ~10GB storage on PyPI, since we've barely used any of our space on the metapackage
  • it matches the directory structure of this repo more naturally

Copy link
Member

Choose a reason for hiding this comment

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

It's the lack of uninstall of the old qiskit-terra package that's potentially the problem, right?

I wonder if we might actually just be better doing it anyway and saying "you might want to manually uninstall qiskit-terra when upgrading"?

Copy link
Member Author

@mtreinish mtreinish Jul 31, 2023

Choose a reason for hiding this comment

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

Yeah I considered it, but the upgrade path is particularly fraught here which is why I pushed forward in this direction. It's actually not just the lack of an uninstall but also the potential uninstall. The obvious issues is a user who does pip install -U qiskit from 0.44.0 -> 0.45.0 in this case if we have a reverse dependency then the lack of an uninstall for qiskit-terra is a potential problem because pip will just dump the qiskit package's contents into site-libs and potentially leaving mismatched versions. But the other side is if you do an uninstall of qiskit-terra <= 0.25.0 (which can happen automatically as part of qiskit-terra package upgrade) after moving code to the qiskit package you'll potentially have a big chunk of the terra code deleted. There are also a lot of other edge cases that can come up to depending on the install method (as there are multiple install mechanisms) which are similarly fraught. Also not to mention the mix of editable installs with regular installs between the packages (ie doing an editable install of qiskit-terra==0.25.0 and a regular install of qiskit==0.45.0) which is likely an issue independently.

The reason I went with this path is it's the only way to maintain an easy upgrade path that doesn't require any manual updates. The only way to ensure users have a good upgrade path if we wanted to reverse the dependency is to tell them to manually uninstall qiskit-terra in all cases before updating. I'm opposed to reversing the dependency for 2 reasons, first because for the user of qiskit they never manually installed qiskit-terra and may not realize it's installed and haven't been managing it at all. The second is we actually did this in the past for qiskit 0.7.0 the code moved from the qiskit package to the qiskit-terra package and there were a lot of user issues around the migration. There was actually a workaround at the time to have the qiskit's setup.py fail wheel builds to avoid an automatic uninstall of qiskit after having upgraded qiskit-terra (which is the reason we don't published wheels for the metapackage, although this PR fixes that oversight). But, having had to deal with the headaches around doing this exact thing in the past I don't think it's something we want to subject users to again, when if we keep the dependency relationship the same as today everything works, it's just a bit more awkward looking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really wish setuptools had a supersedes option that let us say qiskit supersedes the qiskit-terra package so it would correctly handle the upgrade path. I guess it's enough of an edge case that it doesn't come up very often, but for real package managers they typically offer this in the metadata because it does happen.

Copy link
Member

Choose a reason for hiding this comment

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

That's all fair. I do think it's something we should consider for Qiskit 1.0, though - I think we have a chance there to say "to upgrade an existing environment to Qiskit 1.0, you first must pip uninstall qiskit qiskit-terra qiskit-aer qiskit-ibmq-provider" and get away with it more from a user perspective. I also think it'll generally be a bit easier than last time, because we're not fighting the namespace packaging any more. It's pain now, but it frees us a lot more for the future, and the final packaging story which be much more straightforward.

I'm not super worried about editable installs breaking, because that's just dev workflows, and they've already been broken for certain configurations for several months now as we removed the namespace packaging (having a regular package installed that used the namespace packaging broke the editable install).

Also yeah, it would be good if there was a proper way to have one package supersede another.

Copy link
Contributor

@wshanks wshanks Aug 3, 2023

Choose a reason for hiding this comment

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

Here are my thoughts (as someone who will not hear anywhere near as many user complaints as either of you 🙂):

I don't love the dependency reversal because of the scenario @mtreinish mentioned of pip install -U qiskit overwriting an old qiskit-terra's contents without removing it and also because it will break all the downstream projects that depend now explicitly on qiskit-terra without an upper version bound (got confused about this and forgot about in this scenario the dependency was reversed so this wouldn't happen). What about this progression:

  1. Add a check to qiskit/__init__.py that warns if qiskit is not installed but leave all the code in qiskit-terra and keep qiskit as a package depending only on a specific version of qiskit-terra.
  2. After enough time for the community to have switched over to depending on and install qiskit rather than qiskit-terra (a year?), make a release with the code moved into the qiskit package with qiskit-terra still as a dependency but now empty.
  3. After enough time again (another year?) that most users should have a release with the code in the qiskit package, drop the terra dependency.

The goal is to make sure that everyone is using qiskit before shifting the code over, so that the upgrade to an empty terra does not leave someone with no files. It requires a lot of patience though.

Personally, I think it would be better to get rid of terra if not too painful because it will be a point of minor confusion, mostly as a distraction but also some potential confusion since pip does allow some ways of installing packages without dependencies which could lead a user to end up with qiskit and qiskit-terra with different version numbers.

Copy link
Member

Choose a reason for hiding this comment

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

Will: I pretty much agree with everything you said, the reason I was pushing for a more compressed timeline was that I think we'll get rather more acceptance from users between the 0.x -> 1.0 transition that they might need to pip uninstall x y z; pip install x to upgrade than even the 1.0 -> 2.0 transition, even if there are warnings. I'm also in favour of getting rid of qiskit-terra, because I think it'll hopefully be a minor confusion in most cases, but in cases where it does matter, we're going to have to talk users through more pain that it'll cause here, and it'll be a permanent albatross for us if we don't cut it when we deem we "have the chance".

That said, for this immediate PR, we're going to punt the decision a little bit, since the current form is what the metapackage currently does, and we wouldn't do this until at least the 1.0 release, if we do it at all.

tox.ini Outdated Show resolved Hide resolved
1ucian0 and others added 4 commits August 9, 2023 06:33
* some follow up on Qiskit#10530

  * extend some badges
  * This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. -> This framework allows for building, transforming, and visualizing quantum circuits.
  * The explanation of the Bell state is moving to IBM Quantun learning platform
  * I think examples/ should eventually be replaced by proper tutorials
  * Add StackOverflow as a forum
  * Remove link to https://github.com/Qiskit/qiskit-tutorials

* Update README.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update README.md

* Update README.md

* broken lines in badges

* doi

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Some minor tweaks that I can spot right now, though I imagine we'll probably need to bugfix live during the first attempt at this deployment, because it's always hard to spot everything.

What's the plan between this PR and #10584, since we need this PR more urgently but there's a major logical conflict between the two?

qiskit_pkg/MANIFEST.in Outdated Show resolved Hide resolved
qiskit_pkg/setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
qiskit_pkg/setup.py Outdated Show resolved Hide resolved
Comment on lines +1 to +8
---
upgrade:
- |
The ``toqm`` optional setuptools extra that previously enabled running
``pip install qiskit-terra[topm]`` has been removed as nothing in the
Qiskit code base is currently using that package anymore. If you'd like
to use the qiskit TOQM transpiler plugin you should install the
``qiskit-toqm`` package directly.
Copy link
Member

Choose a reason for hiding this comment

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

the bip-mapper extra is also gone - was that intentional, and does it need a reno if so?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did the release note in: #10526 which is the larger removal of the pass.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we'll need to remember to re-add it when we backport this to do the 0.25.1 deployment.

.azure/lint-linux.yml Outdated Show resolved Hide resolved
jakelishman
jakelishman previously approved these changes Aug 10, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

A minor thing about a dodgy comment, but other than that and the query about the ordering of this PR and the other README one, I'm happy with this.

qiskit_pkg/setup.py Outdated Show resolved Hide resolved
@mtreinish mtreinish modified the milestones: 0.45.0, 0.25.1 Aug 15, 2023
@mtreinish mtreinish added this pull request to the merge queue Aug 15, 2023
Merged via the queue into Qiskit:main with commit 8a180ee Aug 15, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Aug 15, 2023
* Move metapackage shim for combined releases

Now that Qiskit 0.44.0 has been released, the Qiskit project is now what
was formerly qiskit-terra. However, because Python packaging lacks a
clean mechanism to enable one package superseding another we're not able
to stop shipping a qiskit-terra package that owns the qiskit python
namespace without introducing a lot of potential user friction. So
moving forward the qiskit project will release 2 packages an inner
qiskit-terra which still contains all the library code and public facing
qiskit package which installs that inner package  only. To enable this
workflow this commit migrates the metapackage setup.py into the terra
repository and setups build automation to publish a qiskit package in
addition to the inner terra package on each release tag.

* some follow up on #10530 (#19)

* some follow up on #10530

  * extend some badges
  * This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. -> This framework allows for building, transforming, and visualizing quantum circuits.
  * The explanation of the Bell state is moving to IBM Quantun learning platform
  * I think examples/ should eventually be replaced by proper tutorials
  * Add StackOverflow as a forum
  * Remove link to https://github.com/Qiskit/qiskit-tutorials

* Update README.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update README.md

* Update README.md

* broken lines in badges

* doi

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Expand lint checks to entire qiskit_pkg dir

* Unify extras in terra setup.py

* Update CI lint job

* Remove unused json imports

* Cleanup manifest file

* Finish comment

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
(cherry picked from commit 8a180ee)

# Conflicts:
#	README.md
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2023
* Move metapackage shim for combined releases (#10530)

* Move metapackage shim for combined releases

Now that Qiskit 0.44.0 has been released, the Qiskit project is now what
was formerly qiskit-terra. However, because Python packaging lacks a
clean mechanism to enable one package superseding another we're not able
to stop shipping a qiskit-terra package that owns the qiskit python
namespace without introducing a lot of potential user friction. So
moving forward the qiskit project will release 2 packages an inner
qiskit-terra which still contains all the library code and public facing
qiskit package which installs that inner package  only. To enable this
workflow this commit migrates the metapackage setup.py into the terra
repository and setups build automation to publish a qiskit package in
addition to the inner terra package on each release tag.

* some follow up on #10530 (#19)

* some follow up on #10530

  * extend some badges
  * This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. -> This framework allows for building, transforming, and visualizing quantum circuits.
  * The explanation of the Bell state is moving to IBM Quantun learning platform
  * I think examples/ should eventually be replaced by proper tutorials
  * Add StackOverflow as a forum
  * Remove link to https://github.com/Qiskit/qiskit-tutorials

* Update README.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update README.md

* Update README.md

* broken lines in badges

* doi

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Expand lint checks to entire qiskit_pkg dir

* Unify extras in terra setup.py

* Update CI lint job

* Remove unused json imports

* Cleanup manifest file

* Finish comment

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
(cherry picked from commit 8a180ee)

# Conflicts:
#	README.md

* Fix merge conflicts

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish deleted the setup-metapackage-publish branch August 15, 2023 20:00
SamD-1998 pushed a commit to SamD-1998/qiskit-terra that referenced this pull request Sep 7, 2023
* Move metapackage shim for combined releases

Now that Qiskit 0.44.0 has been released, the Qiskit project is now what
was formerly qiskit-terra. However, because Python packaging lacks a
clean mechanism to enable one package superseding another we're not able
to stop shipping a qiskit-terra package that owns the qiskit python
namespace without introducing a lot of potential user friction. So
moving forward the qiskit project will release 2 packages an inner
qiskit-terra which still contains all the library code and public facing
qiskit package which installs that inner package  only. To enable this
workflow this commit migrates the metapackage setup.py into the terra
repository and setups build automation to publish a qiskit package in
addition to the inner terra package on each release tag.

* some follow up on Qiskit#10530 (Qiskit#19)

* some follow up on Qiskit#10530

  * extend some badges
  * This Qiskit contains the building blocks for creating and working with quantum circuits, programs, and algorithms. -> This framework allows for building, transforming, and visualizing quantum circuits.
  * The explanation of the Bell state is moving to IBM Quantun learning platform
  * I think examples/ should eventually be replaced by proper tutorials
  * Add StackOverflow as a forum
  * Remove link to https://github.com/Qiskit/qiskit-tutorials

* Update README.md

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update README.md

* Update README.md

* broken lines in badges

* doi

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Expand lint checks to entire qiskit_pkg dir

* Unify extras in terra setup.py

* Update CI lint job

* Remove unused json imports

* Cleanup manifest file

* Finish comment

---------

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deployment of metapackage shim to Terra
7 participants