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

Run test suite during release builds #12202

Closed
mtreinish opened this issue Apr 18, 2024 · 7 comments · Fixed by #13205
Closed

Run test suite during release builds #12202

mtreinish opened this issue Apr 18, 2024 · 7 comments · Fixed by #13205
Assignees
Labels
type: feature request New feature or request type: qa Issues and PRs that relate to testing and code quality

Comments

@mtreinish
Copy link
Member

What should we add?

Currently our release builds are configured to run this small test script: https://github.com/Qiskit/qiskit/blob/main/examples/python/stochastic_swap.py which executes the StochasticSwap transpiler pass. This is an historical artifact from before the use of Rust by qiskit, the stochastic swap pass was our only compiled extension (written in Cython) and the script was exercising solely that to speed up the validation phase of release builds. However in the intervening years more and more of Qiskit is being written in rust (with the 1.1.0 release poised to be >10% written in Rust by lines of code). We should update our wheel build configuration to instead run the unittests to validate that the binaries we publish work on all our supported python versions more broadly than just a single transpiler pass.

This primarily involves updating the cibuildwheel configuration:

qiskit/pyproject.toml

Lines 136 to 163 in 9ede1b6

[tool.cibuildwheel]
manylinux-x86_64-image = "manylinux2014"
manylinux-i686-image = "manylinux2014"
skip = "pp* cp36-* cp37-* *musllinux* *win32 *i686"
test-skip = "*win32 *linux_i686"
test-command = "python {project}/examples/python/stochastic_swap.py"
# We need to use pre-built versions of Numpy and Scipy in the tests; they have a
# tendency to crash if they're installed from source by `pip install`, and since
# Numpy 1.22 there are no i686 wheels, so we force pip to use older ones without
# restricting any dependencies that Numpy and Scipy might have.
before-test = "pip install --only-binary=numpy,scipy numpy scipy"
# Some jobs locally override the before-build and environment configuration if a
# specific job override is needed. For example tier 1 platforms locally override
# the before-build and environment configuration to enable PGO,
# see: .github/workflows/wheels.yml for the jobs where this is done
environment = 'RUSTUP_TOOLCHAIN="stable"'
[tool.cibuildwheel.linux]
before-all = "yum install -y wget && {package}/tools/install_rust.sh"
environment = 'PATH="$PATH:$HOME/.cargo/bin" CARGO_NET_GIT_FETCH_WITH_CLI="true" RUSTUP_TOOLCHAIN="stable"'
repair-wheel-command = "auditwheel repair -w {dest_dir} {wheel} && pipx run abi3audit --strict --report {wheel}"
[tool.cibuildwheel.macos]
environment = "MACOSX_DEPLOYMENT_TARGET=10.12"
repair-wheel-command = "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel} && pipx run abi3audit --strict --report {wheel}"
[tool.cibuildwheel.windows]
repair-wheel-command = "cp {wheel} {dest_dir}/. && pipx run abi3audit --strict --report {wheel}"
to run our unittests instead of the standalone script. We already run the tests once for PGO, but it would be good to validate that the wheels we publish pass the tests as part of the cibuildwheel test phase.

