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

Make tweedledum a hard requirement #6588

Merged
merged 9 commits into from
Jul 1, 2021
Merged

Conversation

mtreinish
Copy link
Member

Summary

This commit switches the tweedledum requirement from being optional to a
hard requirement for installing qiskit-terra. We rely on tweedledum to
synthesize phase oracles which is commonly used functionality and several
issues have been opened. This use of tweedledum will likely continue to
grow so we should just list it as a requirement moving forward.

We originally made it optional because the functionality depending on
tweedledum was isolated to just the classical function compiler which
wasn't widely used. There were also packaging issues in the past where
the available precompiled binaries for tweedledum didn't support all of
our supported environments, but those have been resolved. There is still
an issue for arm64 macOS binaries but Qiskit doesn't have wide support
for that yet (although there is a job publishing universal2 binaries for terra
nothing else supports it yet).

At the same time this commit cleans up the optional requirements list
so that aer is no longer listed there and we add an 'all' extra so that
people can have a simple entypoint to install all the optional extras at
once.

Details and comments

Fixes #6333
Fixes #1253

This commit switches the tweedledum requirement from being optional to a
hard requirement for installing qiskit-terra. We rely on tweedledum to
synthesize phase oracles which is commonly used functionality and several
issues have been opened. This use of tweedledum will likely continue to
grow so we should just list it as a requirement moving forward.

We originally made it optional because the functionality depending on
tweedledum was isolated to just the classical function compiler which
wasn't widely used. There were also packaging issues in the past where
the available precompiled binaries for tweedledum didn't support all of
our supported environments, but those have been resolved. There is still
an issue for arm64 macOS binaries but Qiskit doesn't have wide support
for that yet (although there is a job for terra).

At the same time this commit cleans up the optional requirements list
so that aer is no longer listed there and we add an 'all' extra so that
people can have a simple entypoint to install all the optional extras at
once.

Fixes Qiskit#6333
Fixes Qiskit#1253
@mtreinish mtreinish requested a review from a team as a code owner June 16, 2021 15:41
@mtreinish mtreinish added this to the 0.18 milestone Jun 16, 2021
@mtreinish mtreinish added the Changelog: API Change Include in the "Changed" section of the changelog label Jun 16, 2021
@mtreinish
Copy link
Member Author

I pushed up boschmitt/tweedledum#143 to add wheels for non-x86 architectures, we probably should have those before merging this as we support aarch64 linux. For arm64 macOS last I checked those are still blocked on scikit-build. The fix for that was merged to master: scikit-build/scikit-build@fd17e59 but hasn't been included in a release yet. I think once that is released nothing is stopping tweedledum from publishing universal2 or arm64 macOS wheels too

@1ucian0
Copy link
Member

1ucian0 commented Jun 17, 2021

on hold until boschmitt/tweedledum#143 gets merged.

@1ucian0 1ucian0 added the on hold Can not fix yet label Jun 17, 2021
@nonhermitian
Copy link
Contributor

tweedledum master works on arm64 macs with the master of scikit-build

@boschmitt
Copy link
Contributor

tweedledum master works on arm64 macs with the master of scikit-build

Thanks for the heads up, I will work on automating the CI to build the builds fo the other platforms supported by qiskit.

@boschmitt
Copy link
Contributor

boschmitt commented Jun 23, 2021

Here is the current status: https://github.com/boschmitt/tweedledum/actions/runs/963306146

tweedledum will have a hard time supporting s390x. The reason lies in its external dependencies, which seems to require a little endian architecture to properly run. I might try to find a workaround, but it seems it won't an easy fix.

(https://github.com/boschmitt/tweedledum/actions/runs/967073706)

@mtreinish
Copy link
Member Author

@boschmitt cool, looks good. When you upload non-x86_64 wheels to pypi we can remove the on hold from this PR and prepare to merge it.

@boschmitt
Copy link
Contributor

I think this won't make it to this release. I found that some tests are not passing on these "weird" architectures. The failing tests are for functionalities that are not use by Qiskit right now, but it would be better to sort things out before releasing. (I will still try to make it, but debugging on the CI or emulated environments takes time)

@mtreinish
Copy link
Member Author

mtreinish commented Jun 26, 2021

@boschmitt is aarch64 working ok? That's the only platform that really matters for this pr (since it's the only architecture terra supports). Ppc64le and s390x are more just nice to have if/when we start fully supporting them in Qiskit, but aren't blockers for anything.

@boschmitt
Copy link
Contributor

@boschmitt is aarch64 working ok? That's the only platform that really matters for this pr (since it's the only architecture terra supports). Ppc64le and s390x are more just nice to have if/when we start fully supporting them in Qiskit, but aren't blockers for anything.

Today I made it work. The problem was not only big-endianness vs little-endianness, but also some code that seems to not work on any platform other than normal intel/amd64. (The culprit code lies in a dependency of a dependency)

requirements.txt Outdated Show resolved Hide resolved
mtreinish and others added 2 commits June 29, 2021 07:42
Co-authored-by: Bruno Schmitt <bruno.schmitt@epfl.ch>
@mtreinish mtreinish removed the on hold Can not fix yet label Jun 29, 2021
@mtreinish
Copy link
Member Author

Removing on hold with the recent 1.1.0 release all the wheels look good: https://pypi.org/project/tweedledum/1.1.0/#files so this should be unblocked now

@1ucian0
Copy link
Member

1ucian0 commented Jul 1, 2021

while solving conflict in setup (because #6580), I left bip-mapper dependencies out of all. I dont feel particularly strong about this, so be free to include it.

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

classical_function decorator raises import error missing tweedledum Missing tweedledum as required package
4 participants