Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Nov 20, 2025

Enabled TensorIndexer with some of the tests. Code diffs look benign.

@naoyam naoyam changed the title Enable TensorIndexer Enable TensorIndexer (part 2) Nov 20, 2025
@naoyam
Copy link
Collaborator Author

naoyam commented Nov 20, 2025

!test --diff

@github-actions
Copy link

Description

  • Convert test classes from simple type aliases to proper class inheritance with SetUp methods

  • Enable IdModel option for tensor indexing functionality across multiple test files

  • Apply consistent test setup pattern to 8 different test files

  • Enable tensor indexing features for allocation domain, order inference, inlining, and other test suites

Changes walkthrough

Relevant files
Tests
test_allocation_domain.cpp
Convert allocation domain test to class-based setup           

tests/cpp/test_allocation_domain.cpp

  • Convert AllocationDomainTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Maintain existing test functionality while enabling new features
  • +7/-1     
    test_allocation_order_inference.cpp
    Convert allocation order test to class-based setup             

    tests/cpp/test_allocation_order_inference.cpp

  • Convert AllocationOrderInferenceTest from type alias to class
    inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Preserve existing allocation order testing capabilities
  • +7/-1     
    test_inlining.cpp
    Convert inlining test to class-based setup                             

    tests/cpp/test_inlining.cpp

  • Convert InliningTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Maintain existing inlining test functionality
  • +7/-1     
    test_move_pad.cpp
    Convert move pad test to class-based setup                             

    tests/cpp/test_move_pad.cpp

  • Convert MovePadTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Keep existing ResizeScheduler disable and add tensor indexing enable
  • +2/-1     
    test_move_repeat_forward.cpp
    Convert move repeat forward test to class-based setup       

    tests/cpp/test_move_repeat_forward.cpp

  • Convert MoveRepeatForwardTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Maintain existing move repeat forward testing
  • +7/-1     
    test_move_split_cat.cpp
    Convert move split cat test to class-based setup                 

    tests/cpp/test_move_split_cat.cpp

  • Convert MoveSplitCatTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Preserve existing move split cat testing capabilities
  • +7/-1     
    test_remove_bcast_squeeze.cpp
    Convert remove bcast squeeze test to class-based setup     

    tests/cpp/test_remove_bcast_squeeze.cpp

  • Convert RemoveBcastSqueezeTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Maintain existing broadcast squeeze removal testing
  • +7/-1     
    test_replay.cpp
    Convert replay test to class-based setup                                 

    tests/cpp/test_replay.cpp

  • Convert ReplayTest from type alias to class inheritance
  • Add SetUp method enabling IdModel option for tensor indexing
  • Preserve existing replay test functionality
  • +7/-1     

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review
    SetUp method implementation

    The SetUp() method implementation appears to be incomplete - it only shows the opening brace '{' without the closing brace or body. This may be a display issue in the diff, but should be verified to ensure the SetUp() method is properly implemented.

      void SetUp() override {
        DisableOptionsGuard::getCurOptions().set(DisableOption::ResizeScheduler);
        EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"});
      }
    };

    @naoyam naoyam marked this pull request as ready for review November 20, 2025 23:44
    @naoyam naoyam requested a review from jjsjann123 November 20, 2025 23:44
    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Nov 20, 2025

    Greptile Overview

    Greptile Summary

    Enables TensorIndexer/IdModel for 8 test suites by converting test fixtures from simple typedefs to proper classes with SetUp() overrides.

    • 7 files correctly call NVFuserTest::SetUp() before setting options
    • test_move_pad.cpp missing parent SetUp() call, causing GPU capability check to be skipped

    Confidence Score: 3/5

    • Safe to merge after fixing the missing parent SetUp() call in test_move_pad.cpp
    • The changes are straightforward test configuration updates, but test_move_pad.cpp has a critical bug where it doesn't call the parent's SetUp() method, which skips GPU capability validation
    • tests/cpp/test_move_pad.cpp requires attention - missing parent SetUp() call

    Important Files Changed

    File Analysis

    Filename Score Overview
    tests/cpp/test_allocation_domain.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_allocation_order_inference.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_inlining.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_move_pad.cpp 2/5 Converted constructor to SetUp() override and added IdModel enablement, but missing required parent SetUp() call causing GPU check to be skipped
    tests/cpp/test_move_repeat_forward.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_move_split_cat.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_remove_bcast_squeeze.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()
    tests/cpp/test_replay.cpp 5/5 Converted test fixture from typedef to class with SetUp() override enabling IdModel, correctly calls parent SetUp()

    Sequence Diagram

    sequenceDiagram
        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
    
    Loading

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a 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

    Edit Code Review Agent Settings | Greptile

    Comment on lines +23 to 26
    void SetUp() override {
    DisableOptionsGuard::getCurOptions().set(DisableOption::ResizeScheduler);
    EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"});
    }
    Copy link
    Contributor

    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

    Suggested change
    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"});
    }

    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a 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!

    @naoyam naoyam merged commit ee47f58 into main Nov 20, 2025
    65 of 67 checks passed
    @naoyam naoyam deleted the tensorindexer_misc_update2 branch November 20, 2025 23:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants