Skip to content

Revert #5609#5728

Merged
wujingyue merged 1 commit intomainfrom
md/revert-5609
Dec 20, 2025
Merged

Revert #5609#5728
wujingyue merged 1 commit intomainfrom
md/revert-5609

Conversation

@mdavis36
Copy link
Collaborator

Reverting #5609

Discussion on failing builds : teams

@github-actions
Copy link

Description

  • Reverts PR Check for required environment before attempting installation #5609 that added comprehensive prerequisite validation system

  • Removes all prerequisite validation code from setup.py and tools/prereqs/ module

  • Eliminates NCCL include directory detection and LLVM project-local installation logic

  • Updates build requirements and documentation to remove validation dependencies

Changes walkthrough

Relevant files
Bug fix
16 files
setup.py
Remove prerequisite validation from setup.py                         
+0/-23   
__init__.py
Delete prerequisite validation package                                     
+0/-96   
build_tools.py
Delete CMake and Ninja validation                                               
+0/-140 
compiler.py
Delete C++ compiler validation                                                     
+0/-102 
exceptions.py
Delete prerequisite validation exceptions                               
+0/-23   
git.py
Delete git submodules validation                                                 
+0/-137 
llvm.py
Delete LLVM validation                                                                     
+0/-238 
nccl.py
Delete NCCL validation for distributed builds                       
+0/-311 
platform.py
Delete platform detection utilities                                           
+0/-129 
python_packages.py
Delete Python packages validation                                               
+0/-375 
python_version.py
Delete Python version validation                                                 
+0/-114 
requirements.py
Delete centralized requirements definitions                           
+0/-269 
validate.py
Delete validation orchestrator                                                     
+0/-168 
utils.py
Remove NCCL include directory functions                                   
+0/-34   
setup.py
Remove prerequisite validation from main setup                     
+0/-30   
CMakeLists.txt
Remove NCCL and LLVM detection from CMake                               
+0/-34   
Documentation
1 files
README.md
Update documentation to remove validation info                     
+4/-17   
Configuration changes
2 files
pyproject.toml
Remove pybind11 from build requirements                                   
+1/-1     
requirements.txt
Simplify build dependencies                                                           
+1/-3     

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Incomplete revert

The get_pip_nccl_include_dir function was removed from utils.py, but there may be other references to NCCL include directory handling that should be reviewed for consistency with the revert.

def cmake(config, relative_path):
    from tools.memory import get_available_memory_gb

    # make build directories
    cwd = os.path.dirname(os.path.abspath(__file__))
    cmake_build_dir = (
        os.path.join(cwd, "build") if not config.build_dir else config.build_dir
    )
    if not os.path.exists(cmake_build_dir):
        os.makedirs(cmake_build_dir)

    install_prefix = (
        config.install_dir if config.install_dir else get_default_install_prefix()
    )

    from tools.gen_nvfuser_version import (
        get_pytorch_cmake_prefix,
        get_pytorch_use_distributed,
    )

    # this is used to suppress import error.
    # so we can get the right pytorch prefix for cmake
    import logging

    logger = logging.getLogger("nvfuser")
    logger_level = logger.getEffectiveLevel()
    logger.setLevel(logging.CRITICAL)
Build requirements consistency

The pyproject.toml was updated to remove pybind11[global]>=2.0 requirement, but requirements.txt still includes pybind11[global]. This inconsistency should be verified to ensure build dependencies are properly specified.

[build-system]
requires = ["setuptools>=42", "wheel", "ninja", "cmake>=3.18"]
build-backend = "setuptools.build_meta:__legacy__"

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile Summary

This PR cleanly reverts PR #5609, which added prerequisite validation for build dependencies. The revert removes ~2,248 lines including the entire python/tools/prereqs/ module and associated integration code.

Key Changes:

  • Deleted 10 validation modules (python/tools/prereqs/*.py) that checked Python version, CMake, Ninja, GCC, LLVM, PyTorch, NCCL, and git submodules
  • Removed validation calls from setup.py and python/setup.py that ran before build
  • Reverted CMakeLists.txt changes: removed project-local .llvm/ search logic and NCCL include path handling
  • Restored requirements.txt to minimal dependencies (removed cmake>=3.18 and versioned pybind11[global]>=2.0)
  • Updated README.md to restore original compiler support (GCC 12.4+, clang 16+) vs the stricter requirements (GCC 13+, clang 19+)
  • Removed get_pip_nccl_include_dir() function from python/utils.py

Revert Quality:
The revert is clean and complete - all 20 files touched by PR #5609 have been properly reverted. No prereqs-related code remains in the codebase. The python/tools/prereqs/ directory has been fully deleted.

Impact:
Users will no longer get early validation errors with actionable guidance. Instead, builds may fail with cryptic CMake/linker errors if prerequisites are missing. This returns the project to its pre-validation state.

Confidence Score: 5/5

  • This PR is safe to merge - it's a clean, complete revert with no issues
  • The revert perfectly undoes all changes from PR Check for required environment before attempting installation #5609 (2,248 lines removed across 20 files). All prerequisite validation code has been cleanly removed with no remnants, stale imports, or broken references. The codebase returns to its exact pre-validation state.
  • No files require special attention - all changes are clean deletions/reverts

Important Files Changed

Filename Overview
python/setup.py Removed prerequisite validation logic and environment variable documentation - clean revert
setup.py Removed deprecation warning and prerequisite validation from root setup - clean revert
CMakeLists.txt Removed NCCL include path and project-local LLVM search logic - clean revert
requirements.txt Removed cmake and pybind11 version constraints, reverted to original minimal dependencies
README.md Removed prerequisite validation section, restored original GCC 12.4 support and clang 16+ support
python/utils.py Removed get_pip_nccl_include_dir() function and related NCCL path logic - clean revert

Sequence Diagram

sequenceDiagram
    participant User
    participant Setup as setup.py / python/setup.py
    participant Utils as python/utils.py
    participant CMake as CMakeLists.txt
    participant PreReqs as python/tools/prereqs/*
    
    Note over User,PreReqs: Before Revert (PR #5609)
    User->>Setup: pip install --no-build-isolation -e python -v
    Setup->>PreReqs: validate_prerequisites()
    PreReqs->>PreReqs: Check Python, CMake, Ninja, GCC 13+, LLVM 18.1+, etc.
    alt Validation Fails
        PreReqs-->>Setup: raise Exception
        Setup-->>User: Exit with error message
    else Validation Succeeds
        PreReqs-->>Setup: Success
        Setup->>Utils: cmake(config)
        Utils->>Utils: get_pip_nccl_include_dir()
        Utils->>CMake: Pass -DNCCL_INCLUDE_DIR
        CMake->>CMake: Search .llvm/ for project-local LLVM
        CMake-->>User: Build completes
    end
    
    Note over User,PreReqs: After Revert (This PR)
    User->>Setup: pip install --no-build-isolation -e python -v
    Setup->>Utils: cmake(config)
    Note over PreReqs: prereqs module deleted
    Utils->>CMake: No NCCL_INCLUDE_DIR passed
    CMake->>CMake: Standard LLVM search only
    CMake-->>User: Build completes (or fails with cryptic errors)
Loading

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 20, 2025

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

@xwang233
Copy link
Collaborator

!build

@wujingyue wujingyue merged commit 3df4486 into main Dec 20, 2025
19 checks passed
@wujingyue wujingyue deleted the md/revert-5609 branch December 20, 2025 22:00
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