Skip to content

Conversation

@wujingyue
Copy link
Collaborator

No description provided.

@wujingyue wujingyue requested a review from Priya2698 January 2, 2026 21:45
@wujingyue
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented Jan 2, 2026

Review updated until commit d51d263

Description

  • Removed HostIrLowering option from StreamTest class constructor

  • Simplified StreamTest to be direct alias for NVFuserTest

  • Added explanatory comment about test file purpose and location

  • StreamTest no longer requires special HostIR lowering configuration

Changes walkthrough

Relevant files
Enhancement
test_stream.cpp
Simplify StreamTest by removing HostIrLowering configuration

tests/cpp/test_stream.cpp

  • Removed EnableOptionsGuard and HostIrLowering enablement from
    StreamTest constructor
  • Changed StreamTest class to be simple alias using NVFuserTest
  • Added documentation comment explaining test file purpose and migration
    to Python tests
  • +4/-6     

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Minimal Change Validation

    This is a straightforward cleanup PR that removes the HostIrLowering option from StreamTest constructor. While the change appears safe, verify that the tests still pass and that HostIrLowering functionality is either now default or no longer needed for these stream parallelism tests.

    // The tests in this file verify building blocks for stream parallelism, e.g.,
    // sharding propagation and KernelExecutor. End-to-end tests have been moved to
    // tests/python/direct/test_stream.py because the Python API is sufficient.
    using StreamTest = NVFuserTest;

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Jan 2, 2026

    Greptile Summary

    Simplified the StreamTest fixture by removing the custom constructor that enabled HostIrLowering, converting it to a simple type alias of NVFuserTest.

    • Removed custom StreamTest class with constructor that set EnableOption::HostIrLowering
    • Replaced with type alias: using StreamTest = NVFuserTest;
    • Added explanatory comment noting that the tests verify building blocks for stream parallelism (sharding propagation, KernelExecutor) and that end-to-end tests have moved to tests/python/direct/test_stream.py

    The change indicates that the HostIrLowering option is no longer required for these specific stream tests, likely because they focus on lower-level building blocks rather than full execution paths that would require host IR lowering.

    Confidence Score: 5/5

    • This PR is safe to merge with minimal risk
    • The change is a straightforward simplification that removes an unnecessary option flag from test setup. The remaining tests (AddPerStream, HaveDifferentShardings, ForwardPropagation, BackwardPropagation, ShardedAllocation, ReplicatedAllocation) test low-level building blocks that don't require HostIrLowering. The PR adds clear documentation explaining the scope of these tests, and the change aligns with recent commit 6ea151d which moved higher-level stream matmul tests to Python.
    • No files require special attention

    Important Files Changed

    Filename Overview
    tests/cpp/test_stream.cpp Removed StreamTest class constructor that enabled HostIrLowering option, replaced with type alias to NVFuserTest, and added clarifying comment about test scope

    Sequence Diagram

    sequenceDiagram
        participant Test as StreamTest (Test Suite)
        participant NVFuser as NVFuserTest (Base)
        participant Options as EnableOptionsGuard
        
        Note over Test,NVFuser: Before PR
        Test->>Options: StreamTest() constructor
        Options->>Options: set(EnableOption::HostIrLowering)
        Test->>NVFuser: Inherits from NVFuserTest
        
        Note over Test,NVFuser: After PR
        Note over Test: using StreamTest = NVFuserTest
        Test->>NVFuser: Direct type alias (no custom setup)
        Note over Test,Options: HostIrLowering option no longer needed
    
    Loading

    @wujingyue
    Copy link
    Collaborator Author

    !test

    @wujingyue wujingyue added the enable-auto-merge Auto-merge a PR when: 1) PR mergeable 2) Internal CI complete 3) No failures label Jan 5, 2026
    @wujingyue wujingyue merged commit 06ed9cb into main Jan 5, 2026
    45 of 46 checks passed
    @wujingyue wujingyue deleted the wjy/streamtest branch January 5, 2026 20:58
    @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 5, 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.

    3 participants