Remove qiskit-toqm tests and requirements-dev.txt entry #9057
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Im Qiskit#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 Qiskit#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 Qiskit#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.
|
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:
|
jakelishman
previously approved these changes
Nov 2, 2022
There was a problem hiding this comment.
I also have #8967 that will decouple development requirements from optional accelerators as well, but this looks logically correct right now.
jakelishman
added
stable backport potential
Pull Request Test Coverage Report for Build 3379816164
💛 - Coveralls |
jakelishman
approved these changes
Nov 2, 2022
mergify bot
pushed a commit
that referenced
this pull request
Nov 2, 2022
* 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
mergify bot
added a commit
that referenced
this pull request
Nov 3, 2022
…) (#9061) * Remove qiskit-toqm tests and requirements-dev.txt entry (#9057) * 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 * Remove qiskit-toqm from requirements-dev.txt This fixes a merge conflict and removes the qiskit-toqm entry from the requirements-dev.txt file * Fix lint failure Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In #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. 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.
Details and comments