-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Introduce requirement setup for integrations #2303
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2303 +/- ##
=======================================
Coverage 98.22% 98.22%
=======================================
Files 87 87
Lines 4056 4056
=======================================
Hits 3984 3984
Misses 72 72 ☔ View full report in Codecov by Sentry. |
Good idea. I've already hit this issue several times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a high level this looks great. I like this structure a lot! 👍🏻
* Add link to public agenda for community call to README * remove anchor to the heading for agenda Co-authored-by: nate stemen <[email protected]> --------- Co-authored-by: nate stemen <[email protected]>
* updates dev_requirements for Qiskit 1.0.2 * updates qiskit_utils to not use execute * updates conversions to use qasm2.dumps * updates I gate * adds TransformationPass to _transform_registers * updates test * updates zne tests * fixes ddd tests * fixes pec tests * fixes zne scaling tests * adds tests for transpiler * updates aer version * updates docs examples * slightly refactors qiskit transpilations * updates mitiq paper codeblock * pin myst-parser dependency * fixes docstring and adds tests for multi-register circuit * applies suggestions from comments --------- Co-authored-by: Alessandro Cosentino <[email protected]>
@natestemen @jordandsullivan This is now ready to review. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have updated make install
to include all these requirements, can we also update this line in the main CONTRIBUTING guide to say make install
rather than pip install -e ".[development]"
?
@jordandsullivan Thanks for the thorough review. I addressed / replied to all your comments. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alessandro!
Looking forward to this helping users use Mitiq with less dependency issues.
I was brainstorming potential issues that could still arise, and I came up with the following scenario.
- User gets set up in their desired frontend (not cirq), writing ciruits and running them
- They learn about Mitiq
- They run
pip install mitiq
without looking through the docs too much - They hit compatibility issues
If this users then come to us, it should be easy to help solve their problem, as we can ask them to run pip install mitiq[frontend]
.
This is not a blocker, but I just wanted to write this down both for my understanding1, and so we all know there are still some potential issues this does not solve. I don't think there's any magic bullet for this problem, however.
Footnotes
-
If I'm misunderstanding something, let me know! ↩
@natestemen You're right, that's a valid scenario. One way to mitigate it would be to use the tools from pkg_resources.working_set.require('qiskit~=2.0.0')
VersionConflict: (qiskit 1.0.2 (/opt/miniconda3/envs/mitiqenv/lib/python3.11/site-packages), Requirement.parse('qiskit~=2.0.2')) vs. pkg_resources.working_set.require('qiskit~=1.0.2')
[qiskit 1.0.2 (/opt/miniconda3/envs/mitiqenv/lib/python3.11/site-packages)
...
] Perhaps something for a follow-up PR, subject to prioritization, in case we get more dependency issues reported. |
Yup agreed, lets only pursue further ideas if we get more complaints. Thanks for jotting down a potential next step :) |
Merged this. After the two reviewers approvals, I have only merged main into the branch and solved the merge conflicts. |
Description
At the moment, we don't pin the version of integration packages (Braket, Qiskit, etc.) in the Mitiq package that we ship to users. This causes incompatibility issues, e.g, #2208.
This PR introduces integration packages as extra dependencies.
Example: if a user wants to use Mitiq with Qiskit, they can run
in their enviroment, and similarly for all other integrations we support. This will guarantee that the version of the integration packages we support are compatible with the ones in the user's environment.
🎪 One important bonus is that now the main Mitiq package only depends on
cirq-core
as opposed to the fullcirq
package 🎪This fixes #1201 (see Issue description)
Docs
Readme has been updated.
Note: There will be a lapse of time between merging this PR and the next release, when the README will not be synced with reality of the package. We may consider split the Readme change and ship that along with the release PR. (cc @FarLab as milestone manager.)
Testing
I tested that:
pip install -e .[development]
pip install mitiq
(without any extra)