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.

Sign up for GitHub

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

Fix asv installation configuration #11222

Merged
merged 1 commit into from
Nov 9, 2023
+5 −6
Merged

Fix asv installation configuration #11222

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix asv installation configuration 
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
jakelishman committed Nov 9, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
commit 995d3b73ad5729edb6e1047484b9039e921502f3
11 changes: 5 additions & 6 deletions asv.conf.json
View file
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

],
"uninstall_command": [
"return-code=any python -mpip uninstall -y {project}"
"return-code=any python -m pip uninstall -y qiskit qiskit-terra"
],
"build_command": [
"pip install -U setuptools-rust",
"python setup.py build_rust --release",
"PIP_NO_BUILD_ISOLATION=false python -mpip wheel --no-deps --no-index -w {build_cache_dir} {build_dir}"
"python -m pip install -U build",
"python -m build --outdir {build_cache_dir} --wheel {build_dir}"
],
"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/",
Copy link
Member

Choose a reason for hiding this comment

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

This technically still worked because of the redirects :) but it's better to do this I agree

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"pythons": ["3.8", "3.9", "3.10", "3.11"],
"benchmark_dir": "test/benchmarks",
"env_dir": ".asv/env",