-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove qiskit-toqm tests and requirements-dev.txt entry (backport #9057) #9061
Conversation
* Remove qiskit-toqm tests and requirements-dev.txt entry Im #9042 we removed the special case code to enable using toqm as an optional routing method. This was needed prior to qiskit-terra 0.22.0 as we didn't have the transpiler stage plugin interface. However, now that we've added a dedicated interface for external transpiler passes to integrate into transpile() the toqm passes are using that interface. However, in #9042 the tests for verifying the previously hard-coded toqm routing method path worked as before were left intact. In general this would be a good approach to ensure we're maintaining backwards compatibilty with the new modular interface with earlier releases. However, in this specific case this is causing an issue with how tests are run. Since qiskit-toqm has a necessary dependency on qiskit-terra this means when we install our development requirements before running tests toqm is pulling in the latest stable release of qiskit-terra from pypi. Then we later upgrade that with the source build before running tests. This however this bi-directional test dependency introduces a tension in a number of places. For the most recent example, when we're trying to add support for new platforms (see #9028 for an example) where the stable version of qiskit-terra does not support the platform. We're unable to run CI with qiskit-toqm being installed first since installing stable qiskit-terra won't work/ This commit removes qiskit-toqm from the requirements-dev.txt list and also removes the test cases using it to fix this conflict. In general the testing for qiskit-toqm can now be self contained since it's exercising a stable interface in terra that's tested independently. When weighing the backwards compatibility coverage vs the CI and build complexities having the bidirectional test dependency has just removing the toqm tests and development requirement is the simplest path forward. * Remove unused imports Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 8999047) # Conflicts: # requirements-dev.txt
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
This fixes a merge conflict and removes the qiskit-toqm entry from the requirements-dev.txt file
I suppose this didn't really need to be backported if it's causing problems. |
Yeah, on second thought we probably don't need this on 0.22.x. We only needed this on main to get Python 3.11 support working without tweedledum. As the bidriectional dependency from qiskit-toqm was causing tweedledum to be installed even though nothing actually was requiring it on main. But it raises a larger issue for us when backporting #9028 in that we still require tweedledum on the stable branch. We're going to have to workaround this in the backport PR probably by adding an environment marker on python_version in the requirements list for tweedledum. However, we might still need this to get Python 3.11 working in CI on this branch. I've removed the automerge tag for now though, we can wait until #9028 is backported to see whether we need this backport or not. |
In my local testing with #9071 this is still needed because of the same issue we hit in #9028 with toqm installing qiskit-terra 0.22.1 first. We can revert this post 0.22.2 if wanted. Or I can update all the CI jobs to manually install toqm after we install terra and remove it from the requirements-dev.txt list. But for right now I think we should just remove this. |
This is an automatic backport of pull request #9057 done by Mergify.
Cherry-pick of 8999047 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Mergify commands and options
More conditions and actions can be found in the documentation.
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refresh
will re-evaluate the rules@Mergifyio rebase
will rebase this PR on its base branch@Mergifyio update
will merge the base branch into this PR@Mergifyio backport <destination>
will backport this PR on<destination>
branchAdditionally, on Mergify dashboard you can:
Finally, you can contact us on https://mergify.com