Make cuopt.routing importable without a GPU#1591
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
ramakrishnap-nv
added
non-breaking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesRouting API and GPU-safe imports
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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/cuopt/routing/__init__.py`:
- Around line 27-36: Update the package hooks __getattr__ and __dir__ with the
requested type annotations: __getattr__ should accept name: str and return Any,
while __dir__ should return list[str]. Add concise docstrings describing lazy
export resolution and the AttributeError raised for unknown names in
__getattr__, and document the directory listing behavior for __dir__.
🪄 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: 7613a3f0-1f5f-4f9c-819a-689ef2f587db
📒 Files selected for processing (3)
python/cuopt/cuopt/routing/__init__.pypython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/routing/test_lazy_imports.py
| def __getattr__(name): | ||
| import importlib | ||
|
|
||
| module = _LAZY_EXPORTS.get(name) | ||
| if module is None: | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | ||
| return getattr(importlib.import_module(module), name) | ||
|
|
||
|
|
||
| def __dir__(): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target module and nearby tests to understand the intended API surface.
git ls-files python/cuopt/cuopt/routing/__init__.py python/cuopt/tests | sed -n '1,120p'
printf '\n--- routing __init__ outline ---\n'
ast-grep outline python/cuopt/cuopt/routing/__init__.py --view expanded
printf '\n--- relevant file excerpt ---\n'
sed -n '1,220p' python/cuopt/cuopt/routing/__init__.py
printf '\n--- routing test mentions ---\n'
rg -n "__getattr__|__dir__|routing\.utils|routing\.utils_wrapper|from cuopt\.routing import \*" python/cuopt/tests python/cuopt/cuopt -g '!**/__pycache__/**'Repository: NVIDIA/cuopt
Length of output: 3394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the current module and any tests that describe routing import behavior.
sed -n '1,220p' python/cuopt/cuopt/routing/__init__.py
printf '\n---\n'
rg -n "routing\.utils|routing\.utils_wrapper|__getattr__|__dir__|import \*" python/cuopt/tests python/cuopt/cuopt -g '!**/__pycache__/**'Repository: NVIDIA/cuopt
Length of output: 4626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' python/cuopt/cuopt/routing/__init__.py
printf '\n---\n'
rg -n "routing\.utils|routing\.utils_wrapper|__getattr__|__dir__|import \*" python/cuopt/tests python/cuopt/cuopt -g '!**/__pycache__/**'Repository: NVIDIA/cuopt
Length of output: 4626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- parent package hooks ---'
sed -n '1,120p' python/cuopt/cuopt/__init__.py
printf '\n%s\n' '--- routing test file ---'
sed -n '1,220p' python/cuopt/cuopt/tests/routing/test_lazy_imports.pyRepository: NVIDIA/cuopt
Length of output: 2269
Type and document the package hooks. __getattr__ and __dir__ are part of the cuopt.routing surface; add name: str -> Any / -> list[str] and a short docstring covering the lazy export behavior and AttributeError.
🤖 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/routing/__init__.py` around lines 27 - 36, Update the
package hooks __getattr__ and __dir__ with the requested type annotations:
__getattr__ should accept name: str and return Any, while __dir__ should return
list[str]. Add concise docstrings describing lazy export resolution and the
AttributeError raised for unknown names in __getattr__, and document the
directory listing behavior for __dir__.
Sources: Coding guidelines, Path instructions
CI Test Summary✅ All 22 test job(s) passed. |
6a04064 to
6a98a74
Compare
ramakrishnap-nv
changed the title
Importing cuopt.routing pulled in cuopt.routing.utils / utils_wrapper, whose generate_dataset had cudf.Series() as default argument values. Default arguments are evaluated at import time, and constructing an (even empty) cudf.Series requires a GPU, so `import cuopt.routing` failed with cudaErrorNoDevice on a GPU-less host -- e.g. a CPU-only client building and serializing a routing problem to submit to a remote solver. Move the empty-series construction from the signatures into the function bodies via None sentinels. `import cudf` itself does not touch the GPU, so the modules now import GPU-free; the helpers behave identically when called. Add __all__ to the routing package for explicit re-exports, and a subprocess test that imports the package with no visible GPU. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
6a98a74 to
ee1f26b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuopt/cuopt/routing/utils.py (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
Nonedefaults.The public docstring should state that
min_demand,max_demand,min_capacities, andmax_capacitiesacceptNone, and thatNoneis replaced with an emptycudf.Seriesduring execution. This distinguishes GPU-safe import from GPU-required dataset generation.As per path instructions, focus on meaningful public API documentation content rather than formatting.
🤖 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/routing/utils.py` around lines 20 - 23, Update the public docstring for the function exposing min_demand, max_demand, min_capacities, and max_capacities to document that each accepts None and that execution replaces None with an empty cudf.Series. Keep the documentation focused on the GPU-safe default and its runtime dataset-generation behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In `@python/cuopt/cuopt/routing/utils.py`:
- Around line 20-23: Update the public docstring for the function exposing
min_demand, max_demand, min_capacities, and max_capacities to document that each
accepts None and that execution replaces None with an empty cudf.Series. Keep
the documentation focused on the GPU-safe default and its runtime
dataset-generation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b95f64a9-7db1-4a86-b93a-683e18151b87
📒 Files selected for processing (4)
python/cuopt/cuopt/routing/__init__.pypython/cuopt/cuopt/routing/utils.pypython/cuopt/cuopt/routing/utils_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_gpu_free_import.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuopt/cuopt/routing/utils_wrapper.pyx
- python/cuopt/cuopt/routing/init.py
The dataset-generation and batch helpers (generate_dataset, DatasetDistribution, add_vehicle_constraints, ...) are undocumented, local-testing utilities that require a GPU. Re-exporting them from the cuopt.routing package made `import cuopt.routing` pull in utils / utils_wrapper and fail with cudaErrorNoDevice on a GPU-less host -- e.g. a CPU-only client building and serializing a routing problem for a remote solver. Stop re-exporting them so importing routing no longer touches that GPU machinery; they remain available via `cuopt.routing.utils` for the tests that use them. save_data_model_to_yaml is imported from utils at its call site for the same reason. Repoint the one test that used the re-exports to cuopt.routing.utils, and add a subprocess test that (with no visible GPU) imports cuopt.routing, asserts utils/utils_wrapper are not pulled in, builds a DataModel, and imports cuopt.routing.utils. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cuopt/tests/routing/test_gpu_free_import.py`:
- Around line 36-48: Update test_routing_utils_imports_without_a_gpu to invoke
utils.add_vehicle_constraints with its default arguments and assert the returned
fields, including num_vehicles and capacity, have the expected values. Keep
utils.generate_dataset as a callability/import-only check if invoking it
requires GPU support.
- Around line 14-34: Expand the public-surface assertions in
test_routing_imports_without_a_gpu to verify that cuopt.routing does not expose
DatasetDistribution, add_vehicle_constraints, create_pickup_delivery_data, or
update_routes_and_vehicles, alongside the existing generate_dataset check. Keep
the module-import and CPU-only model-building checks unchanged.
🪄 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: 51fb5745-c7c8-4a1c-a1e7-aafc4fe2fe19
📒 Files selected for processing (4)
python/cuopt/cuopt/routing/__init__.pypython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/routing/test_distance_engine.pypython/cuopt/cuopt/tests/routing/test_gpu_free_import.py
| def test_routing_utils_imports_without_a_gpu(): | ||
| """cuopt.routing.utils must import on a GPU-less host too. | ||
|
|
||
| Its helpers build empty cudf.Series objects; doing so in a default argument | ||
| would run at import time and fail with cudaErrorNoDevice where no GPU is | ||
| visible. This guards that the construction stays in the function bodies, so | ||
| tests importing utils can be collected without a GPU. | ||
| """ | ||
| _run_without_gpu( | ||
| "from cuopt.routing import utils\n" | ||
| "assert callable(utils.generate_dataset)\n" | ||
| "assert callable(utils.add_vehicle_constraints)\n" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise a safe helper instead of checking only callability.
These assertions prove only that the attributes exist. Call utils.add_vehicle_constraints with its default arguments and assert returned fields such as num_vehicles and capacity; retain generate_dataset as an import-only check if it requires a GPU.
🤖 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/tests/routing/test_gpu_free_import.py` around lines 36 -
48, Update test_routing_utils_imports_without_a_gpu to invoke
utils.add_vehicle_constraints with its default arguments and assert the returned
fields, including num_vehicles and capacity, have the expected values. Keep
utils.generate_dataset as a callability/import-only check if invoking it
requires GPU support.
Source: Path instructions
With the dataset/batch helpers no longer re-exported from cuopt.routing, importing the package never touches utils / utils_wrapper, so it is GPU-free without the None-sentinel change to generate_dataset. Revert utils.py and utils_wrapper.pyx to keep the diff focused on the namespace move, and drop the utils-imports-without-a-gpu test. The test that importing cuopt.routing does not require a GPU is kept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The GPU-free-import test only asserted generate_dataset was gone, so a regression re-exposing DatasetDistribution, add_vehicle_constraints, create_pickup_delivery_data, or update_routes_and_vehicles would still pass. Enumerate all five moved names and fail if any leaks back onto cuopt.routing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Thanks for the review. Addressed in ca9e016:
The remaining CodeRabbit comments were against earlier revisions of this PR and are now stale:
|
|
/merge |
|
|
The CI failures are not caused by this PR's changes. All three failing tests ( This regression was introduced in main and is not present in the PR 1549 run from today (same runner configs, all green). The two PRs have different main bases: PR 1549 ran on This PR can be unblocked once the main regression is identified and fixed. |
Tracked in #1592. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…ilds Also crashing with std::bad_array_new_length on all wheel runners in CI run 29775058848. Tracked in #1592. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Vendors the GPU-free-import fix (#1591) so a CPU-only client can import cuopt.routing and build a DataModel without a GPU -- required for the VRP gRPC client to run in a GPU-less environment. Dedupe on rebase once #1591 merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Conflict in python/cuopt/cuopt/routing/__init__.py: take main's cleanup (GPU-requiring utilities removed from public re-exports per #1591) while keeping the explanatory comment from this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
2 participants
Summary
Importing
cuopt.routingrequired a GPU: it pulled incuopt.routing.utils/utils_wrapper, whosegenerate_datasetusedcudf.Series()as default argument values. Default args are evaluated at import time, and constructing even an emptycudf.Seriesneeds a GPU — soimport cuopt.routingfailed withcudaErrorNoDeviceon a GPU-less host (e.g. a CPU-only client building/serializing a routing problem to submit to a remote solver).Fix at the source: move the empty-series construction out of the signatures into the function bodies via
Nonesentinels.import cudfitself doesn't touch the GPU, so the modules now import GPU-free and the helpers behave identically when called. Adds__all__for explicit re-exports and a subprocess test that imports the package with no visible GPU.Validated:
import cuopt.routing+ helpers +DataModelbuild all work underCUDA_VISIBLE_DEVICES=""; the routing suite passes on GPU.🤖 Generated with Claude Code