Ensure metapackage is installed during CI and tox #11119
Conversation
jakelishman
added
type: qa
|
One or more of the the following people are requested to review this:
|
jakelishman
force-pushed
the
fix-ci-aer-requirements
branch
from
c505f25 to
57abe30
Compare
This ensures that the local version of the metapackage is also built and installed on all CI runs (and in `tox`, where it's overridden) so that dependencies on the metapackage in our optionals (e.g. Aer) will not cause the older released version of Terra to be installed. `tox` does not like having two local packages under test simultaneously through its default configuration, so this fakes things out by putting the two packages in the run dependencies and setting `skip_install`.
jakelishman
force-pushed
the
fix-ci-aer-requirements
branch
from
57abe30 to
592c8bc
Compare
|
I think I got all the relevant locations with this PR, but it's a bit hard to be sure. |
|
This PR is now a necessary step on the way to fixing #11240. I can't release a new version of The order needs to be:
|
| # We pretend that we're not actually installing the package, because we need tox to let us have two | ||
| # packages ('qiskit' and 'qiskit-terra') under test at the same time. For that, we have to stuff | ||
| # them into 'deps'. | ||
| skip_install = true |
There was a problem hiding this comment.
I"m a bit concerned about this, I seem to remember skip_install causing weird behavior in tox >4.0.0, I think with regards to tracking source updates or something, I don't fully remember but I know it caused issues for me on the upgrade to 4.0.0.
There was a problem hiding this comment.
I'm open to other ideas here, this is just the only thing I could think of to trick tox into letting us put two local packages under test at the same time. I'm moving ever further into the "let's ditch tox" camp.
jakelishman
force-pushed
the
fix-ci-aer-requirements
branch
from
c322d16 to
1dae6a2
Compare
Pull Request Test Coverage Report for Build 6877155276
💛 - Coveralls |
There was a problem hiding this comment.
This LGTM, I'm a bit cautious about the tox config but I tested it locally and it seems to work fine. There will probably be some complexity around builds not getting re-triggered correctly because tox isn't tracking things the same way having qiskit in the requirements list, but I'm not sure that mechanism is working correctly without this change either though because of the rust/python split. I think we definitely should look at moving away from tox in parallel to this though. Something like nox will give us more flexibility.
* Ensure metapackage is installed during CI and tox This ensures that the local version of the metapackage is also built and installed on all CI runs (and in `tox`, where it's overridden) so that dependencies on the metapackage in our optionals (e.g. Aer) will not cause the older released version of Terra to be installed. `tox` does not like having two local packages under test simultaneously through its default configuration, so this fakes things out by putting the two packages in the run dependencies and setting `skip_install`. * Fix sdist build * Use regular installs for metapackage * Simplify build requirements install (cherry picked from commit f7f6cb1)
* Ensure metapackage is installed during CI and tox This ensures that the local version of the metapackage is also built and installed on all CI runs (and in `tox`, where it's overridden) so that dependencies on the metapackage in our optionals (e.g. Aer) will not cause the older released version of Terra to be installed. `tox` does not like having two local packages under test simultaneously through its default configuration, so this fakes things out by putting the two packages in the run dependencies and setting `skip_install`. * Fix sdist build * Use regular installs for metapackage * Simplify build requirements install (cherry picked from commit f7f6cb1) Co-authored-by: Jake Lishman <[email protected]>
In Qiskit#11119 we removed the use_develop flag in the tox config which was used to tell tox to use a single editable install for running a job (avoiding the need to build from sdist on every run) as part of a temporary refactoring to ensure we installed the metapackage along with terra. Now that we've standardized the packaging in Qiskit#11271 that was undone, but the usedevelop flag was not added back. This has caused tox to rebuild the package from sdist on every run even if there are no code changes. This commit restores the missing option to fix this.
Summary
This ensures that the local version of the metapackage is also built and installed on all CI runs (and in
tox, where it's overridden) so that dependencies on the metapackage in our optionals (e.g. Aer) will not cause the older released version of Terra to be installed.toxdoes not like having two local packages under test simultaneously through its default configuration, so this fakes things out by putting the two packages in the run dependencies and settingskip_install.Details and comments
This should (hopefully) get CI moving again. The necessary changes to
toxare indicative of larger developer (but not user) pain, though - installing any downstream Qiskit project on top of an editable install of Terra is liable to reinstall the current release version of Terra via a pin from theqiskitmetapackage. To work around this, developers should ensure that they have donepip install ./qiskit_pkgin their dev env, and they may need to rerun that when Terra bumps its version number (editable mode won't affect pip's version resolution).