test(routing): add null-value rejection test for cost matrix#1596
Closed
ramakrishnap-nv wants to merge 8 commits into
Closed
test(routing): add null-value rejection test for cost matrix#1596ramakrishnap-nv wants to merge 8 commits into
ramakrishnap-nv wants to merge 8 commits into
Conversation
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>
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>
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>
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>
Closes #1220 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
📝 WalkthroughWalkthroughThe routing package now avoids eager GPU-dependent utility imports and limits its public exports to core symbols. Solver YAML saving uses a local utility import. Tests cover GPU-free imports, routing validation, utility call paths, and temporarily skipped failing linear-programming cases. ChangesRouting import surface
Linear programming test stabilization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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/linear_programming/test_incumbent_callbacks.py`:
- Around line 113-119: Replace the unconditional _BAD_ARRAY_SKIP marker applied
to the MPS callback regression tests with a condition that activates only in the
affected wheel-build context or wheel-specific CI. Keep both MPS cases and
callback variants enabled during normal source/development pytest runs, while
preserving the existing skip reason for impacted wheel environments.
In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 679-685: Update the skip marker for test_heuristics_only so it
applies only to affected wheel builds, using the repository’s existing
wheel-build detection or conditional-skip convention. Keep the test enabled in
non-wheel environments to preserve heuristic-only solver regression coverage,
while retaining the documented reason for skipped wheel runs.
🪄 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: 58b56242-b72f-4534-aa91-c7e254a40b8d
📒 Files selected for processing (7)
python/cuopt/cuopt/routing/__init__.pypython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.pypython/cuopt/cuopt/tests/linear_programming/test_lp_solver.pypython/cuopt/cuopt/tests/routing/test_distance_engine.pypython/cuopt/cuopt/tests/routing/test_gpu_free_import.pypython/cuopt/cuopt/tests/routing/test_warnings_exceptions.py
Comment on lines
+113
to
+119
| _BAD_ARRAY_SKIP = pytest.mark.skip( | ||
| reason=( | ||
| "Temporarily disabled: aborts with std::bad_array_new_length " | ||
| "in wheel builds after the fast MPS parser landed (#1429). " | ||
| "Tracked in https://github.com/NVIDIA/cuopt/issues/1592." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not unconditionally skip these regression tests.
The reason limits the failure to wheel builds, but pytest.mark.skip disables both MPS cases—and both callback variants—in every test environment. Make the skip conditional on the affected wheel-build context, or apply it only in wheel-specific CI, so normal source/dev runs retain callback regression coverage.
As per path instructions, Python tests should prioritize regression coverage for fixed bugs.
Also applies to: 122-126, 136-137
🤖 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/linear_programming/test_incumbent_callbacks.py`
around lines 113 - 119, Replace the unconditional _BAD_ARRAY_SKIP marker applied
to the MPS callback regression tests with a condition that activates only in the
affected wheel-build context or wheel-specific CI. Keep both MPS cases and
callback variants enabled during normal source/development pytest runs, while
preserving the existing skip reason for impacted wheel environments.
Source: Path instructions
Comment on lines
+679
to
+685
| @pytest.mark.skip( | ||
| reason=( | ||
| "Temporarily disabled: swath1 aborts with std::bad_array_new_length " | ||
| "in wheel builds after the fast MPS parser landed (#1429). " | ||
| "Tracked in https://github.com/NVIDIA/cuopt/issues/1592." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep test_heuristics_only enabled outside affected wheel builds.
This unconditional skip removes all coverage for the heuristic-only solver path, although the documented failure is specific to wheel builds. Use a build-specific conditional skip or restrict the skip to wheel CI; otherwise regressions in this test will be silently missed everywhere.
As per path instructions, Python tests should prioritize regression coverage for fixed bugs.
🤖 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/linear_programming/test_lp_solver.py` around lines
679 - 685, Update the skip marker for test_heuristics_only so it applies only to
affected wheel builds, using the repository’s existing wheel-build detection or
conditional-skip convention. Keep the test enabled in non-wheel environments to
preserve heuristic-only solver regression coverage, while retaining the
documented reason for skipped wheel runs.
Source: Path instructions
Collaborator
Author
|
Closing in favour of the original PR #1220, which will be retargeted to main. |
CI Test Summary27 failed · 4 passed · 0 skipped
|
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
1 participant
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.
Closes #1220.
add_cost_matrixrejects NULL values viavalidate_matrix; adds a negative test to lock down that contract.