Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Jan 14, 2026

Following up with #5781

@naoyam
Copy link
Collaborator Author

naoyam commented Jan 14, 2026

!test

@github-actions
Copy link

github-actions bot commented Jan 14, 2026

Auto-merge Status

❌ Internal CI is finished (pending)
✅ No failed checks
✅ PR is mergeable
ℹ️ PR mergeable_state: unstable

Description

  • Enable TensorIndexer id_model and id_model_extra_validation for all legacy Python tests

  • Move environment variable configuration from individual tests to pytest conftest

  • Centralize NVFUSER_ENABLE options in tests/python/conftest.py

  • Remove redundant validation logic from utils.py

Changes walkthrough

Relevant files
Configuration changes
conftest.py
Add pytest configuration for TensorIndexer options             

tests/python/conftest.py

  • Add os import for environment variable manipulation
  • Implement pytest_configure hook to set NVFUSER_ENABLE options
  • Append id_model and id_model_extra_validation to NVFUSER_ENABLE for
    all tests
  • +17/-0   
    Enhancement
    utils.py
    Remove redundant validation logic from test utilities       

    tests/python/utils/utils.py

  • Remove redundant id_model_extra_validation check and append logic
  • Simplify execute call to use _enable_options directly
  • Clean up test utility function by removing duplicate validation
  • +0/-2     

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    🔒 Security concerns

    No security concerns identified. The changes are limited to test configuration and environment variable management.

    ⚡ Recommended focus areas for review
    Environment Variable Scope

    The global environment variable setting in conftest.py affects all tests in the directory. While this is likely intentional for enabling TensorIndexer, consider if this might have unintended side effects on other tests that don't expect these options to be enabled.

    existing = os.environ.get("NVFUSER_ENABLE", "")
    new_options = "id_model,id_model_extra_validation"
    
    if existing:
        os.environ["NVFUSER_ENABLE"] = f"{existing},{new_options}"
    else:
        os.environ["NVFUSER_ENABLE"] = new_options

    Test failures (partial, pipeline still running)

    • (Medium, 3) Thunder-compiled nanoGPT returns zero loss (scalar mismatch) in test_networks on multiple CUDA runners

      Test Name A100 GB200 H100 Source
      thunder.tests.test_networks.test_nanogpt_complete_autograd_nvfuser_cuda_thunder.dtypes.float32

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Jan 14, 2026

    Greptile Summary

    Enabled TensorIndexer (id_model and id_model_extra_validation) for legacy Python tests by setting the NVFUSER_ENABLE environment variable globally in conftest.py during pytest configuration.

    • Added pytest_configure hook in tests/python/conftest.py that appends id_model,id_model_extra_validation to the NVFUSER_ENABLE environment variable, preserving any existing values
    • Removed redundant runtime addition of id_model_extra_validation from tests/python/utils/utils.py:325 in the exec_nvfuser method, as this is now handled globally via the environment variable
    • This change aligns the legacy Python test configuration with the approach used for python_direct tests and C++ tests in earlier PRs
    • The implementation correctly handles existing NVFUSER_ENABLE values by appending rather than overwriting, ensuring compatibility with any pre-existing configuration

    Confidence Score: 5/5

    • This PR is safe to merge with no issues identified
    • The changes are straightforward and well-aligned with the established pattern from previous TensorIndexer enablement PRs. The implementation correctly appends to existing environment variables rather than overwriting them, and removes code that would now be redundant. The approach is consistent with how python_direct tests were updated in PR Enable TensorIndexer with all python_direct tests #5781, and the changes are minimal with clear intent.
    • No files require special attention

    Important Files Changed

    Filename Overview
    tests/python/conftest.py Added pytest hook to enable TensorIndexer globally via NVFUSER_ENABLE environment variable for all tests in this directory
    tests/python/utils/utils.py Removed redundant runtime addition of id_model_extra_validation option, now handled globally via environment variable

    Sequence Diagram

    sequenceDiagram
        participant Pytest
        participant conftest
        participant OSEnv
        participant Test
        participant NVFuserTest
        participant FusionDefinition
        participant CppOptions
        
        Pytest->>conftest: pytest_configure(config)
        conftest->>OSEnv: Get NVFUSER_ENABLE
        OSEnv-->>conftest: existing value or empty
        conftest->>OSEnv: Set NVFUSER_ENABLE with id_model options
        
        Pytest->>Test: Run test
        Test->>NVFuserTest: exec_nvfuser(fusion_func, inputs)
        Note over NVFuserTest: Removed runtime addition<br/>of id_model_extra_validation
        NVFuserTest->>FusionDefinition: execute(inputs, options)
        FusionDefinition->>CppOptions: Read NVFUSER_ENABLE env var
        CppOptions-->>FusionDefinition: id_model, id_model_extra_validation
        FusionDefinition->>CppOptions: Apply runtime enable options
        FusionDefinition->>FusionDefinition: Execute with TensorIndexer enabled
    
    Loading

    @naoyam naoyam requested a review from jjsjann123 January 14, 2026 20:20
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Jan 14, 2026

    There's a failure with a Thunder test (thunder.tests.test_networks.test_nanogpt_complete_autograd_nvfuser_cuda_thunder.dtypes.float32). Not sure why Thunder tests can be impacted from this change.

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Jan 15, 2026

    There's a failure with a Thunder test (thunder.tests.test_networks.test_nanogpt_complete_autograd_nvfuser_cuda_thunder.dtypes.float32). Not sure why Thunder tests can be impacted from this change.

    Retried the tests and they are gone. Wonder if there's some non-deterministic issue.

    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.

    LGTM

    @jjsjann123 jjsjann123 added the enable-auto-merge Auto-merge a PR when: 1) PR mergeable 2) Internal CI complete 3) No failures label Jan 15, 2026
    @naoyam naoyam merged commit 2b0490a into main Jan 15, 2026
    60 of 61 checks passed
    @naoyam naoyam deleted the tensorindexer_pytest_legacy branch January 15, 2026 00:34
    @github-actions github-actions bot removed the enable-auto-merge Auto-merge a PR when: 1) PR mergeable 2) Internal CI complete 3) No failures label Jan 15, 2026
    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.

    2 participants