add routing and math flag for selective builds#1209
Open
Iroy30 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR implements optional build-time slicing for CUOPT into independent math/LP and routing components. CMake options gate source and test compilation, build.sh validates flag combinations and propagates slice choices, Python build artifacts are generated per-slice, and runtime guards enforce that modules are only imported when the installed wheel includes them. ChangesBuild-time slicing and runtime component selection for CUOPT math/routing
Project metadata updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuopt_server/pyproject.toml`:
- Line 20: Replace the current license metadata that uses the text variant
(license = { text = "Apache-2.0" }) with the file variant so the LICENSE file is
included in built distributions; specifically update the license entry to
license = { file = "LICENSE" } in pyproject.toml (replace the existing license =
{ text = "Apache-2.0" } line).
In `@python/cuopt/CMakeLists.txt`:
- Around line 26-27: The cache variable CUOPT_PYTHON_COMPONENT should be
normalized and validated immediately after being declared: convert the cached
value to uppercase (e.g., with string(TOUPPER ... ) into a local variable) and
then check that it is one of "FULL", "LP", or "ROUTING"; if not, call
message(FATAL_ERROR ...) to fail the configure with a clear error. Update the
file to keep the existing set(... CACHE STRING ...) and set_property(...) but
add the normalization and validation logic referencing CUOPT_PYTHON_COMPONENT
and the normalized name so typos (like "routing" or "L P") are rejected at
configure time.
In `@python/cuopt/cuopt/__init__.py`:
- Around line 10-39: The import loops using _CUOPT_SLICE try multiple native
modules (names like "libcuopt_lp", "libcuopt", "libcuopt_routing") and call each
module's load_library(), but if none import successfully the code silently
continues; add a detection and raise an ImportError with a clear message when no
native library is loaded. Implement this by marking success (e.g., a boolean
like loaded = False) or by using the for-else pattern after each loop that
attempts imports (the blocks referencing _native and _native_pkg and calling
load_library()), and if the loop finishes without loading any library raise
ImportError including _CUOPT_SLICE and the attempted module names to aid
debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ffcb1da6-5f34-48b6-aacc-0da914750b72
📒 Files selected for processing (13)
build.shcpp/CMakeLists.txtcpp/src/CMakeLists.txtcpp/tests/CMakeLists.txtpython/cuopt/CMakeLists.txtpython/cuopt/cuopt/CMakeLists.txtpython/cuopt/cuopt/__init__.pypython/cuopt/cuopt/_build_variant.py.inpython/cuopt/cuopt/distance_engine/__init__.pypython/cuopt/cuopt/linear_programming/__init__.pypython/cuopt/cuopt/routing/__init__.pypython/cuopt_self_hosted/pyproject.tomlpython/cuopt_server/pyproject.toml
| ] | ||
| license = "Apache-2.0" | ||
| license-files = ["LICENSE"] | ||
| license = { text = "Apache-2.0" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify LICENSE file existence and distribution inclusion
# Check if LICENSE files exist in the repository
echo "=== Searching for LICENSE files ==="
fd -t f -i 'license' -E '.git' -E 'node_modules'
# Check if other pyproject.toml files handle license-files differently
echo -e "\n=== Checking license configuration in other pyproject.toml files ==="
fd -t f 'pyproject.toml' -x grep -A1 -B1 'license'Repository: NVIDIA/cuopt
Length of output: 554
🏁 Script executed:
cat -n python/cuopt_server/pyproject.tomlRepository: NVIDIA/cuopt
Length of output: 3456
🏁 Script executed:
# Check git history for the pyproject.toml change
cd python/cuopt_server
git log -n 3 --oneline pyproject.toml
git diff HEAD~1 HEAD pyproject.toml 2>/dev/null || echo "No previous commit to compare"Repository: NVIDIA/cuopt
Length of output: 141
🏁 Script executed:
# Check if there are more commits for this file
cd python/cuopt_server
git log --all --oneline pyproject.toml | head -20
# Also check the diff to see what changed in the recent commits
git show HEAD:pyproject.toml > /tmp/current.toml 2>/dev/null
if git show HEAD~1:pyproject.toml > /tmp/previous.toml 2>/dev/null; then
echo "=== Diff between HEAD~1 and HEAD ==="
diff -u /tmp/previous.toml /tmp/current.toml || true
else
echo "No previous version found"
fi
# Check if license-files is mentioned anywhere in the codebase
echo -e "\n=== Searching for license-files references ==="
rg 'license-files' --type py --type tomlRepository: NVIDIA/cuopt
Length of output: 1407
🏁 Script executed:
# Check the commit that mentions SPDX license identifiers
git show 2da7b39:python/cuopt_server/pyproject.toml 2>/dev/null | head -50
# Also check other pyproject.toml files in the repository to see how they handle license
echo -e "\n=== License configuration in other projects ==="
find . -name pyproject.toml -type f | xargs grep -l "license" | head -10 | xargs grep -A1 -B1 "license" | head -60Repository: NVIDIA/cuopt
Length of output: 2601
Fix LICENSE file inclusion in distribution.
The license-files = ["LICENSE"] entry was removed and the license format was changed to license = { text = "Apache-2.0" }. With the text variant, the LICENSE file is no longer included in wheel distributions. Change to license = { file = "LICENSE" } to ensure the LICENSE file is properly included per PEP 639 compliance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt_server/pyproject.toml` at line 20, Replace the current license
metadata that uses the text variant (license = { text = "Apache-2.0" }) with the
file variant so the LICENSE file is included in built distributions;
specifically update the license entry to license = { file = "LICENSE" } in
pyproject.toml (replace the existing license = { text = "Apache-2.0" } line).
Comment on lines
+26
to
+27
| set(CUOPT_PYTHON_COMPONENT "FULL" CACHE STRING "Python package slice: FULL, LP (math), or ROUTING") | ||
| set_property(CACHE CUOPT_PYTHON_COMPONENT PROPERTY STRINGS FULL LP ROUTING) |
There was a problem hiding this comment.
Fail fast on unsupported CUOPT_PYTHON_COMPONENT values.
Line [26]-Line [27] only set cache metadata; they do not enforce validity. A typo (for example, routing/L P) can silently produce the wrong slice behavior across build-time gating and runtime import checks. Please normalize and validate with a hard configure error.
Proposed fix
set(CUOPT_PYTHON_COMPONENT "FULL" CACHE STRING "Python package slice: FULL, LP (math), or ROUTING")
set_property(CACHE CUOPT_PYTHON_COMPONENT PROPERTY STRINGS FULL LP ROUTING)
+string(TOUPPER "${CUOPT_PYTHON_COMPONENT}" _cuopt_python_component_upper)
+set(_cuopt_valid_python_components FULL LP ROUTING)
+if(NOT _cuopt_python_component_upper IN_LIST _cuopt_valid_python_components)
+ message(
+ FATAL_ERROR
+ "Invalid CUOPT_PYTHON_COMPONENT='${CUOPT_PYTHON_COMPONENT}'. "
+ "Allowed values: FULL, LP, ROUTING."
+ )
+endif()
+set(CUOPT_PYTHON_COMPONENT "${_cuopt_python_component_upper}" CACHE STRING
+ "Python package slice: FULL, LP (math), or ROUTING" FORCE)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| set(CUOPT_PYTHON_COMPONENT "FULL" CACHE STRING "Python package slice: FULL, LP (math), or ROUTING") | |
| set_property(CACHE CUOPT_PYTHON_COMPONENT PROPERTY STRINGS FULL LP ROUTING) | |
| set(CUOPT_PYTHON_COMPONENT "FULL" CACHE STRING "Python package slice: FULL, LP (math), or ROUTING") | |
| set_property(CACHE CUOPT_PYTHON_COMPONENT PROPERTY STRINGS FULL LP ROUTING) | |
| string(TOUPPER "${CUOPT_PYTHON_COMPONENT}" _cuopt_python_component_upper) | |
| set(_cuopt_valid_python_components FULL LP ROUTING) | |
| if(NOT _cuopt_python_component_upper IN_LIST _cuopt_valid_python_components) | |
| message( | |
| FATAL_ERROR | |
| "Invalid CUOPT_PYTHON_COMPONENT='${CUOPT_PYTHON_COMPONENT}'. " | |
| "Allowed values: FULL, LP, ROUTING." | |
| ) | |
| endif() | |
| set(CUOPT_PYTHON_COMPONENT "${_cuopt_python_component_upper}" CACHE STRING | |
| "Python package slice: FULL, LP (math), or ROUTING" FORCE) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt/CMakeLists.txt` around lines 26 - 27, The cache variable
CUOPT_PYTHON_COMPONENT should be normalized and validated immediately after
being declared: convert the cached value to uppercase (e.g., with string(TOUPPER
... ) into a local variable) and then check that it is one of "FULL", "LP", or
"ROUTING"; if not, call message(FATAL_ERROR ...) to fail the configure with a
clear error. Update the file to keep the existing set(... CACHE STRING ...) and
set_property(...) but add the normalization and validation logic referencing
CUOPT_PYTHON_COMPONENT and the normalized name so typos (like "routing" or "L
P") are rejected at configure time.
Comment on lines
+10
to
+39
| if _CUOPT_SLICE == "LP": | ||
| for _native in ("libcuopt_lp", "libcuopt"): | ||
| try: | ||
| _m = __import__(_native, fromlist=["load_library"]) | ||
| except ModuleNotFoundError: | ||
| continue | ||
| _m.load_library() | ||
| del _m | ||
| break | ||
| del _native | ||
| elif _CUOPT_SLICE == "ROUTING": | ||
| for _native in ("libcuopt_routing", "libcuopt"): | ||
| try: | ||
| _m = __import__(_native, fromlist=["load_library"]) | ||
| except ModuleNotFoundError: | ||
| continue | ||
| _m.load_library() | ||
| del _m | ||
| break | ||
| del _native | ||
| else: | ||
| libcuopt.load_library() | ||
| del libcuopt | ||
| for _native_pkg in ("libcuopt", "libcuopt_lp", "libcuopt_routing"): | ||
| try: | ||
| _m = __import__(_native_pkg, fromlist=["load_library"]) | ||
| except ModuleNotFoundError: | ||
| continue | ||
| _m.load_library() | ||
| del _m | ||
| break | ||
| del _native_pkg |
There was a problem hiding this comment.
Silent failure when no native library is found.
If none of the native libraries (libcuopt_lp, libcuopt, etc.) can be imported, the loops complete without calling load_library() and no error is raised. The user proceeds with a broken module state that will fail later with a confusing error.
Consider raising an ImportError after the loop if no library was successfully loaded:
🐛 Proposed fix
if _CUOPT_SLICE == "LP":
+ _loaded = False
for _native in ("libcuopt_lp", "libcuopt"):
try:
_m = __import__(_native, fromlist=["load_library"])
except ModuleNotFoundError:
continue
_m.load_library()
del _m
+ _loaded = True
break
del _native
+ if not _loaded:
+ raise ImportError("Could not load native cuopt library for LP slice")
+ del _loaded
elif _CUOPT_SLICE == "ROUTING":
+ _loaded = False
for _native in ("libcuopt_routing", "libcuopt"):
try:
_m = __import__(_native, fromlist=["load_library"])
except ModuleNotFoundError:
continue
_m.load_library()
del _m
+ _loaded = True
break
del _native
+ if not _loaded:
+ raise ImportError("Could not load native cuopt library for ROUTING slice")
+ del _loaded
else:
+ _loaded = False
for _native_pkg in ("libcuopt", "libcuopt_lp", "libcuopt_routing"):
try:
_m = __import__(_native_pkg, fromlist=["load_library"])
except ModuleNotFoundError:
continue
_m.load_library()
del _m
+ _loaded = True
break
del _native_pkg
+ if not _loaded:
+ raise ImportError("Could not load any native cuopt library")
+ del _loaded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt/cuopt/__init__.py` around lines 10 - 39, The import loops using
_CUOPT_SLICE try multiple native modules (names like "libcuopt_lp", "libcuopt",
"libcuopt_routing") and call each module's load_library(), but if none import
successfully the code silently continues; add a detection and raise an
ImportError with a clear message when no native library is loaded. Implement
this by marking success (e.g., a boolean like loaded = False) or by using the
for-else pattern after each loop that attempts imports (the blocks referencing
_native and _native_pkg and calling load_library()), and if the loop finishes
without loading any library raise ImportError including _CUOPT_SLICE and the
attempted module names to aid debugging.
jameslamb
requested changes
May 14, 2026
Member
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
I left one comment asking for clarification but more broadly... can you please add a description explaining this?
I don't understand the changes that say "try split wheels first, then full libcuopt" or what the _build_variant.py.in is used for. GitHub search doesn't return any other file by that name across all of GitHub (https://github.com/search?q=path%3A_build_variant.py.in&type=code), so I'm guessing this is something cuOpt-custom? If you can explain this change in more detail I'd be happy to review again.
| ] | ||
| license = "Apache-2.0" | ||
| license-files = ["LICENSE"] | ||
| license = { text = "Apache-2.0" } |
Member
There was a problem hiding this comment.
What is the purpose of this change?
We very intentionally migrated all projects in RAPIDS away from this form and towards the form you see in the repo as part of rapidsai/build-planning#152
The form you're proposing is considered deprecated, and newer versions of build tools may stop supporting it: https://peps.python.org/pep-0639/#deprecate-license-key-table-subkeys
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Issue
Checklist