Use retworkx #3810
Use retworkx #3810
Conversation
This commit is the start of the conversion from using networkx as the internal structure of the dagcircuit to use retworkx instead.
The to_networkx() method is used to return a networkx graph object representing the internal dag structure. Since the previous commit switched this to retwork's PyDAG class this function was broken. To enable the continued function of networkx based utilities (mainly the dag drawer) this commit adds an implementation that iterates over PyDAG twice (so it's not super efficient at O(2n)) to add the nodes and edges. While this isn't the most effecient algorithm it works. To speed it up in the future a function can be added to retworkx to either directly generate the networkx object or alternatively generate the parameters so it can be passed to networkx in a single call.
The predecessors and successors methods of the DAGCircuit class were expecting an iterator returned not a list. Thist commit corrects the return type by wrapping the list in iter().
The output of retworkx.dag_longest_path() returns the node indexes, not the nodes themselves. This commit updates the DAGCircuit.longest_path() method to make sure it's returning DAGNode objects, not the indexes.
The apply operation back conditional tests were using in_edges and out_edges to check the structure of the dag. However with retworkx the return type from those functions change to return the node id instead of the node instance. This was causing the tests to fail because it was attempting to compare a DAGNode object with an int. This commit fixes the issue by making comparing DAGNode._node_id to the value of in_edges() and out_edges().
Using python to deduplicate the output from retworkx in the dagcircuit class gets increasinly inefficient as the number of nodes increases. Relying on retworkx to deduplicate moving forward will be more efficient.
… Update hardcoded test cases to be zero indexed to match retworkx
…ndices Zero index hardcoded test cases
mtreinish
requested review from
1ucian0,
ajavadia,
itoko,
kdk and
maddy-tod
as code owners
|
I can see #2860 uses NetworkX #3762 (which is a restricted version of the above) also uses NetworkX's |
|
I'll have to look into those algorithms to see how they work, but none of the algorithms I've looked at so far have been difficult to implement. We can open issues about those on the retworkx repo. You can continue to use networkx with this pr, if you want it from the dagcircuit class you can do |
|
|
||
| def quantum_successors(self, node): | ||
| """Returns iterator of the successors of a node that are | ||
| connected by a quantum edge as DAGNodes.""" | ||
| for successor in self.successors(node): | ||
| for successor in reversed(list(self.successors(node))): |
There was a problem hiding this comment.
This was needed from the test results, I think the traversal order was different or something. But, there was a test case expecting the output in an order that was backwards from what retworkx was doing
There was a problem hiding this comment.
If the tests are wrong (e.g. asserting an order on an unordered output), it'd be better to fix the tests (and ideally validate that they still pass on networkx) rather than introduce workarounds in the code. If there are too many to tackle at once (which seems likely), can we collect them in an issue to address later?
There was a problem hiding this comment.
I just removed this and reran the tests locally only one test is failing:
test.python.test_dagcircuit.TestDagOperations.test_quantum_successors
---------------------------------------------------------------------
Captured traceback:
~~~~~~~~~~~~~~~~~~~
Traceback (most recent call last):
File "/home/mtreinish/git/qiskit/qiskit-core/test/python/test_dagcircuit.py", line 378, in test_quantum_successors
self.assertEqual(next(successor_cnot).type, 'out')
File "/usr/lib64/python3.8/unittest/case.py", line 912, in assertEqual
assertion_func(first, second, msg=msg)
File "/usr/lib64/python3.8/unittest/case.py", line 1292, in assertMultiLineEqual
self.fail(self._formatMessage(msg, standardMsg))
File "/usr/lib64/python3.8/unittest/case.py", line 753, in fail
raise self.failureException(msg)
AssertionError: 'op' != 'out'
- op
+ out
There was a problem hiding this comment.
I think the failure here is related to @itoko's next comment on the using out_edges vs in_edges for the definition of edges.
There was a problem hiding this comment.
Hmm, I tried both updating the edges function and removing the reversed here individually and both introduced failures by themselves. But let me give fixing that func and removing this together and see if I can get that working
|
Ok #3762 is going to merge soon, can you rebase after that? Hopefully not much needs changing. Then I'll take another look |
Taking a quick look at #3762 I don't think there will be any changes needed, since it only interacts with the dagcircuit api which doesn't change here, but I'll rebase it after it merges and keep an eye on it. |
|
I just ran the tests locally after rebasing and everything worked fine without any changes |
As @itoko pointed out in review the edges() function in networkx was using the outgoing edges from the graph not the incoming edges for it's edges() function. This doesn't normally matter for the case of returning edges for the entire dagcircuit because the list will be the same given the structure. But, there was a possible edge case when edges() was called with an output node or input node that would have behaved differently. This was not caught in the tests because there are no places in the code that currently do this, but there are potentially users of the function relying on this behavior currently so it's better to mirror the functionality. To ensure this remains consistent a test case is added to verify input and output nodes behave as expected when passed into edges().
There was as stary iter() wrapper around the output of topological_nodes() in the tests. This was leftover from an earlier iteration of the branch where topological_nodes() returned a list. This has been fixed (to maintain compatibility with the current api) so the iter is no longer needed.
|
After our discussion earlier today I've been thinking more about how we can provide a fallback for the next release in case something unforeseen comes up. Besides merging this sooner so we have time to test and debug any issues that arise, I was thinking maybe adding a copy of the existing networkx DAGCircuit class and freezing changes to DAGCircuit's api for the rest of the release. Then when 0.13.0 is released we delete that fallback copy. So instead of having to keep up with changes and having a dual maintenance problem we just agree to not change the api until after the release (or an alternative abstraction layer on top of dagcircuit). Then if a user encounters an unforeseen issue with the new library they can just monkey patch |
|
I'm on board with this idea. Two comments:
For the dual maintenance issue, I don't expect it will be a huge problem, |
Sure we can do this, monkey patching just seemed the easiest to me if we documented it in the release note (since it wouldn't require adding anything to the code).
I still think making it opt-out is a better approach here. This isn't really a deprecation it's a new feature (significantly improved performance) and we don't normally go through such a conservative process for new features. There will always be bugs (and often are in new features), but I agree that since this is such a core piece of terra being a bit more conservative makes sense. So I'd rather we give users a relief valve in case we're missing something and they encounter a bug. So we don't push out a release that breaks someone with no alternative. If we make it opt-in I doubt most users will ever see it until we flip the switch for real and then we're just delaying finding any potential bugs we're missing until that future date.
The reason I'm concerned about dual maintenance is that there are at least 2 or 3 PRs open right now besides this one which touch the DAGCircuit API: #3772, #3503, and #3860 (or docs in the case of this one). It's true in general there aren't many changes but I'd rather just have us make sure we're agreeing to minimize changes during the period of having multiple copies. Otherwise we're basically making ourselves duplicate all the work |
I think that's the key concern. Unlike adding a new feature, this is a restructuring that affects nearly every qiskit user.
The goal of a release of opt-in is increasing exposure beyond the terra development team, to the broader community and those with an active performance problem that could be addressed by retworkx. True there will always be bugs, but a rollout helps us gauge rough scale and severity, and more importantly, to learn early in the transition if there are users or use cases for which the new structure is difficult or not viable. From all the work on retworkx, we don't expect it will be, but there will always be unforeseen issues.
There are a few ways we can manage dual implementations, but I wouldn't want to forgo a rollout to ease this process. (None of those three PRs are urgent.) |
This commit defines the __slots__ property for the base Bit class. __slots__ is used to explicitly declare the members of a class. Which means that internally the class no longer is using __dict__ and reduces both memory and speed overhead associated with that. [1] After switching to retworkx in Qiskit#3810 bit.__eq__ becomes the biggest bottleneck in performance. This will improve the runtime performance of that. [1] https://docs.python.org/3/reference/datamodel.html?#slots
* Define __slots__ for bit class This commit defines the __slots__ property for the base Bit class. __slots__ is used to explicitly declare the members of a class. Which means that internally the class no longer is using __dict__ and reduces both memory and speed overhead associated with that. [1] After switching to retworkx in #3810 bit.__eq__ becomes the biggest bottleneck in performance. This will improve the runtime performance of that. [1] https://docs.python.org/3/reference/datamodel.html?#slots * Regenerate pickles for new class format
|
This has been superseded by #3916 which makes the DAGCircuit switchable and defaults to retworkx. This will give users an option to fallback to networkx for the 0.13.0 release. So I'm closing this I won't delete the branch though to keep this as a reference. |
* Define __slots__ for bit class This commit defines the __slots__ property for the base Bit class. __slots__ is used to explicitly declare the members of a class. Which means that internally the class no longer is using __dict__ and reduces both memory and speed overhead associated with that. [1] After switching to retworkx in Qiskit#3810 bit.__eq__ becomes the biggest bottleneck in performance. This will improve the runtime performance of that. [1] https://docs.python.org/3/reference/datamodel.html?#slots * Regenerate pickles for new class format
Summary
This PR switches the graph library used for the DAGCircuit class to use retworkx[1] instead of networkx. This provides a significant speed up over networkx because retworkx is written in rust instead of pure python.
[1] https://github.com/mtreinish/retworkx
Details and comments
Co-Authored-By: Lauren Capelluto [email protected]