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 missing use_develop to tox.ini #11380

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

mtreinish
Copy link
Member

Summary

In #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 #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.

Details and comments

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.
@mtreinish mtreinish added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Dec 7, 2023
@mtreinish mtreinish requested a review from a team as a code owner December 7, 2023 16:45
@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 Dec 7, 2023

Pull Request Test Coverage Report for Build 9172475719

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 13 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.601%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/parse.rs 6 97.15%
crates/qasm2/src/lex.rs 7 92.37%
Totals Coverage Status
Change from base Build 9163549280: -0.01%
Covered Lines: 62294
Relevant Lines: 69524

💛 - Coveralls

@jakelishman
Copy link
Member

tbf, I think this is tox doing what it's intended to do. If you want to skip the reinstall step with tox 4, the command is

tox run --skip-pkg-install [-e py310]

That said, I don't really use the testenv environments from tox, so I'm also fine if you'd rather switch it back to using the editable installs.

@wshanks
Copy link
Contributor

wshanks commented May 21, 2024

A problem developers run into is that that without use_develop the qiskit in the repo takes precedence over the version installed by tox and then fails to import if the Rust components have not been compiled in the repo. Somehow that should be fixed.

One option is to set use_develop. Another would be to add PYTHONSAFEPATH=1 to tox's setenv. The first option would be faster since it would not need to rebuild the Rust libraries on every run. The second one would be more correct since it would be testing installation from a wheel and could catch packaging issues. There could be some middle ground between the two by setting tox up to build the package in the repo or other fixed directory instead of in a temporary directory so that it can reuse the Rust libraries when building the wheel.

@mtreinish
Copy link
Member Author

The reason I didn't go for PYTHONSAFEPATH is that it's not supported on older versions of Python, it only works with Python >=3.11 but we still support older python versions. The way I solved this in rustworkx was to set changedir = {toxinidir}/tests so that all the tox commands run out of tests/ instead of the repo root which would also work. I don't have a strong opinion, but this issue has been a pain for too long on local development so picking a solution and implementing it would be good.

@wshanks
Copy link
Contributor

wshanks commented May 21, 2024

Ah, okay. I also don't have a preference but agree something should be done. I use my own environment for the most part and only use tox if something I run seems odd and I want retry it in a clean environment.

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 type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants