-
Notifications
You must be signed in to change notification settings - Fork 75
CMake Dependency Validation & Reporting #5740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…d basic constraint requirements.
|
Review updated until commit 55bce4e Description
|
| Relevant files |
|---|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 PR contains tests |
| 🔒 Security concerns No major security concerns identified. The JSON export functionality should be reviewed to ensure only safe variables are exported. |
| ⚡ Recommended focus areas for review |
JSON Export Security
|
Greptile SummaryThis PR introduces a comprehensive CMake dependency validation and reporting system that helps users diagnose and resolve build setup issues early. The implementation adds a Python-based reporting layer on top of CMake's dependency detection to provide clear, actionable error messages with installation instructions. Core Changes:
Key Features:
Technical Quality: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CMake
participant LogCapture
participant DepHandlers as Dependency Handlers
participant DepUtils as DependencyUtilities
participant Python as check_dependencies.py
participant ReqClasses as Requirement Classes
User->>CMake: Run cmake
CMake->>CMake: enable_language(CUDA)
CMake->>CMake: include DependencyRequirements.cmake
CMake->>CMake: include DependencyUtilities.cmake
CMake->>LogCapture: include LogCapture.cmake
alt NVFUSER_ENABLE_DEPENDENCY_REPORT=ON
CMake->>LogCapture: start_capture()
LogCapture->>LogCapture: Override message() function
LogCapture->>LogCapture: Set LOG_CAPTURE_MODE=TRUE
loop For each dependency
CMake->>DepHandlers: handle_<dependency>()
DepHandlers->>DepHandlers: find_package() or custom validation
DepHandlers->>DepUtils: set_dependency_report_status()
DepUtils->>DepUtils: Check version compatibility
DepUtils->>DepUtils: Set NVFUSER_REQUIREMENT_<name>_STATUS
DepUtils->>DepUtils: Update NVFUSER_DEPENDENCIES_OK if failure
DepHandlers-->>LogCapture: message() calls captured to buffer
end
CMake->>LogCapture: stop_capture(DEP_LOGS)
LogCapture->>LogCapture: Set LOG_CAPTURE_MODE=FALSE
LogCapture-->>CMake: Return captured logs
CMake->>DepUtils: report_dependencies()
DepUtils->>DepUtils: export_dependency_json(nvfuser_dependencies.json)
DepUtils->>DepUtils: Write all CMake variables to JSON
DepUtils->>Python: execute_process(check_dependencies.py)
Python->>Python: Load JSON file
Python->>ReqClasses: Create requirement objects for each dependency
ReqClasses->>ReqClasses: Parse CMake variables (status, version, found, optional)
Python->>Python: generate_report()
Python->>ReqClasses: format_status_line() for each requirement
ReqClasses-->>Python: Return colored status lines
Python->>User: Print formatted dependency report
alt Any failures detected
Python->>ReqClasses: generate_help() for failed requirements
ReqClasses-->>Python: Return installation instructions
Python->>User: Print help section with instructions
end
Python-->>DepUtils: Exit with status code
DepUtils->>User: Display Python output
CMake->>LogCapture: dump_captured_logs(DEP_LOGS)
LogCapture->>LogCapture: Parse buffer entries (TYPE<<<DELIM>>>CONTENT)
LogCapture->>User: Print detailed CMake output
alt NVFUSER_DEPENDENCIES_OK=FALSE
CMake->>User: FATAL_ERROR with failure message
end
else NVFUSER_ENABLE_DEPENDENCY_REPORT=OFF
loop For each dependency
CMake->>DepHandlers: handle_<dependency>()
DepHandlers->>DepHandlers: find_package(REQUIRED) - fails immediately
end
end
|
|
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
35 files reviewed, 5 comments
wujingyue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| INCOMPATIBLE = "INCOMPATIBLE" | ||
|
|
||
|
|
||
| class Requirement(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's ABC used here for? I remember using it a lot in Python 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract Base Class module for python. The details are unfortunately lost on me, it looks like there is something called Protocols now in python but I'm more familiar with this pattern
python/utils.py
Outdated
| "-B", | ||
| cmake_build_dir, | ||
| ] | ||
| if config.cutlass_max_jobs > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 453 is the reason I prefer always -D to conditional -D.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR now builds CUTLASS by default, so once this hits main everyone will be building the CUTLASS lib as a part of their workflow (unless they explicitly disable it). I set the default from 0 to 2 in CMake (per the recommendation in the comments) as I started running out of memory. I don't think everyone want to set the max job flag for cutlass on every build. --- We can go back to ALWAYS passing this option here if we'd like, but we then need to maintain the default in 2 places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to how the report interacts with CMake for this PR, I am of the belief that CMake should generally be the source of truth for builds (since CMake is responsible for orchestrating compilation) and that the python setup process should just be an interface to CMake from python for overriding defaults. Logic that is performed by the python setup process should focus more on python build/installation related tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set the default from 0 to 2 in CMake (per the recommendation in the comments) as I started running out of memory.
SGTM!
I am of the belief that CMake should generally be the source of truth for builds (since CMake is responsible for orchestrating compilation)
Makes sense.
we then need to maintain the default in 2 places.
Yes, that's bad.
With this PR, I'm seeing two different default values already: config.cutlass_max_jobs=0 and CUTLASS_MAX_JOBS=2. This is confusing.
What do you think about the following contract? All fields in BuildConfig are None by default. This function (def cmake) only feed configs that are not None to the cmake command. Other configs will stay as they are in CMakeCache.txt.
(Doesn't have to be in this PR of course.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd like to come back to this specifically at some point and enforce the defaults purely in CMake where we can. I think there needs to be some more standardization/ consistency between environment and build option naming too. (tagging w/ an issue)
The default here of 0 is a little clunky, I'll update just this option to use a None check for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
35 files reviewed, 1 comment
xwang233
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Once the internal CI builds pass, we're ready to go. Thanks for working on this.
|
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
35 files reviewed, 1 comment
Co-authored-by: Jingyue Wu <wujingyue@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
35 files reviewed, 4 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
36 files reviewed, 1 comment
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
36 files reviewed, 1 comment
|
!build |
|
!build |
|
|
||
| if os_type == "Linux": | ||
| if platform_info.get("ubuntu_based"): | ||
| print(" # Ubuntu/Debian (LLVM APT repository):") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not trust apt install clang-<version>? cc @mdavis36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was brought over from 5609. I typically do, this could probably be expanded to mention at least trying it. However, this is the recommended way from llvm and would additionally allow someone on an older debian version to install a very modern llvm version without having to mess with updating repositories.
Summary
This PR aims to aid new users in setting up and installing nvFuser successfully. This is done by providing user a comprehensive python report (based on #5609) as early as possible in the build process:
Differences (This PR vs #5609)
The outcome of the report is determined by CMake's evaluation of the constraints we place on requirements. The report has no effect on the ability to build nvFuser. All failure logic is define by the CMake system.
The report scripts are used to aid in formatting and printing pertinent information to the user. This is done by directly referencing CMake variables in python and allowing python to handle complicated string manipulation and formatting (which CMake is really bad at...).
The contents of the help messages largely remains the same as #5609. Giving user guidance based on their build platform.
CMake Changes
cmake/DependencyRequirements.cmakeis the single source of truth for version requirements, components and the state ofOPTIONALfor each dependency.NVFUSER_ENABLE_DEPENDENCY_REPORTis by defaultON. If this is setOFFthen dependencies will be evaluated as "normal" in CMake and the build configuration will exit of the first failure.cmake/deps/handle_<name>.cmakefile for some organization/clarity.Success Case
LANGUAGESthe project is built for first - this cant be skipped AFAIK.NVFUSER_ENABLE_DEPENDECY_REPORT=Off)Failure Case (example : pybind11 version too low)
Report fails with installation instructions for users.
FATAL_ERRORwhen pybind11 mismatches.