-
Couldn't load subscription status.
- Fork 31
fix: use graphlib in build-parallel core #759
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
base: main
Are you sure you want to change the base?
Conversation
dc4b529 to
ee18657
Compare
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.
This looks much easier to follow. I have one question about the difference in ordering inline.
|
I'm going to rewrite this PR on top of #763 once the PR is approved. |
ee18657 to
1a8583c
Compare
| continue | ||
| visited.add(edge.key) | ||
| yield edge.destination_node | ||
| for install_edge in self._traverse_install_requirements( |
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.
This needs to include the build requirements of the edge.destination_node, too.
- A needs B to build.
- B needs C to build.
- B needs D to install.
To build A you need B, C, and D.
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.
That's the beauty of graphlib and my iter_build_requirements approach -- I can handle each package in the package set independently. There is no need to track build requirements of build requirements here.
iter_build_requirements()give me a set of packages that must be installed in the build environment of a package.- then I feed all packages and their build requirements into graphlib.
TopologicalSorter.preparefreezes the graph and checks for cycles. - each iteration over the graph returns nodes that have all their build requirements satisfied. The first round are self-bootstrapping packages like
setuptoolsandflit. The second round everything that only needssetuptoolsorflit, and so on.
In your example:
A.iter_build_requirements()returnsB(direct build requirement) andD(via traverse_install_requirements(B.children)).B.iter_build_requirements()returnsC(direct build requirement)C.iter_build_requirements()is emptyD.iter_build_requirements()is empty
>>> topo = graphlib.TopologicalSorter()
>>> topo.add("A", "B", "D")
>>> topo.add("B", "C")
>>> topo.add("C")
>>> topo.add("D")
>>> topo.prepare()
>>> while topo.is_active():
... nodes = topo.get_ready()
... print(nodes)
... topo.done(*nodes)
...
('D', 'C')
('B',)
('A',)
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.
OK, I don't understand how this code ensures that both build and installation requirements are included, though.
I see on line 141 that build requirements are skipped when iterating over installation requirements and I see on line 118 that installation requirements are skipped when iterating over build requirements. So as far as I can tell, only nodes where the edge has a type of build are included. That's the bug we have today, we aren't building everything we need in the right order.
This is the same issue we had in the bootstrap code where D needs to be treated as a build requirement of A. There we had the why chain to look at to realize that D was a build dependency, even though the edge type was install. Here we don't have that why chain.
Rather than being very specific with the algorithm, we can just build all of the dependencies of A, regardless of their type, before we build A. That will fix the issue AND give us simpler code, which is a big benefit in this logic since it's been generally buggy.
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 have added additional comments and test examples. The line numbers do not match the code any more.
I think you are misunderstanding the code of iter_build_requirements. The method has two loops and two yields. The outer loop is a shallow iteration over the direct build requirements of the package. The inner loop is recursive, depth first algorithm that yields the installation requirements for each build requirement.
- all direct build dependencies (packages in
[build-system].requires) - the children of packages in (1) that are installation requirements
- recursive dependencies of (2), depth first
- ...
iter_build_requirements() returns the entire build dependency set -- all packages that have to be installed in the build environment of a package.
The DependencyGraph.get_build_topology method feeds all nodes + all their entire build dependency set into the topological sorter. On every loop, the topological sorter returns all nodes that have no unsatisfied dependencies. In other words, the new algorithm immediately starts to build a package when its entire build dependency set is satisfied.
The algorithm may build a package although one of its installation requirements is not satisfied, yet. That's okay, because we don't have to install packages immediately. The algorithm ensures that all packages are processes, because all packages are in the graph.
Convert the `DependencyNode` and `DependencyEdge` into data classes. The classes are frozen (immutable) and support comparison, sorting, and hashing. Signed-off-by: Christian Heimes <[email protected]>
Extend `DependencyNode` to get all install dependencies and build requirements. The new method return unique dependencies by recursively walking the dependency graph. The build requirements include all recursive installation requirements of build requirements. Signed-off-by: Christian Heimes <[email protected]>
The `DependencyGraph` can now construct a `graphlib.TopologicalSorter` of dependencies and their build dependencies. The graph sorter returns packages according to their build order. Signed-off-by: Christian Heimes <[email protected]>
Our custom dependency tracker for build-parallel code has a bug. It does not track and handle installation requirements of build requirements correctly. This causes build bugs when the installation requirement tree of a build dependency is deep. This PR completely rewrites and replaces the core logic of build-parallel command with stdlib's `graphlib` module. The `TopologicalSorter` detects cycles and tracks when a new node becomes ready. Nodes with a dependency on an exclusive build node now become built-ready earlier. I also changed the code to use a single instance of `ThreadPoolExecutor`. It's a tiny bit more efficient and saves one level of indention. See: python-wheel-build#755 Signed-off-by: Christian Heimes <[email protected]>
1a8583c to
59c500f
Compare
|
Closing in favor of #796 |
Our custom dependency tracker for build-parallel code has a bug. It does not track and handle installation requirements of build requirements correctly. This causes build bugs when the installation requirement tree of a build dependency is deep.
This PR completely rewrites and replaces the core logic of build-parallel command with stdlib's
graphlibmodule. TheTopologicalSorterdetects cycles and tracks when a new node becomes ready. Nodes with a dependency on an exclusive build node now become built-ready earlier.I also changed the code to use a single instance of
ThreadPoolExecutor. It's a tiny bit more efficient and saves one level of indention.See: #755