refactor: Remove cugraph, refactor GNN plugin cmake #4014
Conversation
WalkthroughSignificant changes, this pull request brings. Dependencies related to CuGraph, removed they are, simplifying the Exa.TrkX plugin's track building process. Across multiple files, modifications span, including CMakeLists.txt configurations, Python bindings, and source files. The shift from CuGraph to BoostTrackBuilding, a notable change it is. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Examples/Python/python/acts/examples/reconstruction.py (1)
1801-1801: Wise choice, replacing CugraphTrackBuilding with BoostTrackBuilding is!Align with PR objectives, this change does. Remove heavy cugraph dependency while maintaining track building functionality, it does.
Consider documenting performance characteristics of BoostTrackBuilding compared to previous implementation, we should. Help future maintainers understand trade-offs, it would.
Plugins/ExaTrkX/CMakeLists.txt (1)
Line range hint
82-89: Document the linker flag in more detail, we should!Good that explained the need for
-no-as-neededflag, you have. But enhance the documentation further, we could. Specific scatter operations required and potential implications, mention we should.Add more detailed comment:
- # Should not discard TorchScatter even if its not needed at this point - # since we need the scatter_max operation in the torch script later + # Prevent discarding of TorchScatter library even when not directly referenced + # Required for: + # - scatter_max operation in torch script + # - future scatter operations in graph neural networks + # Note: This may increase runtime dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CMakeLists.txt(0 hunks)Examples/Python/python/acts/examples/reconstruction.py(1 hunks)Examples/Python/src/ExaTrkXTrackFinding.cpp(0 hunks)Plugins/ExaTrkX/CMakeLists.txt(3 hunks)Plugins/ExaTrkX/README.md(0 hunks)Plugins/ExaTrkX/include/Acts/Plugins/ExaTrkX/CugraphTrackBuilding.hpp(0 hunks)Plugins/ExaTrkX/src/CugraphTrackBuilding.cpp(0 hunks)Plugins/ExaTrkX/src/weaklyConnectedComponentsCugraph.hpp(0 hunks)docs/getting_started.md(0 hunks)
💤 Files with no reviewable changes (7)
- Plugins/ExaTrkX/README.md
- Plugins/ExaTrkX/include/Acts/Plugins/ExaTrkX/CugraphTrackBuilding.hpp
- docs/getting_started.md
- Plugins/ExaTrkX/src/weaklyConnectedComponentsCugraph.hpp
- Examples/Python/src/ExaTrkXTrackFinding.cpp
- CMakeLists.txt
- Plugins/ExaTrkX/src/CugraphTrackBuilding.cpp
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: merge-sentinel
- GitHub Check: lint
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (4)
Examples/Python/python/acts/examples/reconstruction.py (1)
Line range hint
1780-1801: Consistent track builder implementation across backends, verify we must!Notice that both Torch and Onnx backends use BoostTrackBuilding now, I do. Good for consistency, this is!
Run this command to confirm no remaining references to CugraphTrackBuilding exist:
✅ Verification successful
Purged completely, CugraphTrackBuilding has been!
Searched the entire codebase with the Force, I have. No trace of CugraphTrackBuilding remains, hmm. Consistent use of BoostTrackBuilding across both backends, there is. Balance to the Force, this brings!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to CugraphTrackBuilding rg "CugraphTrackBuilding"Length of output: 25
Script:
#!/bin/bash # Search for any cugraph-related references case-insensitively rg -i "cugraph" # Also search specifically in Python files and config files find . -type f \( -name "*.py" -o -name "*.json" -o -name "*.yaml" -o -name "*.yml" \) -exec grep -i "cugraph" {} \;Length of output: 137
Plugins/ExaTrkX/CMakeLists.txt (3)
8-22: Organized well, the source files are!Pleased with the clean separation of ONNX and Torch sources using
target_sources, I am. Clear organization by feature, this brings. Strong with the Force, this code structure is!
74-74: Successfully removed cugraph dependency, you have!Clean and simple, the ONNX configuration now is. Removed the cugraph dependency as planned, we have. Private linking of OnnxRuntime, sufficient it is!
49-59: Verify CUDA 20 compatibility with all dependencies, we must!Updated to CUDA standard 20, I see. Wise decision it may be, but verify compatibility with all dependencies and target platforms, we must.
Run this command to check CUDA toolkit version requirements:
|
Removes cuda because it is not tested and a very heavy dependency
Refactors the exatrkx cmake code
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
Removed Dependencies
cugraphlibrary dependency from Exa.TrkX pluginBuild Configuration
Track Finding
Documentation