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

Implement multithreaded stochastic swap in rust #7658

Merged
merged 42 commits into from
Feb 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
9d9c2dc
Implement multithreaded stochastic swap in rust
mtreinish Feb 11, 2022
7af1c43
Sanitize packaging to support future modules
mtreinish Feb 16, 2022
2f8de01
Adjust random normal distribution to use correct mean
mtreinish Feb 16, 2022
6da1b7b
Remove unecessary extra scope from locked read
mtreinish Feb 16, 2022
3cf6e04
Remove unecessary explicit type from opt_edges variable
mtreinish Feb 16, 2022
f838ff6
Fix indices typo in NLayout constructor
mtreinish Feb 16, 2022
780f327
Remove explicit lifetime annotation from swap_trials
mtreinish Feb 16, 2022
35fba96
Use sum() instead of fold()
mtreinish Feb 16, 2022
d72d6c5
Fix lint and add rust style and lint checks to CI
mtreinish Feb 16, 2022
0b6e4d7
Fix returned layout mapping from NLayout
mtreinish Feb 17, 2022
a9a879d
Tweak tox configuration to try and reliably build rust extension
mtreinish Feb 17, 2022
57790c8
Make swap_trials parallelization configurable
mtreinish Feb 17, 2022
ea23eae
Merge remote-tracking branch 'origin/main' into parallel-stochastic-s…
mtreinish Feb 17, 2022
18a8b56
Revert "Make swap_trials parallelization configurable"
mtreinish Feb 17, 2022
6a63d11
Add docs to swap_trials() and remove unecessary num_gates arg
mtreinish Feb 17, 2022
873de02
Fix race condition leading to non-deterministic behavior
mtreinish Feb 18, 2022
2d9aae0
Merge branch 'main' into parallel-stochastic-swap-in-rust
mtreinish Feb 18, 2022
76167c7
Apply suggestions from code review
mtreinish Feb 18, 2022
2685c01
Fix compiler errors in previous commit
mtreinish Feb 18, 2022
610b306
Revert accidental commit of parallel reduction in compute_cost
mtreinish Feb 18, 2022
c510764
Eliminate short circuit for depth == 1 swap_trial() result
mtreinish Feb 18, 2022
b7607c1
Add missing docstrings
mtreinish Feb 18, 2022
5e31fb2
Add section to contributing on installing form source
mtreinish Feb 18, 2022
cfc8fcb
Make rust python classes pickleable
mtreinish Feb 18, 2022
d726101
Add rust compiler install to linux wheel jobs
mtreinish Feb 18, 2022
fb70158
Try more tox changes to fix docs builds
mtreinish Feb 18, 2022
25187b8
Revert "Eliminate short circuit for depth == 1 swap_trial() result"
mtreinish Feb 18, 2022
b2a0536
Fix submodule declaration and module attribute on rust classes
mtreinish Feb 18, 2022
f2737aa
Fix rust lint
mtreinish Feb 18, 2022
2588837
Fix docs job definition
mtreinish Feb 18, 2022
93ded7f
Disable multiprocessing parallelism in unit tests
mtreinish Feb 19, 2022
e7a6b93
Fix typo in azure pipelines config
mtreinish Feb 20, 2022
91f0da9
Remove unecessary extension compilation for image tests
mtreinish Feb 20, 2022
6da2a3a
Add test script to explicitly verify parallel dispatch
mtreinish Feb 20, 2022
3d1d561
Avoid multi-threading when run in a multiprocessing context
mtreinish Feb 22, 2022
25b2747
Merge branch 'main' into parallel-stochastic-swap-in-rust
mtreinish Feb 22, 2022
fc109d2
Apply suggestions from code review
mtreinish Feb 23, 2022
eaf92f2
Minor fixes from review comments
mtreinish Feb 23, 2022
b318a2a
Merge remote-tracking branch 'origin/main' into parallel-stochastic-s…
mtreinish Feb 23, 2022
4c4c9d4
Simplify tox configuration
mtreinish Feb 23, 2022
73296a4
Add missing pieces of cargo configuration
mtreinish Feb 23, 2022
eb69288
Merge branch 'main' into parallel-stochastic-swap-in-rust
mergify[bot] Feb 28, 2022
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: 11 additions & 0 deletions .cargo/config
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[target.x86_64-apple-darwin]
rustflags = [
"-C", "link-arg=-undefined",
"-C", "link-arg=dynamic_lookup",
]

