Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Oct 17, 2025

Make the greedy scheduler enabled by default. Removed NVFUSER_ENABLE=greedy_scheduler and added NVFUSER_DISABLE=greedy_scheduler just in case.

@naoyam
Copy link
Collaborator Author

naoyam commented Oct 17, 2025

!test --diff

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

Review updated until commit 3a79735

Description

  • Enable greedy scheduler by default

  • Remove explicit enable flags in tests

  • Add disable option for greedy scheduler

  • Update scheduler activation logic


Changes walkthrough 📝

Relevant files
Enhancement
options.cpp
Move greedy scheduler to disable options                                 

csrc/options.cpp

  • Removed greedy_scheduler from enable options
  • Added greedy_scheduler to disable options
  • +1/-1     
    options.h
    Update option enums                                                                           

    csrc/options.h

  • Removed GreedyScheduler from EnableOption
  • Added GreedyScheduler to DisableOption
  • +1/-1     
    Bug fix
    greedy.cpp
    Update scheduler enable logic                                                       

    csrc/scheduler/greedy.cpp

  • Changed check from enable to disable option
  • Updated rejection reason message
  • Added comment about current op support
  • +8/-5     
    Tests
    test_greedy.cpp
    Clean up test setup                                                                           

    tests/cpp/test_greedy.cpp

  • Removed explicit greedy scheduler enable
  • Removed temporary test case
  • +0/-34   
    test_moe.cpp
    Remove redundant scheduler enable                                               

    tests/cpp/test_moe.cpp

  • Removed explicit greedy scheduler enable
  • Simplified test conditions
  • +0/-4     
    test_scatter.cpp
    Clean up test setup                                                                           

    tests/cpp/test_scatter.cpp

    • Removed explicit greedy scheduler enable
    +0/-1     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Logic Change

    The condition for enabling the greedy scheduler has been changed from checking an enable option to checking a disable option. This alters the default behavior and may affect existing workflows that rely on the previous enable/disable logic.

    bool GreedyScheduler::canScheduleCompileTime(Fusion* fusion) {
      if (isOptionDisabled(DisableOption::GreedyScheduler)) {
        scheduler_debug_utils::canScheduleRejectReason(
            SchedulerType::Greedy, "Disabled");
        return false;
      }
    Option Removal

    The "greedy_scheduler" option has been removed from the enable options and added to the disable options. This change in configuration management should be validated for consistency with the project's option handling design.

    const std::unordered_map<std::string, EnableOption>& getEnableOptions() {
      static const std::unordered_map<std::string, EnableOption> available_options =
          {
              {"cutlass_scheduler", EnableOption::CutlassScheduler},
              {"fuse_matmul", EnableOption::FuseMatmul},
              {"fuse_multiple_matmuls", EnableOption::FuseMultipleMatmuls},
              {"id_model", EnableOption::IdModel},
              {"id_model_extra_validation", EnableOption::IdModelExtraValidation},
              {"io_to_lower_precision", EnableOption::IoToLowerPrecision},
              {"kernel_db", EnableOption::KernelDb},
    Test Removal

    A test case named TMP has been removed, which might have been used for temporary testing or debugging. The impact of removing this test should be assessed to ensure no functionality is left untested.

    INSTANTIATE_TEST_SUITE_P(
        ,
        GreedySchedulerTestShmemSize,
        testing::Values(128, 256, 512, 1024, 2048, 4096),
        [](const auto& info) {
          std::ostringstream os;
          os << info.param;
          return os.str();
        });
    
    } // namespace nvfuser

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Oct 17, 2025

    !test --diff


    auto constrained_ops = getAllConstrainedOps(fusion);
    if (constrained_ops.empty()) {
    // Only enabled for these ops for now. Notably, PadOp, which is
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Small adjustment. Since the scheduler is not yet capable of optimization such as vectorization, it's only enabled for ops that are not supported by any other schedulers.

    return os.str();
    });

    TEST_F(GreedySchedulerTest, TMP) {
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Accidentally added

    @naoyam naoyam marked this pull request as ready for review October 17, 2025 06:13
    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.

    1 participant