Submodule checkout during setup

[ksivaman]

Description

One of our most common build related issues is the need that users have to checkout submodules separately. This PR automates that process as much as possible. More details in docstring and comments.

Type of change

• Documentation change (change only to the documentation, either a fix or a new content)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Infra/Build change
• Code refactoring

Changes

• Automatically checkout git submodule during setup whenever possible.

Checklist:

• I have read and followed the contributing guidelines
• The functionality is complete
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds automatic git submodule initialization to setup.py through a new git_check_submodules() function that runs during the build process. The function detects if git and .gitmodules are present, validates that submodules are either uninitialized or at the correct commit (rejecting modified/ahead/behind states), then executes git submodule update --init --recursive. This addresses a common user pain point where builds fail because submodules weren't manually initialized. The implementation fits into the existing setup workflow by being called early in the build process, before CMake configuration for C++/CUDA extensions that depend on submodule contents (like cudnn-frontend, cutlass, googletest in 3rdparty/).

Critical Issues:

The error handling in lines 175-176 has a severe flaw: the broad except subprocess.CalledProcessError catches all subprocess failures, including the assertion on line 161. This means if a developer has modified submodules locally, the assertion error message (explaining how to fix it) is silently suppressed, and the build continues without properly initialized submodules. This will cause cryptic downstream build failures when CMake can't find expected headers in 3rdparty/. Additionally, if git submodule update fails due to network issues or permissions, the exception handler silently returns, allowing the build to proceed with missing dependencies.

The exception handler should either:

1. Only catch specific initialization failures while re-raising validation errors, or
2. Print a clear warning message before returning, or
3. Fail the build explicitly if submodules are required

Confidence: 2/5 - The core functionality improves user experience, but the error handling creates a significant risk of silent failures that will be harder to debug than the original "missing submodules" error.

_{1 file reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[ksivaman]

/te-ci

[greptile-apps]

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The developer has not addressed the critical error-handling issues identified in previous reviews. The PR still contains the same problematic code where a broad except subprocess.CalledProcessError on lines 175-176 silently swallows all subprocess errors, including assertion failures and git command failures. This means that if git submodule update fails due to network issues, permission problems, or the assertion on line 161 fires (when a submodule has modified/incorrect state indicated by '+' or other characters), the error is completely hidden from users, and the build continues with potentially uninitialized or incorrect submodules. The original intent—automating submodule checkout to improve user experience—remains sound, but the implementation still allows silent failures that will cause cryptic downstream build errors.

Important Files Changed

Filename	Score	Overview
setup.py	2/5	Adds git_check_submodules() function to auto-checkout submodules, but critical error-handling issues remain unaddressed from previous reviews

Confidence score: 2/5

• This PR requires careful review and should not be merged without addressing the error-handling issues that allow silent failures
• Score reflects that the broad exception handler on lines 175-176 still swallows all errors (assertion failures, git command failures), potentially causing users to encounter cryptic build failures when submodules are uninitialized or incorrect—the same critical issues raised in all three previous reviews remain unfixed
• Pay close attention to setup.py lines 152-176, specifically the exception handling logic and the assertion that can fail silently

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The PR adds automatic git submodule initialization to setup.py by checking submodule status and running git submodule update --init --recursive when needed. A new environment variable NVTE_SKIP_SUBMODULE_CHECKS_DURING_BUILD allows developers to bypass these checks. The implementation addresses a common user pain point where builds fail because submodules aren't checked out. However, the broad exception handler on line 180-181 creates a critical silent failure mode: it catches CalledProcessError from both the assertion failure (line 165-173) and the git update command (line 176-178), preventing users from seeing error messages when submodules are in invalid states (modified, merge conflicts, etc.). This means if a submodule has status '+' (modified) or 'U' (merge conflict), the assertion will fail but be silently swallowed, causing cryptic build failures later when CUDA/C++ compilation encounters incorrect headers.

Important Files Changed

Filename	Score	Overview
setup.py	2/5	Added automatic git submodule initialization with status validation, but broad exception handler silently catches assertion failures and git errors, preventing error messages from reaching users when submodules are in invalid states.

Confidence score: 2/5

• This PR introduces a silent failure mode that could lead to difficult-to-debug build issues when submodules are in unexpected states.
• Score reflects the critical issue where the broad except CalledProcessError on lines 180-181 catches both assertion failures (lines 165-173) and git command failures (lines 176-178), hiding all error messages from users. Additionally, the assertion itself will raise AssertionError, not CalledProcessError, so it won't be caught at all and will instead crash the build with a cryptic traceback rather than the helpful message in the assertion.
• Pay close attention to lines 162-181 in setup.py—the exception handling logic needs to distinguish between "git not available" (acceptable fallback), "submodules uninitialized" (should auto-fix), and "submodules in invalid state" (should fail with clear error message). Consider splitting the try/except blocks or catching specific exceptions.

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The latest update modifies the assertion error message in setup.py to prioritize recommending the NVTE_SKIP_SUBMODULE_CHECKS_DURING_BUILD environment variable before suggesting manual submodule checkout. The logic update also now tolerates both uninitialized ('-') and correctly initialized (' ') submodules, automatically running git submodule update --init --recursive to bring uninitialized submodules up to date. This change integrates with the build system's existing submodule validation workflow defined in git_check_submodules(), which is called early in setup to prevent builds from proceeding with incorrectly initialized dependencies. The modification improves the developer experience by providing clearer guidance and automating the common case where submodules simply need initialization rather than manual intervention.

Important Files Changed

Filename	Score	Overview
setup.py	3/5	Reordered assertion error message to recommend env var escape hatch first, then manual checkout; logic now auto-initializes uninitialized submodules

Confidence score: 3/5

• This change improves user guidance but retains the previously flagged silent error-handling behavior that may hide legitimate build issues
• Score reflects the continued presence of the broad except handler (line 180) that swallows all subprocess errors without logging, including assertion failures and git command failures, which can mask network issues, permission problems, or repository corruption
• Pay close attention to setup.py lines 156-181, particularly the exception handling block—consider whether the silent failure mode is acceptable or if errors should be logged/surfaced to prevent cryptic downstream build failures

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[ksivaman]

/te-ci