-
Notifications
You must be signed in to change notification settings - Fork 42
feat: add TrackingTopologicalSorter #795
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
Conversation
|
Adding the use case to the PR as it is not clear from the PR description |
|
The code looks fine to me. I will take deeper look tomorrow as well. @dhellmann PTAL. |
|
@tiran The class has no logging of decisions which is why this will be difficult to debug
Can we add |
|
Here is what AI agent suggested me for the debugging: It can give us all the required information during debugging. |
`TopologicalSorter.get_ready()` returns a node only once. The tracking topological sorter keeps track which nodes are marked as done. The `get_available()` method returns nodes again and again, until they are marked as done. The graph is active until all nodes are marked as done. Individual nodes can be marked as exclusive nodes. ``get_available`` treats exclusive nodes special and returns: 1. one or more non-exclusive nodes 2. exactly one exclusive node that is a predecessor of another node 3. exactly one exclusive node The class uses a lock for ``is_activate`, ``get_available`, and ``done``, so the methods can be used from threading pool and future callback. Signed-off-by: Christian Heimes <[email protected]>
tiran
left a comment
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 addressed most of your code reviews.
About logging: I have not added logging on purpose. It is the responsibility of the caller to log what is going on. It would be too noisy to do logging here and in the caller.
|
@tiran What's your opinion on adding the dump_state() to this in future as mentioned in #795 (comment) . dump_state() can be only used for debugging. |
I don't think we need it. PR #796 is introducing a new debug tool to analyze the build steps of a graph: $ fromager graph build-graph e2e/build-parallel/graph.json
Build dependencies (6):
cython==3.1.1, flit-core==3.12.0, packaging==25.0, setuptools-scm==8.3.1, setuptools==80.8.0, wheel==0.46.1
Build rounds:
1. flit-core==3.12.0, setuptools==80.8.0
2. cython==3.1.1, imapclient==3.0.1, jinja2==3.1.6, markupsafe==3.0.2, more-itertools==10.7.0, packaging==25.0
3. setuptools-scm==8.3.1, wheel==0.46.1
4. imapautofiler==1.14.0, jaraco-classes==3.4.0, jaraco-context==6.0.1, jaraco-functools==4.1.0, keyring==25.6.0, pyyaml==6.0.2
Building 16 packages in 4 rounds. |
TopologicalSorter.get_ready()returns a node only once. The tracking topological sorter keeps track which nodes are marked as done. Theget_available()method returns nodes again and again, until they are marked as done. The graph is active until all nodes are marked as done.Individual nodes can be marked as exclusive nodes.
get_availabletreats exclusive nodes special and returns:The class uses a lock for
is_activate`,get_available`, anddone, so the methods can be used from threading pool and future callback.