-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[IR][Transform] Implement SequentialNode::ResolveDependency() #18645
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
This commit implements the ResolveDependency() method for SequentialNode to handle pass dependency resolution. The implementation: 1. Collects all enabled passes from the current list 2. Recursively collects required passes from the global registry 3. Builds a dependency graph using adjacency lists 4. Performs topological sort using Kahn's algorithm 5. Detects and handles circular dependencies with warnings 6. Updates the passes list with the sorted order Key features: - Handles transitive dependencies correctly - Respects PassContext to filter passes by opt_level - Gracefully handles unresolvable dependencies - Warns about circular dependencies Also includes fixes for Relax/Torch from_exported_program crash with FakeTensor and lifted tensors. Tested: - All 3 tests pass - Code compiles successfully - Lint checks pass
Summary of ChangesHello @Dayuxiaoshui, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the IR transformation framework by introducing robust pass dependency resolution within Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new dependency resolution mechanism for Sequential passes using topological sort, which is a great improvement over the existing simple dependency handling. It also includes a fix for crashes when importing PyTorch ExportedProgram with FakeTensor.
The implementation of ResolveDependency in C++ is well-structured. However, my main concern is that this new method doesn't seem to be called anywhere, and the existing Sequential::operator() still uses the old, simpler dependency resolution logic. This means the core improvement of this PR might not be active.
I've also added some comments on the C++ implementation regarding exception handling and a small simplification. Additionally, the new Python tests for dependency resolution seem to only cover basic cases and don't validate the new topological sort logic. It would be great to add more comprehensive tests for different dependency graphs.
The fix for the PyTorch frontend looks good and the accompanying test is a valuable addition.
This commit implements the ResolveDependency() method for SequentialNode to handle pass dependency resolution using topological sort. Key changes: - Add TryGetPass() helper function to safely retrieve passes from global registry without throwing exceptions - Implement ResolveDependency() method that: * Collects all enabled passes from the current list * Recursively collects required passes (including transitive dependencies) * Builds a dependency graph * Performs topological sort using Kahn's algorithm * Detects and warns about circular dependencies - Update SequentialNode::operator() to call ResolveDependency() at the beginning to activate the new dependency resolution logic - Add comprehensive test cases covering: * Simple dependency chains * Shared dependencies * Transitive dependencies * Opt-level filtering All tests pass and the code compiles successfully.
This commit implements the ResolveDependency() method for SequentialNode to handle pass dependency resolution. The implementation:
Key features:
Also includes fixes for Relax/Torch from_exported_program crash with FakeTensor and lifted tensors.
Tested: