-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[WIP] Switch from retworkx
to rustworkx
#8340
Conversation
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:
|
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 for doing this. This PR is perhaps a little premature, since reseaux isn't released on PyPI at all yet, so we're having to change requirements files to have it pass CI. It perhaps isn't super urgent that Terra merges this, because retworkx/reseaux should be ensuring that any future versions of retworkx (not reseaux) released to PyPI don't break the compatibility we're using, and we ought to have at least a version or two's grace period where both are available for us to make the switch neatly.
Since this is (technically) a dependency change and it may change the types of various internal Terra objects that users may be depending on (it depends on whether the legacy retworkx
package freezes, or becomes a naming alias to the same Python objects in reseaux
, but either way it's not officially in Terra's control), I don't think we can include this in a patch release. That means we just need to look to merge it before the 0.22 release, which is still about 2 months away, so we've got a bit of time.
qiskit/dagcircuit/dagdependency.py
Outdated
def to_retworkx(self): | ||
"""Returns the DAGDependency in retworkx format.""" | ||
def to_reseaux(self): | ||
"""Returns the DAGDependency in reseaux format.""" |
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.
We can't change method names like this - we'll need to go through a deprecation and backwards-compatibility period.
edit: to add, if the retworkx
and reseaux
namespaces don't point to the exact same Python objects, then to_retworkx
will need to become a converter function from reseaux
format PyDiGraph
to retworkx.PyDiGraph
, so that isinstance(DAGDependency.to_retworkx(), retworkx.PyDiGraph)
is still true.
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.
I am working towards making them the same object, but that will depends on how we release 0.12
requirements.txt
Outdated
retworkx>=0.11.0 | ||
reseaux @ git+https://github.com/IvanIsCoding/retworkx@rename-retworkx |
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.
Please change this to a regular requirement so we don't forget - Terra won't be merging this PR til the named package is released on PyPI anyway.
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.
The PR is WIP, I put this more in-place so we can actually get results from the CI rather than having import errors
This is very premature indeed, it is more to check how many things break in the CI and what the size of the diff is.Currently the only failures are because the fix of Qiskit/rustworkx#614 has not been merged yet, so overall things are not too bad. We will need to add a release note of the dependency change, but if we opt for |
retworkx
to reseaux
retworkx
to rustworkx
Pull Request Test Coverage Report for Build 3207065069
💛 - Coveralls |
Closing in favor of #9162 |
Summary
Related to Qiskit/rustworkx#641
retworkx
will change its name as a courtesy to the NetworkX developers. This is the PR to migrate to the new name in qiskit-terra. Even thoughretworkx
will continue working, it is worth migrating to the new name as soon as possible as the old name will be deprecated in two releases.Details and comments
Depends on
rustworkx
0.12 (or the new name if it changes) being released to PyPI