There is an open question, whether we should execute the full test suite or just a subset of the tests. I think for our tier 1 platforms (https://docs.quantum.ibm.com/start/install#operating-system-support) we probably should have enough CI time to run the full test suite. But for our tier 2 platforms this might be tricky because we run those builds under QEMU emulation. So for tier 2 platforms we might want a "smoke test" subset of the test suite that executes quicker because everything is very slow under emulation.

@mtreinish mtreinish added type: feature request New feature or request unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ type: qa Issues and PRs that relate to testing and code quality labels Apr 18, 2024
@1ucian0
Copy link
Member

1ucian0 commented May 29, 2024

This issue is participating on UnitaryHack 2024, between May 29 and June 12, 2024.

Because the nature of the event, there is no need to request to be assigned: just go ahead and PR your fix. The first/best PR can get the bounty (or it could be shared if they complement each other).

@Erik-Chagas
Copy link

Erik-Chagas commented Jun 2, 2024

Hi, I'm working on this issue as part of the unitaryHACK event. I'm unsure whether unittests and smoke tests (described as quicker tests for Tier 2 platforms) already exist in the Qiskit project and so we should just update cibuildwheel so that it runs the necessary unittests. Thank you in advance for your attention.

@mtreinish
Copy link
Member Author

The unit test suite does already exist, that's what we run on every CI test job already. The smoke tests I mentioned don't exist as a category yet. It would be about selecting a limited subset of the larger complete test suite to run. That way we have some test coverage coverage but also a reasonable runtime.

@Erik-Chagas
Copy link

Erik-Chagas commented Jun 6, 2024

Hi, I thought about performing the tests using the 'tox' command for Tier 1 and 'tox -- tests/smoke' for Tier 2 Linux aarch64, changing the pyproject.toml file like this:

[tool.cibuildwheel]
manylinux-x86_64-image = "manylinux2014"
manylinux-i686-image = "manylinux2014"
skip = "pp* cp36-* cp37-* *musllinux* *win32 *i686 cp38-macosx_arm64"
test-skip = "*win32 *linux_i686"
test-command = "tox" # 'tox' command applied for testing
# We need to use pre-built versions of Numpy and Scipy in the tests; they have a
# tend to crash if they're installed from source by `pip install`, and since
# Numpy 1.22 there are no i686 wheels, so we force pip to use older ones without
# restricting any dependencies that Numpy and Scipy might have.
before-test = "pip install --only-binary=numpy,scipy numpy scipy"
# Some jobs locally override the before-build and environment configuration if a
# specific job override is needed. For example tier 1 platforms locally override
# the before-build and environment configuration to enable PGO,
# see: .github/workflows/wheels.yml for the jobs where this is done
environment = 'RUSTUP_TOOLCHAIN="stable"'

# New command session for Linux aarch64
[tool.cibuildwheel.linux.aarch64]
test-command = "tox -- tests/smoke"
before-all = "yum install -y wget && {package}/tools/install_rust.sh"
environment = 'PATH="$PATH:$HOME/.cargo/bin" CARGO_NET_GIT_FETCH_WITH_CLI="true" RUSTUP_TOOLCHAIN="stable"'
repair-wheel-command = "auditwheel repair -w {dest_dir} {wheel} && pipx run abi3audit --strict --report {wheel}"

[tool.cibuildwheel.linux]
before-all = "yum install -y wget && {package}/tools/install_rust.sh"
environment = 'PATH="$PATH:$HOME/.cargo/bin" CARGO_NET_GIT_FETCH_WITH_CLI="true" RUSTUP_TOOLCHAIN="stable"'
repair-wheel-command = "auditwheel repair -w {dest_dir} {wheel} && pipx run abi3audit --strict --report {wheel}"

[tool.cibuildwheel.macos]
environment = "MACOSX_DEPLOYMENT_TARGET=10.12"
repair-wheel-command = "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel} && pipx run abi3audit --strict --report {wheel}"

[tool.cibuildwheel.windows]
repair-wheel-command = "copy {wheel} {dest_dir}/ && pipx run abi3audit --strict --report {wheel}"

@mtreinish
Copy link
Member Author

That looks like the basic idea, the only issue is you can't use tox because that will trigger a build from source, the cibuildwheel flow is that they've built and installed the package at the point tests are run so the test-command runs in that environment with qiskit already installed. It probably would be best to update before-test to also add -r requirements-dev.txt to the pip install command and then call stestr directly in the test-command.

@1ucian0
Copy link
Member

1ucian0 commented Jul 18, 2024

UnitaryHACK 2024 finished. So this issue goes back to standard status.

@1ucian0 1ucian0 removed the unitaryhack Issues/PR participating (now or in the past) in the UnitaryHack event see https://unitaryhack.dev/ label Jul 18, 2024
@Procatv
Copy link
Contributor

Procatv commented Jul 31, 2024

Can I be assigned to this issue, I'd like to take a look at it!

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Sep 20, 2024
This commit updates the cibuildwheel configuration to run the unittests
as the test/validation step after building and auditing the wheels.
Previously we were running a standalone "example" script as the
validation because at one time many years ago StochasticSwap was the
only compiled code in qiskit so this was sufficient coverage. But now
that Qiskit is >16% written in Rust as of this commit this script is not
sufficient to validate the wheels are functional prior to uploading to
pypi. For tier 1 platforms this updates the cibuildwheel configuration
to run the full unit test suite, on tier 2 platforms (which is currently
only linux aarch64) we run just the transpiler integration tests as we
have more time pressure to run the tests on those platforms and don't
have the budget to run the full test suite on all supported python
versions.

Fixes Qiskit#12202
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Sep 21, 2024
This commit updates the cibuildwheel configuration to run the unittests
as the test/validation step after building and auditing the wheels.
Previously we were running a standalone "example" script as the
validation because at one time many years ago StochasticSwap was the
only compiled code in qiskit so this was sufficient coverage. But now
that Qiskit is >16% written in Rust as of this commit this script is not
sufficient to validate the wheels are functional prior to uploading to
pypi. For tier 1 platforms this updates the cibuildwheel configuration
to run the full unit test suite, on tier 2 platforms (which is currently
only linux aarch64) we run just the transpiler integration tests as we
have more time pressure to run the tests on those platforms and don't
have the budget to run the full test suite on all supported python
versions.

Fixes Qiskit#12202
github-merge-queue bot pushed a commit that referenced this issue Sep 24, 2024
This commit updates the cibuildwheel configuration to run the unittests
as the test/validation step after building and auditing the wheels.
Previously we were running a standalone "example" script as the
validation because at one time many years ago StochasticSwap was the
only compiled code in qiskit so this was sufficient coverage. But now
that Qiskit is >16% written in Rust as of this commit this script is not
sufficient to validate the wheels are functional prior to uploading to
pypi. For tier 1 platforms this updates the cibuildwheel configuration
to run the full unit test suite, on tier 2 platforms (which is currently
only linux aarch64) we run just the transpiler integration tests as we
have more time pressure to run the tests on those platforms and don't
have the budget to run the full test suite on all supported python
versions.

Fixes #12202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment