-
Notifications
You must be signed in to change notification settings - Fork 8.2k
feat: add terminal node detection to Graph class #10789
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
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9bb2d7b to
91b211c
Compare
dkaushik94
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.
Looks good!
| Returns: | ||
| list[str]: List of vertex IDs that are terminal nodes. | ||
| """ | ||
| return [vertex.id for vertex in self.vertices if not self.successor_map.get(vertex.id, [])] |
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.
@coderabbitai what are your thoughts on this?
How is the parallel graphs tackled?
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.
@edwinjosechittilappilly Great question! The implementation looks solid and handles parallel graphs naturally. Here's my analysis:
Implementation Thoughts
The get_terminal_nodes() method uses a straightforward approach by checking the successor_map to identify vertices with no outgoing edges. This is both efficient and correct since the successor_map is already maintained by the graph structure.
Handling Parallel Graphs
For parallel graphs (multiple independent branches executing concurrently), this implementation handles them elegantly:
Example scenario:
Input -> Branch A -> Terminal A
-> Branch B -> Terminal B
-> Branch C -> Terminal C
The method will return all terminal nodes: [Terminal A, Terminal B, Terminal C]. This is the correct behavior because:
- No special handling needed - The method doesn't distinguish between serial vs. parallel paths; it simply identifies all vertices without successors
- Natural coverage - Multiple disconnected branches each get their terminal nodes included
- Useful for downstream logic - Returning all terminals allows callers to determine when the entire graph (including all parallel branches) has completed execution
Potential Considerations
- Isolated nodes - A single vertex with no edges is correctly identified as terminal (covered in your tests)
- Cyclic graphs - Vertices in cycles won't be terminal unless they have no edges leaving the cycle
- Empty graph - Returns empty list (correct behavior)
The implementation is clean and leverages existing graph infrastructure well. For a POC, this is a solid foundation for terminal-node detection work!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10789 +/- ##
==========================================
- Coverage 32.96% 32.95% -0.01%
==========================================
Files 1387 1387
Lines 65385 65387 +2
Branches 9680 9680
==========================================
- Hits 21551 21547 -4
- Misses 42736 42742 +6
Partials 1098 1098
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5a3e9fb to
9db2912
Compare
ogabrielluiz
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.
LGTM
fe1de68 to
538d7e4
Compare
76582da to
22439a5
Compare
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Jkavia. * #10789 (comment) The following files were modified: * `src/lfx/src/lfx/graph/graph/base.py`
edwinjosechittilappilly
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.
LGTM
Part of POC to detect terminal nodes, adding a dead method block to be used in future work.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.