[target.aarch64-apple-darwin]
rustflags = [
"-C", "link-arg=-undefined",
"-C", "link-arg=dynamic_lookup",
]
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,8 @@ test/ipynb/mpl/circuit/result_test.json
test/ipynb/mpl/graph/*.png
test/ipynb/mpl/graph/*.zip
test/ipynb/mpl/graph/result_test.json

# Added by cargo

/target
Cargo.lock
2 changes: 1 addition & 1 deletion .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ unsafe-load-any-extension=no
# A comma-separated list of package or module names from where C extensions may
# be loaded. Extensions are loading into the active Python interpreter and may
# run arbitrary code
extension-pkg-allow-list=retworkx, numpy, tweedledum
extension-pkg-allow-list=retworkx, numpy, tweedledum, qiskit._accelerate


[MESSAGES CONTROL]
Expand Down
29 changes: 26 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,32 @@ build all the documentation into `docs/_build/html` and the release notes in
particular will be located at `docs/_build/html/release_notes.html`

## Installing Qiskit Terra from source
Please see the [Installing Qiskit Terra from
Source](https://qiskit.org/documentation/contributing_to_qiskit.html#installing-terra-from-source)
section of the Qiskit documentation.

Qiskit Terra is primarily written in Python but there are some core routines
that are written in the [Rust](https://www.rust-lang.org/) programming
language to improve the runtime performance. For the released versions of
qiskit-terra we publish precompiled binaries on the
[Python Package Index](https://pypi.org/) for all the supported platforms
which only requires a functional Python environment to install. However, when
building and installing from source you will need a rust compiler installed. You can do this very easily
using rustup: https://rustup.rs/ which provides a single tool to install and
configure the latest version of the rust compiler.
[Other installation methods](https://forge.rust-lang.org/infra/other-installation-methods.html)
exist too. For windows users besides rustup you will also need install
the Visual C++ build tools so that rust can link against the system c/c++
libraries. You can see more details on this in the
[rustup documentation](https://rust-lang.github.io/rustup/installation/windows.html).

Once you have a rust compiler installed you can rely on the normal Python
build/install steps to install Qiskit Terra. This means you just run
`pip install .` in your local git clone to build and install Qiskit Terra.

Do note that if you do use develop mode/editable install (via `python setup.py develop` or `pip install -e .`) the Rust extension will be built in debug mode
without any optimizations enabled. This will result in poor runtime performance.
If you'd like to use an editable install with an optimized binary you can
run `python setup.py build_rust --release --inplace` after you install in
editable mode to recompile the rust extensions in release mode.


## Test

Expand Down
33 changes: 33 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
[package]
name = "qiskit-terra"
version = "0.20.0"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[lib]
name = "qiskit_accelerate"
crate-type = ["cdylib"]

[dependencies]
rayon = "1.5"
numpy = "0.15.1"
rand = "0.8"
rand_pcg = "0.3"
rand_distr = "0.4.3"

[dependencies.pyo3]
version = "0.15.1"
features = ["extension-module", "hashbrown"]

[dependencies.ndarray]
version = "^0.15.0"
features = ["rayon"]

[dependencies.hashbrown]
version = "0.11.2"
features = ["rayon"]

[profile.release]
lto = 'fat'
codegen-units = 1
3 changes: 3 additions & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ include test/python/pickles/*.pickle
include test/python/qasm/*.qasm
include test/python/visualization/references/*.png
include test/python/notebooks/*.ipynb

include Cargo.toml
recursive-include src *
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 16 additions & 2 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ stages:
pip install -U "cplex" "qiskit-aer" "z3-solver" -c constraints.txt
mkdir -p /tmp/terra-tests
cp -r test /tmp/terra-tests/.
cp tools/verify_parallel_map.py /tmp/terra-tests/.
cp .stestr.conf /tmp/terra-tests/.
cp -r .stestr /tmp/terra-tests/. || :
sudo apt-get update
Expand All @@ -193,8 +194,11 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 4294967295))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./verify_parallel_map.py
popd
displayName: 'Run tests'
env:
QISKIT_PARALLEL: FALSE
Comment on lines +200 to +201
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a potentially large change to CI - is it completely necessary? If it's only necessary for the verify_parallel_map script, perhaps we could just run that in a separate CI step?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was necessary when I wrote this change, see the commit message on: 93ded7f for the details. But, I updated the logic here in 3d1d561 to not run multithreaded code when we're in parallel_map() so I think we might be able to get away without adding this. That being said doing this made CI noticeably faster because we're not running 2x processes everywhere anymore.

Copy link
Member

Choose a reason for hiding this comment

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

This is good enough for me - my only minor concern is that we might fail to test some code that should run in parallel now, but I think we should be able to override that anyway if necessary within the test suite.

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, we can just copy the model I used in verify_parallel_map to override the env variable, or alternatively we can always explicitly add on to that script too for things we want to ensure are run in parallel.

- task: CopyFiles@2
condition: failed()
displayName: 'Copy images'
Expand Down Expand Up @@ -239,7 +243,6 @@ stages:
virtualenv image_tests
image_tests/bin/pip install -U -r requirements.txt -c constraints.txt
image_tests/bin/pip install -U -c constraints.txt -e ".[visualization]"
image_tests/bin/python setup.py build_ext --inplace
sudo apt-get update
sudo apt-get install -y graphviz pandoc
image_tests/bin/pip check
Expand Down Expand Up @@ -286,6 +289,8 @@ stages:
tools/verify_headers.py qiskit test
python tools/find_optional_imports.py
reno lint
cargo fmt --check
cargo clippy -- -D warnings
displayName: 'Style and lint'
- job: 'Docs'
pool: {vmImage: 'ubuntu-latest'}
Expand Down Expand Up @@ -314,7 +319,6 @@ stages:
set -e
python -m pip install --upgrade pip setuptools wheel
pip install -U tox
python setup.py build_ext --inplace
sudo apt-get update
sudo apt-get install -y graphviz
displayName: 'Install dependencies'
Expand Down Expand Up @@ -384,7 +388,10 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 4294967295))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./tools/verify_parallel_map.py
displayName: 'Run tests'
env:
QISKIT_PARALLEL: FALSE
- task: CopyFiles@2
condition: failed()
displayName: 'Copy images'
Expand Down Expand Up @@ -454,10 +461,12 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 1024))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./tools/verify_parallel_map.py
displayName: 'Run tests'
env:
LANG: 'C.UTF-8'
mtreinish marked this conversation as resolved.
Show resolved Hide resolved
PYTHONIOENCODING: 'utf-8:backslashreplace'
QISKIT_PARALLEL: FALSE
- task: CopyFiles@2
condition: failed()
displayName: 'Copy images'
Expand Down Expand Up @@ -538,6 +547,7 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 1024))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./tools/verify_parallel_map.py
env:
LANG: 'C.UTF-8'
PYTHONIOENCODING: 'utf-8:backslashreplace'
Expand Down Expand Up @@ -630,7 +640,10 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 4294967295))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./tools/verify_parallel_map.py
displayName: 'Run tests'
env:
QISKIT_PARALLEL: FALSE
- task: CopyFiles@2
condition: failed()
displayName: 'Copy images'
Expand Down Expand Up @@ -712,6 +725,7 @@ stages:
export PYTHONHASHSEED=$(python -S -c "import random; print(random.randint(1, 4294967295))")
echo "PYTHONHASHSEED=$PYTHONHASHSEED"
stestr run
python ./tools/verify_parallel_map.py
displayName: 'Run tests'
- task: CopyFiles@2
condition: failed()
Expand Down
17 changes: 8 additions & 9 deletions examples/python/stochastic_swap.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,23 +73,22 @@
# Build the expected output to verify the pass worked
expected = QuantumCircuit(qr, cr)
expected.cx(qr[1], qr[2])
expected.h(qr[2])
expected.swap(qr[0], qr[1])
expected.h(qr[0])
expected.cx(qr[1], qr[3])
expected.h(qr[3])
expected.h(qr[2])
expected.measure(qr[1], cr[0])
expected.h(qr[0])
expected.swap(qr[1], qr[3])
expected.h(qr[3])
expected.cx(qr[2], qr[1])
expected.h(qr[3])
expected.swap(qr[0], qr[1])
expected.measure(qr[2], cr[2])
expected.swap(qr[1], qr[3])
expected.measure(qr[3], cr[3])
expected.cx(qr[1], qr[0])
expected.measure(qr[1], cr[0])
expected.measure(qr[0], cr[1])
expected.cx(qr[3], qr[1])
expected.measure(qr[0], cr[3])
expected.measure(qr[3], cr[0])
expected.measure(qr[1], cr[1])
expected_dag = circuit_to_dag(expected)

# Run the pass on the dag from the input circuit
pass_ = StochasticSwap(coupling, 20, 999)
after = pass_.run(dag)
jakelishman marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
7 changes: 6 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[build-system]
requires = ["Cython>=0.27.1", "setuptools", "wheel"]
requires = ["Cython>=0.27.1", "setuptools", "wheel", "setuptools-rust"]
build-backend = "setuptools.build_meta"

[tool.black]
line-length = 100
Expand All @@ -16,3 +17,7 @@ test-command = "python {project}/examples/python/stochastic_swap.py"
# 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"

[tool.cibuildwheel.linux]
before-all = "yum install -y wget && {package}/tools/install_rust.sh"
environment = 'PATH="$PATH:$HOME/.cargo/bin"'
9 changes: 9 additions & 0 deletions qiskit/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@
import sys
import warnings

import qiskit._accelerate

# Globally define compiled modules. The normal import mechanism will not
# find compiled submodules in _accelerate because it relies on file paths
# manually define them on import so people can directly import
# qiskit._accelerate.* submodules and not have to rely on attribute access
sys.modules["qiskit._accelerate.stochastic_swap"] = qiskit._accelerate.stochastic_swap
Comment on lines +23 to +27
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a potential shortcoming of PyO3, or maybe it's general Python extensions. Either way, this seems like a fine workaround.

The second sentence in the comment is hard to understand - I think maybe it's just missing a full stop at the end of line 24 and a new sentence starting on line 25?

Copy link
Member Author

@mtreinish mtreinish Feb 23, 2022

Choose a reason for hiding this comment

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

It's a limitation of compiled extensions in general. Python's importer doesn't know how to search for submodules in a compiled extension. It's just extra annoying from rust because it doesn't give you a way to expose multiple binary libs from a single library project as it expects people to use workspaces to do multiple libraries so we're stuck either doing this or creating a separate workspace (which means a separate dir with a standalone cargo.toml) which would have a lot of disadvantages.



# qiskit errors operator
from qiskit.exceptions import QiskitError, MissingOptionalLibraryError

Expand Down
13 changes: 0 additions & 13 deletions qiskit/transpiler/passes/routing/cython/__init__.py

This file was deleted.

This file was deleted.

Loading