Skip to content

Delete legacy build#5921

Open
wujingyue wants to merge 1 commit intomainfrom
wjy/setup
Open

Delete legacy build#5921
wujingyue wants to merge 1 commit intomainfrom
wjy/setup

Conversation

@wujingyue
Copy link
Collaborator

No description provided.

@wujingyue
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

Description

  • Remove legacy root-level setup.py file

  • Delete argparse-based argument parsing from python/utils.py

  • Simplify python/setup.py to use BuildConfig directly

  • Eliminate command-line argument parsing for build configuration

Changes walkthrough

Relevant files
Enhancement
setup.py
Simplify build configuration setup                                             

python/setup.py

  • Replace create_build_config() with direct BuildConfig instantiation
  • Remove argparse setup and sys.argv manipulation
  • Keep environment variable override and C++ standard validation
  • +3/-8     
    utils.py
    Remove legacy argparse configuration                                         

    python/utils.py

  • Remove argparse import and parse_args() function
  • Delete create_build_config() function entirely
  • Keep override_build_config_from_env() function unchanged
  • +0/-175 
    setup.py
    Remove legacy root setup file                                                       

    setup.py

  • Delete entire root-level setup.py file
  • Remove legacy build configuration and argument parsing
  • Eliminate duplicate version_tag and main functions
  • +0/-107 

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Configuration Options Migration

    The PR removes all argparse-based command line options (cmake-only, no-python, no-cutlass, no-test, no-benchmark, no-ninja, build-with-ucc, build-with-asan, build-without-distributed, explicit-error-check, no-system-nvtx, debug, debinfo, build-dir, install-dir, install_requires, extras_require, version-tag, wheel-name, cpp standard). Need to verify that all these configuration options are still accessible through environment variables and that the migration path is clearly documented for users who were using these command line flags.

    def main():
        config = BuildConfig()
        # Override build config from environment variables.
        override_build_config_from_env(config)
    
        if "clean" in sys.argv:
            # only disables BUILD_SETUP, but keep the argument for setuptools
            config.build_setup = False
    
        if config.cpp_standard < 20:
            raise ValueError("nvfuser requires C++20 standard or higher")
    
        run(config, version_tag(config), relative_path="..")
    Backward Compatibility

    The complete removal of argparse functionality means users can no longer pass build configuration via command line arguments. This is a breaking change that should be clearly documented. Need to verify that the environment variable approach provides equivalent functionality and that the documentation explains the new configuration method.

    # Override BuildConfig with environment variables. Only change if variable
    # exists. Do not use default to override argparse.
    def override_build_config_from_env(config):

    Test failures

    • (Medium, 3) Thunder higher-order inplace alias update shape mismatch in nvFuser CUDA tests

      Test Name A100 GB200 H100 Source
      thunder.tests.test_update_aliases.test_higher_order_inplace_alias_update_nvfuser_cuda_thunder.dtypes.float32

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Feb 4, 2026

    Greptile Overview

    Greptile Summary

    This PR removes the legacy argparse-based build system in favor of a cleaner environment variable-based approach, simplifying the build interface and aligning with modern Python packaging standards (PEP 517).

    Changes:

    • Deleted root-level setup.py (107 lines) - legacy build entry point no longer needed
    • Removed create_build_config() and parse_args() functions from python/utils.py (180 lines)
    • Updated python/setup.py to instantiate BuildConfig() directly with defaults, relying solely on environment variables for customization
    • Removed command-line argument forwarding logic (sys.argv manipulation)

    Impact:

    • Build command changes from python setup.py --cmake-only to NVFUSER_BUILD_CMAKE_ONLY=1 pip install -e python
    • Documentation already updated to reflect new build approach (README.md shows environment variable usage)
    • Breaking change: .github/workflows/lint.yml:57 still uses old CLI syntax python setup.py --cmake-only and will fail

    Confidence Score: 2/5

    • PR has a critical syntax error in CI workflow that will break the lint job
    • The code changes themselves are clean and well-structured, but the PR missed updating .github/workflows/lint.yml which still uses the deprecated CLI argument syntax (--cmake-only). This will cause the clang-tidy CI job to fail immediately.
    • .github/workflows/lint.yml needs to be updated to use environment variables instead of CLI arguments

    Important Files Changed

    Filename Overview
    python/setup.py Simplified build setup by removing argparse-based CLI and using environment variables exclusively
    python/utils.py Removed 180 lines of argparse argument parsing logic and create_build_config function
    setup.py Deleted entire root-level setup.py file (legacy build entry point)

    Sequence Diagram

    sequenceDiagram
        participant User
        participant pip
        participant python/setup.py
        participant BuildConfig
        participant utils.override_build_config_from_env
        participant utils.run
    
        User->>pip: pip install --no-build-isolation -e python -v
        pip->>python/setup.py: main()
        python/setup.py->>BuildConfig: BuildConfig() [defaults]
        BuildConfig-->>python/setup.py: config object
        python/setup.py->>utils.override_build_config_from_env: override_build_config_from_env(config)
        Note over utils.override_build_config_from_env: Reads NVFUSER_BUILD_* env vars
        utils.override_build_config_from_env-->>python/setup.py: updated config
        python/setup.py->>python/setup.py: version_tag(config)
        python/setup.py->>utils.run: run(config, version_tag, relative_path="..")
        Note over utils.run: Executes cmake build and setuptools
        utils.run-->>pip: Build complete
    
    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.

    3 files reviewed, 1 comment

    Edit Code Review Agent Settings | Greptile

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Feb 4, 2026

    Additional Comments (1)

    .github/workflows/lint.yml
    the command python setup.py --cmake-only is incompatible with the new build system that uses environment variables instead of CLI arguments

              NVFUSER_BUILD_CMAKE_ONLY=1 python setup.py
    

    Copy link
    Collaborator

    @xwang233 xwang233 left a comment

    Choose a reason for hiding this comment

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

    as long as CI passes 🤞

    Copy link
    Collaborator

    @mdavis36 mdavis36 left a comment

    Choose a reason for hiding this comment

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

    Looks like greptile is correct about the .github lintrunner. The clang-tidy ob it trying to do a full build because the arg is ignored here

    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