-
Notifications
You must be signed in to change notification settings - Fork 70
Enable TensorIndexer (part 2) #5561
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
|
!test --diff |
Description
|
| Relevant files | |||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Tests |
|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 No relevant tests |
| ⚡ Recommended focus areas for review |
SetUp method implementation
|
Greptile OverviewGreptile SummaryEnables TensorIndexer/IdModel for 8 test suites by converting test fixtures from simple typedefs to proper classes with
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Test as Test Framework
participant TF as Test Fixture
participant Parent as NVFuserTest
participant Guards as OptionsGuards
participant IdModel as IdModel/TensorIndexer
Test->>TF: Instantiate test fixture
TF->>Parent: Constructor
Parent->>Guards: Create enable_options_guard_<br/>and disable_options_guard_
Test->>TF: Call SetUp()
TF->>Parent: NVFuserTest::SetUp()
Parent->>Parent: Check GPU capability (Pascal+)
TF->>Guards: Set EnableOption::IdModel {"all"}
Guards->>IdModel: Enable TensorIndexer
Test->>Test: Run test cases
Test->>TF: Teardown
TF->>Guards: Destructor restores options
Guards->>IdModel: Restore previous state
|
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.
8 files reviewed, 1 comment
| void SetUp() override { | ||
| DisableOptionsGuard::getCurOptions().set(DisableOption::ResizeScheduler); | ||
| EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"}); | ||
| } |
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.
syntax: missing NVFuserTest::SetUp() call at the beginning
| void SetUp() override { | |
| DisableOptionsGuard::getCurOptions().set(DisableOption::ResizeScheduler); | |
| EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"}); | |
| } | |
| void SetUp() override { | |
| NVFuserTest::SetUp(); | |
| DisableOptionsGuard::getCurOptions().set(DisableOption::ResizeScheduler); | |
| EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"}); | |
| } |
jjsjann123
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.
🥳 Looking forward to enable it by default!
Enabled TensorIndexer with some of the tests. Code diffs look benign.