Skip to content

Fix asv installation configuration #11222

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

Merged
mtreinish merged 1 commit into from
Nov 9, 2023
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
11 changes: 5 additions & 6 deletions asv.conf.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy Markdown
Member

@mtreinish mtreinish Nov 9, 2023

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
Copy Markdown
Member Author

@jakelishman jakelishman Nov 9, 2023

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
Copy Markdown
Member

@mtreinish mtreinish Nov 9, 2023

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": "https://github.com/Qiskit/qiskit-terra/commit/",
"show_commit_url": "https://github.com/Qiskit/qiskit/commit/",
Copy link
Copy Markdown
Member

@mtreinish mtreinish Nov 9, 2023

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
Copy Markdown
Member Author

@jakelishman jakelishman Nov 9, 2023

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",
Expand Down