Fix asv installation configuration #11222
Conversation
Previously, the `uninstall` component of the asv configuration was attempting to uninstall the package `qiskit`. This _is_ the name of the project, but it's not the name of the relevant Python package, and so was not being uninstalled correctly. This meant that asv's managed environments would retain the previous build, unless the updating the commit changed the version number of the package. This behaviour caused our tracking bot to be benchmarking the state of the package only at the commit points that bump version numbers on `main`. Typically, this means it was only ever benchmarking the release-candidate versions of the package since we migrated from the metapackage.
jakelishman
added
type: qa
|
One or more of the the following people are requested to review this:
|
| ], | ||
| "branches": ["main"], | ||
| "dvcs": "git", | ||
| "environment_type": "virtualenv", | ||
| "show_commit_url": "http://github.com/Qiskit/qiskit-terra/commit/", | ||
| "show_commit_url": "http://github.com/Qiskit/qiskit/commit/", |
There was a problem hiding this comment.
This technically still worked because of the redirects :) but it's better to do this I agree
There was a problem hiding this comment.
Yeah for sure, and at the time that the asv benchmarks were merged in, the repo hadn't yet been renamed so it wasn't ready to be replaced then.
| @@ -4,20 +4,19 @@ | |||
| "project_url": "https://qiskit.org", | |||
| "repo": ".", | |||
| "install_command": [ | |||
| "in-dir={env_dir} python -mpip install {wheel_file}[all] python-constraint qiskit-experiments==0.3.0" | |||
| "in-dir={env_dir} python -m pip install {wheel_file}[all] python-constraint qiskit-experiments==0.3.0" | |||
There was a problem hiding this comment.
Do you think it makes sense to have a matrix here for the experiments version and python-constraint: https://asv.readthedocs.io/en/stable/asv.conf.json.html#matrix? This pattern was used in the original asv benchmarks that I added back in 2019 but back then asv didn't have support for reasoning about third-part dependencies explicitly like this, so having a pinned install in the install command was the only pattern to enable this.
There was a problem hiding this comment.
I didn't know about that, but it looks good. It gets potentially hairy with qiskit-experiments, because that has a transitive dependency on Terra, and matrix requirements apparently get installed before the main project. The current form forces version resolution to happen all at once, which is a bit more stable, I suppose.
There was a problem hiding this comment.
That makes sense, let's just leave this for now. I don't think we even need qiskit-experiments here anymore. I'll push up a separate PR to remove it.
Pull Request Test Coverage Report for Build 6813062893
💛 - Coveralls |
Summary
Previously, the
uninstallcomponent of the asv configuration was attempting to uninstall the packageqiskit. This is the name of the project, but it's not the name of the relevant Python package, and so was not being uninstalled correctly. This meant that asv's managed environments would retain the previous build, unless the updating the commit changed the version number of the package.This behaviour caused our tracking bot to be benchmarking the state of the package only at the commit points that bump version numbers on
main. Typically, this means it was only ever benchmarking the release-candidate versions of the package since we migrated from the metapackage.Details and comments
I was able to use this locally to correctly detect changes in the
time_circuit_copybenchmark between #10314 and #11040, with the uninstall working correctly: