routing: add distance breaks - #1196
Conversation
|
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:
📝 WalkthroughWalkthroughThis pull request adds cumulative-distance vehicle breaks with soft minimum targets, hard maximum limits, optional locations, multiple cycles, and configurable objective weighting. It integrates the feature across C++, Python, server APIs, solver state, serialization, tests, and documentation. ChangesDistance-breaks integration
Estimated code review effort: 4 (Complex) | ~60 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt_server/cuopt_server/utils/routing/optimization_data_model.py (1)
473-552:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror
vehicle_distance_breaksthroughupdate_fleet_data()too.This PR wires the new field through the create/set path only.
update_fleet_data()still has no parameter, validation call, or storage branch forvehicle_distance_breaks, so update callers cannot modify or clear distance breaks after initial creation.🤖 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/cuopt_server/utils/routing/optimization_data_model.py` around lines 473 - 552, update_fleet_data is missing the new vehicle_distance_breaks parameter and plumbing; add a vehicle_distance_breaks parameter to update_fleet_data, call get_none_for_empty_list(vehicle_distance_breaks) inside it, pass vehicle_distance_breaks through to the validate_fleet_data(...) call (match the updating=True usage used in update path), and ensure the validated value is stored in the same internal field that set_fleet_data uses (the model attribute used to persist distance breaks). This mirrors the handling in set_fleet_data and allows callers to modify/clear distance breaks on updates.
🧹 Nitpick comments (2)
python/cuopt_server/cuopt_server/utils/routing/data_definition.py (1)
134-168: ⚡ Quick winDocument float precision accurately and consider adding a Pydantic validator for range constraints.
Type documentation mismatch: The Field descriptions state
dtype: float32formax_rangeandmin_range, but the Pythonfloattype is typically 64-bit. Either update the documentation tofloat64or explicitly usenp.float32if 32-bit precision is required for downstream processing.Missing constraint validation: The
min_rangedescription states"0 <= min_range < max_range", but no Pydantic validator enforces this relationship. Whilevalidation_fleet_data.pymay validate this (per the PR summary), consider adding a@root_validatorhere for defense-in-depth:🛡️ Suggested Pydantic validator
`@root_validator`(skip_on_failure=True) def validate_range_bounds(cls, values): min_range = values.get('min_range', 0.0) max_range = values.get('max_range') if max_range is not None and min_range >= max_range: raise ValueError(f"min_range ({min_range}) must be < max_range ({max_range})") return values🤖 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/cuopt_server/utils/routing/data_definition.py` around lines 134 - 168, The Field descriptions for max_range and min_range incorrectly state "dtype: float32" while the attributes use Python float (64-bit); update the docstrings to reflect float64 or switch the type to np.float32 if 32-bit is required (refer to max_range and min_range Field definitions). Also add a Pydantic class-level validator (use `@root_validator`(skip_on_failure=True)) in the same data model to enforce 0 <= min_range < max_range and raise a clear ValueError if the constraint fails (refer to min_range, max_range and the class where Field is defined).cpp/tests/routing/unit_tests/distance_breaks.cu (1)
275-368: ⚡ Quick winAdd a regression for the lower-bound side of the distance window.
These tests pin
d_maxenforcement and multi-cycle insertion, but I don't see a case that asserts the newmin_rangebehavior when a charger is reached before the lower bound. That is the part of the feature that differs most from time-based breaks, so it should have an explicit regression.As per coding guidelines,
.github/.coderabbit_review_guide.md: "require edge-case coverage for empty/invalid windows, boundary tolerances, multi-cycle window generation, and infeasible scenarios".🤖 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 `@cpp/tests/routing/unit_tests/distance_breaks.cu` around lines 275 - 368, Add a regression test that verifies the distance-break lower bound (min_range) is enforced: create a new TEST (e.g., distance_breaks, break_distance_window_min_enforced) using a small cost matrix and order/charging_station setup similar to break_distance_window_enforced but call data_model.add_distance_break with nonzero min_range (e.g., add_distance_break(0, min_range, max_range, ...)). Solve and iterate h.locations/h.node_types to compute cumulative distance like in the existing test and assert any found BREAK satisfies cumulative >= min_range and cumulative <= max_range, and assert a break was found; also include a case where the nearest charger would occur before min_range so the solver must place the break later to validate the lower-bound behavior.
🤖 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 `@cpp/src/routing/data_model_view.cu`:
- Around line 36-42: thrust::unique only removes consecutive duplicates so the
current check can miss non-adjacent duplicates; fix by sorting the copied device
vector before calling unique: after creating tmp and copying locations
(rmm::device_uvector<i_t> tmp(...) and raft::copy(...)), call thrust::sort with
the same thrust policy/stream (handle->get_thrust_policy(), tmp.begin(),
tmp.end()), then call thrust::unique and recompute unique_items as end -
tmp.begin(); keep the cuopt::cuopt_expects(n == unique_items, ...) assertion but
run it after the sort+unique sequence.
- Around line 25-43: In validate_break_locations, explicitly reject negative
counts and null pointer inputs before any device-side work: add checks so that
if n < 0 you fail via cuopt::cuopt_expects with
cuopt::error_type_t::ValidationError, and if n > 0 && locations == nullptr you
likewise fail with a ValidationError; perform these checks before calling
cuopt::routing::detail::check_min_max_values, before allocating
rmm::device_uvector, and before any raft::device_span construction in callers
(e.g., places building raft::device_span<const i_t>(break_locations,
num_break_locations)).
In `@cpp/src/routing/local_search/delivery_insertion.cuh`:
- Around line 106-108: The early returns in the delivery insertion scan abort on
the first distance-window miss; change them to continue scanning (or otherwise
only break on monotone/upper-bound-only failures) so a candidate that is
initially too-early for distance_min can still become feasible later as
cumulative distance grows. Concretely, replace the return-after-failure in calls
to node_t<i_t,f_t,REQUEST>::window_forward_feasible and
node_t<i_t,f_t,REQUEST>::window_backward_feasible (and the analogous checks at
the other mentioned sites) with logic that continues the loop unless the
feasibility failure is provably monotone (e.g., an upper-bound violation),
ensuring the scan does not prematurely stop. Ensure you update all occurrences
noted (the forward/backward checks around the delivery insertion scan) so they
follow the same continue-or-break-on-monotone rule.
In `@cpp/src/routing/node/distance_node.cuh`:
- Around line 96-105: The combine() function currently only sums window
mismatches and ignores vehicle_info.max_cost; update combine to also compute the
combined raw travel distance for the two fragments (combined_raw = prev raw
distance + distance_between + next raw distance—use the appropriate raw distance
fields on distance_node_t), then if vehicle_info.max_cost is finite add max(0.,
combined_raw - vehicle_info.max_cost) to the returned excess (i.e., return
prev.excess_forward + next.excess_backward + upper_excess + lower_excess +
max_distance_excess) so joins that violate max_cost are penalized/treated as
infeasible; keep using symbols combine, vehicle_info.max_cost, distance_between,
prev and next to locate and change the code.
In `@cpp/src/routing/problem/problem.cu`:
- Around line 689-690: The code is populating
node_distance_min_h/node_distance_max_h for every non-uniform break rather than
only for distance-based breaks, which makes special_nodes.distance_min non-empty
and incorrectly flips has_distance_window in populate_dimensions_info(); fix by
only appending to node_distance_min_h and node_distance_max_h when
vehicle_break.is_distance_based_ is true (e.g., wrap the push_back calls in an
if (vehicle_break.is_distance_based_) block), and/or introduce an explicit
per-node distance-based flag vector (e.g., node_is_distance_based_h) that is
populated from add_vehicle_break(...) and used by populate_dimensions_info()
instead of relying on buffer non-emptiness so time-only breaks remain
unmodified.
- Around line 654-670: The overlap validation only compares the current
time-based break to the immediately previous break and skips if that previous
break is distance-based; update the check in the block using
non_uniform_breaks.at(v), break_earliest_h, break_latest_h, is_distance_based_,
dim and v so it searches backward for the most recent prior break where
is_distance_based_ == false and then assert break_earliest_h[v][dim] >=
break_latest_h[v][prev_time_dim] via cuopt_expects (instead of comparing only
dim-1); ensure this logic runs only when the current break is time-based and
preserves the existing error message "breaks should not be overlapping!".
In `@cpp/src/routing/route/distance_route.cuh`:
- Around line 180-196: compute_cost currently overwrites inf_cost[dim_t::DIST]
in the has_distance_window branch with only window-related excess, dropping
vehicle_max_cost infeasibility; change the has_distance_window logic in
compute_cost so inf_cost[dim_t::DIST] accumulates both the window excess
(excess_forward[n_nodes_route] + upper_bound + lower_bound) and any vehicle max
cost excess (max(0., distance_forward[n_nodes_route] - vehicle_info.max_cost)),
e.g. compute both contributions and set inf_cost[dim_t::DIST] to their sum or
the max as appropriate so vehicle_info.max_cost is enforced even when
dim_info.has_distance_window is true.
In `@cpp/src/routing/util_kernels/set_nodes_data.cuh`:
- Around line 61-62: The header set_nodes_data.cuh is using
std::numeric_limits<f_t>::max() (see the f_t vehicle_max_cost assignment) but
doesn't include <limits>, making the header rely on transitive includes; fix it
by adding a direct `#include` <limits> to set_nodes_data.cuh so the use of
std::numeric_limits is self-contained and order-independent.
In `@cpp/tests/routing/routing_test.cuh`:
- Around line 1038-1063: Add an explicit assertion that distance break nodes are
actually present in the solution produced by test_cvrptw_distance_breaks: after
constructing host_assignment_t h_routing_solution(routing_solution) and before
calling check_distance_break_windows, scan the routes in h_routing_solution and
assert that at least one node with type BREAK (or, stronger, at least one BREAK
per vehicle that had add_distance_break called) is present; this ensures
add_distance_break calls actually resulted in emitted BREAK nodes rather than
silently being dropped.
In `@docs/cuopt/source/cuopt-python/routing/routing-examples.rst`:
- Around line 73-80: Update the documentation text around
DataModel.add_distance_break to correctly describe the generated windows when
n_cycles is nonzero: state that cycle k yields the window [k * max_range +
min_range, (k + 1) * max_range] (so each window width is max_range - min_range,
not always max_range) and explicitly note that visits before the lower bound
(the min_range edge) are penalized rather than strictly forbidden; reference the
parameters n_cycles, max_range and min_range in the description to make the
behavior precise and consistent with the public API.
In `@docs/cuopt/source/routing-features.rst`:
- Around line 66-73: Update the add_distance_break docs to accurately describe
cycle windows and lower-bound behavior: state that each cycle k=0..n_cycles-1
defines a required stop window [k*max_range + min_range, (k+1)*max_range] whose
width is (max_range - min_range) when min_range>0, clarify whether endpoints are
inclusive, and explicitly note that min_range is not a hard feasibility cutoff
but a preferred lower bound (early charging is allowed by the solver and will be
penalized according to the implementation rather than strictly forbidden); keep
references to add_distance_break, n_cycles, max_range, min_range (distance_min),
and charging_stations so readers can locate the behavior in the API.
In `@python/cuopt_server/cuopt_server/utils/routing/solver.py`:
- Around line 229-243: The vehicle_distance_breaks block currently passes raw
charging_stations into data_model.add_distance_break; for waypoint-graph
requests you must remap those station IDs through optimization_data.locations
(use the same locations.loc[...] pattern used for
vehicle_locations/vehicle_break_locations) to convert to the correct index
positions and build a cudf.Series of the remapped indices (or None) before
calling add_distance_break; additionally ensure any station IDs present only in
vehicle_distance_breaks are appended to optimization_data.locations (so they
exist when the cost matrix is generated) and add validation to detect
index-mapping mismatches or missing station entries at the server/library
boundary.
In `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 395-428: add_distance_break is missing type hints and complete
docstring details; update its signature to include Python typing (e.g.,
vehicle_ids: Union[int, Sequence[int]], max_range: float, charge_duration: int,
charging_stations: Optional[cudf.Series]=None, min_range: float=0.0, n_cycles:
Optional[int]=None) and document returns and errors: state that max_range and
min_range are in the primary cost-matrix units, the method mutates the model in
place and returns None, and list ValueError conditions to raise (e.g., negative
max_range or min_range, min_range > max_range, negative or zero charge_duration,
n_cycles if provided must be > 0, and invalid/empty vehicle_ids); also mention
any numeric tolerances/semantics for cycles and cumulative-distance windows;
keep the function name add_distance_break references so reviewers can locate the
change.
- Around line 431-433: The handling of vehicle_ids only checks for Python int
and should accept NumPy integer scalars like n_cycles does; update the
conditional that normalizes vehicle_ids (the block using "if
isinstance(vehicle_ids, int): vehicle_ids = [vehicle_ids]") to check
isinstance(vehicle_ids, (int, np.integer)) so np.int32/np.int64 scalars are
converted to a single-item list before the for vid in vehicle_ids loop; mirror
the same pattern used for n_cycles and ensure numpy is imported or referenced as
np in the module.
---
Outside diff comments:
In `@python/cuopt_server/cuopt_server/utils/routing/optimization_data_model.py`:
- Around line 473-552: update_fleet_data is missing the new
vehicle_distance_breaks parameter and plumbing; add a vehicle_distance_breaks
parameter to update_fleet_data, call
get_none_for_empty_list(vehicle_distance_breaks) inside it, pass
vehicle_distance_breaks through to the validate_fleet_data(...) call (match the
updating=True usage used in update path), and ensure the validated value is
stored in the same internal field that set_fleet_data uses (the model attribute
used to persist distance breaks). This mirrors the handling in set_fleet_data
and allows callers to modify/clear distance breaks on updates.
---
Nitpick comments:
In `@cpp/tests/routing/unit_tests/distance_breaks.cu`:
- Around line 275-368: Add a regression test that verifies the distance-break
lower bound (min_range) is enforced: create a new TEST (e.g., distance_breaks,
break_distance_window_min_enforced) using a small cost matrix and
order/charging_station setup similar to break_distance_window_enforced but call
data_model.add_distance_break with nonzero min_range (e.g.,
add_distance_break(0, min_range, max_range, ...)). Solve and iterate
h.locations/h.node_types to compute cumulative distance like in the existing
test and assert any found BREAK satisfies cumulative >= min_range and cumulative
<= max_range, and assert a break was found; also include a case where the
nearest charger would occur before min_range so the solver must place the break
later to validate the lower-bound behavior.
In `@python/cuopt_server/cuopt_server/utils/routing/data_definition.py`:
- Around line 134-168: The Field descriptions for max_range and min_range
incorrectly state "dtype: float32" while the attributes use Python float
(64-bit); update the docstrings to reflect float64 or switch the type to
np.float32 if 32-bit is required (refer to max_range and min_range Field
definitions). Also add a Pydantic class-level validator (use
`@root_validator`(skip_on_failure=True)) in the same data model to enforce 0 <=
min_range < max_range and raise a clear ValueError if the constraint fails
(refer to min_range, max_range and the class where Field is defined).
🪄 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: 14c568cf-bc52-4b28-af4b-5bdfafbcb7f4
📒 Files selected for processing (39)
cpp/include/cuopt/routing/data_model_view.hppcpp/include/cuopt/routing/routing_structures.hppcpp/src/routing/data_model_view.cucpp/src/routing/dimensions.cuhcpp/src/routing/ges/compute_delivery_insertions.cuhcpp/src/routing/ges/lexicographic_search/lexicographic_search.cucpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/src/routing/local_search/compute_ejections.cuhcpp/src/routing/local_search/compute_insertions.cucpp/src/routing/local_search/delivery_insertion.cuhcpp/src/routing/local_search/permutation_helper.cuhcpp/src/routing/local_search/sliding_window.cucpp/src/routing/node/break_node.cuhcpp/src/routing/node/distance_node.cuhcpp/src/routing/node/node.cuhcpp/src/routing/problem/problem.cucpp/src/routing/problem/special_nodes.cuhcpp/src/routing/route/break_route.cuhcpp/src/routing/route/distance_route.cuhcpp/src/routing/solution/solution.cuhcpp/src/routing/util_kernels/set_nodes_data.cuhcpp/tests/routing/CMakeLists.txtcpp/tests/routing/level1/l1_routing_test.cucpp/tests/routing/routing_test.cuhcpp/tests/routing/unit_tests/distance_breaks.cudocs/cuopt/source/cuopt-python/routing/examples/distance_break_example.pydocs/cuopt/source/cuopt-python/routing/routing-examples.rstdocs/cuopt/source/routing-features.rstpython/cuopt/cuopt/routing/vehicle_routing.pxdpython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_distance_breaks.pypython/cuopt_server/cuopt_server/tests/utils/utils.pypython/cuopt_server/cuopt_server/utils/routing/data_definition.pypython/cuopt_server/cuopt_server/utils/routing/optimization_data_model.pypython/cuopt_server/cuopt_server/utils/routing/solver.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.pypython/cuopt_server/cuopt_server/utils/solver.py
| if (!node_t<i_t, f_t, REQUEST>::window_forward_feasible( | ||
| delivery_node, route.vehicle_info(), weights, excess_limit)) { | ||
| return; |
There was a problem hiding this comment.
Don't abort the insertion scan on the first distance-window miss.
These returns were safe for time-window pruning, but they are too aggressive once window_forward_feasible / window_backward_feasible includes distance breaks. A candidate can be invalid only because it is too early for distance_min, then become valid a few positions later as cumulative distance grows. Returning here drops the rest of the scan and can miss valid or lower-cost insertions.
Please keep scanning after a lower-bound/window-slack failure here, or only early-exit on cases that are still monotone (for example, proven upper-bound violations).
Also applies to: 141-145, 201-203, 235-239
🤖 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 `@cpp/src/routing/local_search/delivery_insertion.cuh` around lines 106 - 108,
The early returns in the delivery insertion scan abort on the first
distance-window miss; change them to continue scanning (or otherwise only break
on monotone/upper-bound-only failures) so a candidate that is initially
too-early for distance_min can still become feasible later as cumulative
distance grows. Concretely, replace the return-after-failure in calls to
node_t<i_t,f_t,REQUEST>::window_forward_feasible and
node_t<i_t,f_t,REQUEST>::window_backward_feasible (and the analogous checks at
the other mentioned sites) with logic that continues the loop unless the
feasibility failure is provably monotone (e.g., an upper-bound violation),
ensuring the scan does not prematurely stop. Ensure you update all occurrences
noted (the forward/backward checks around the delivery insertion scan) so they
follow the same continue-or-break-on-monotone rule.
rg20
added
feature request
|
@np96 Thanks for the improvements. It will take a bit of time for me to review all the changes :). Did this implementation improve the accuracy/quality of the results for your use cases? |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
python/cuopt/cuopt/tests/routing/test_distance_breaks.py (1)
338-376: ⚡ Quick winAdd one lower-bound-violation / infeasible distance-window case.
All solver scenarios in this file are feasible. The new feature’s distinctive path is the lack of a distance-wait analogue, so an early break should violate
min_rangeand flow through infeasibility cost. Without one case that forces that behavior, regressions in the new penalty plumbing can slip past the Python integration suite.As per coding guidelines, "Flag missing coverage for edge cases (empty, infeasible, unbounded, degenerate) when adding new code paths".
Also applies to: 379-455
🤖 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 `@cpp/tests/routing/routing_test.cuh`:
- Around line 595-629: The helper check_distance_break_windows currently treats
min_range as a hard feasibility bound and fails when a BREAK cumulative distance
is below min_range; update the function check_distance_break_windows to remove
the lower-bound assertion (the ASSERT_GE that checks cumulative vs min_range)
while keeping the upper-bound ASSERT_LE against max_range and still incrementing
break_count for BREAK nodes (node_type_t::BREAK). Ensure no new hard-feasibility
check is introduced for min_range here—lower-bound behavior should be validated
via infeasibility/objective tests elsewhere.
In `@python/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py`:
- Around line 214-219: The validator currently only checks n_cycles > 0 for each
vehicle_distance_breaks entry; add an upper bound to prevent explosion by either
(a) capping entry.n_cycles to a constant MAX_N_CYCLES (e.g. define MAX_N_CYCLES
near the top of validation_fleet_data.py) and rejecting values > MAX_N_CYCLES
with the same False/"vehicle_distance_breaks: n_cycles must be <= MAX_N_CYCLES"
style, or (b) compute the total generated breaks across all entries (sum of
entry.n_cycles or min(entry.n_cycles, MAX_N_CYCLES) per entry) and reject the
request if total > MAX_TOTAL_BREAKS, returning a clear error like
"vehicle_distance_breaks: total generated breaks exceeds limit"; update the
validation branch that currently references entry.n_cycles and n_cycles to
enforce the chosen cap and surface the appropriate error message.
In `@python/cuopt/cuopt/tests/routing/test_distance_breaks.py`:
- Around line 305-309: The test only checks properties of "Break" rows if they
exist, allowing regressions where no breaks or breaks for wrong vehicles still
pass; update the test that calls sol.get_route() and inspects routes to
explicitly assert the expected set/count of vehicles that have breaks (e.g.,
compute the set of vehicle identifiers from routes where type == "Break" and
compare to the expected vehicle set), and add a found_break guard (fail/assert
when no "Break" rows are present) before validating break locations against
break_loc_set; apply the same pattern to the other affected blocks (around lines
327-335, 444-455, 467-472) so each case verifies both presence and correctness
of break-bearing vehicles, not just properties of observed breaks.
- Around line 142-147: The test test_distance_break_invalid_vehicle_id currently
parametrizes invalid vehicle ids [-1, 4, 100] but omits the exact upper-bound
invalid case (fleet_size == 3); update the pytest.mark.parametrize for vid in
that test to include 3 so the list becomes [-1, 3, 4, 100] (or add 3 alongside
the existing values) to catch off-by-one acceptance issues when calling
model.add_distance_break(vid, ...).
🪄 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: 02c01d55-1910-4946-9805-00ddd80111b6
📒 Files selected for processing (19)
cpp/include/cuopt/routing/data_model_view.hppcpp/src/routing/data_model_view.cucpp/src/routing/node/distance_node.cuhcpp/src/routing/problem/problem.cucpp/src/routing/problem/special_nodes.cuhcpp/src/routing/route/distance_route.cuhcpp/src/routing/util_kernels/set_nodes_data.cuhcpp/tests/routing/level1/l1_routing_test.cucpp/tests/routing/routing_test.cuhcpp/tests/routing/unit_tests/distance_breaks.cudocs/cuopt/source/cuopt-python/routing/examples/distance_break_example.pydocs/cuopt/source/cuopt-python/routing/routing-examples.rstdocs/cuopt/source/routing-features.rstpython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/routing/test_distance_breaks.pypython/cuopt_server/cuopt_server/utils/routing/data_definition.pypython/cuopt_server/cuopt_server/utils/routing/optimization_data_model.pypython/cuopt_server/cuopt_server/utils/routing/solver.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
✅ Files skipped from review due to trivial changes (1)
- docs/cuopt/source/cuopt-python/routing/examples/distance_break_example.py
🚧 Files skipped from review as they are similar to previous changes (9)
- cpp/include/cuopt/routing/data_model_view.hpp
- docs/cuopt/source/routing-features.rst
- cpp/src/routing/util_kernels/set_nodes_data.cuh
- cpp/src/routing/problem/problem.cu
- cpp/src/routing/problem/special_nodes.cuh
- docs/cuopt/source/cuopt-python/routing/routing-examples.rst
- python/cuopt_server/cuopt_server/utils/routing/data_definition.py
- cpp/src/routing/node/distance_node.cuh
- cpp/src/routing/data_model_view.cu
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/routing/vehicle_routing.py`:
- Around line 455-459: Validate vehicle_ids immediately after the scalar-to-list
conversion: if vehicle_ids is None or not a supported scalar/iterable type
(accept int/np.integer or any iterable like list/tuple/np.ndarray/cudf.Series),
raise a clear ValueError indicating "invalid/empty vehicle_ids" (or similar text
matching the docstring) when the sequence is empty; keep the existing branch
that converts an int/np.integer to [int(vehicle_ids)], then check that the
resulting container is non-empty before any iteration and raise the actionable
ValueError for unsupported types or empty containers so callers fail fast.
- Around line 415-419: The docstring in vehicle_routing.py currently claims
distance-window endpoints are strictly infeasible; update that paragraph to
state that distance-window violations are handled by applying an infeasibility
cost (penalized) rather than being impossible, while still noting there is no
"wait" analogue in the distance dimension so the cumulative distance can't be
delayed; adjust the wording to match the PR behavior. Also ensure the public API
docstring for the related class/function includes pydocstyle-compliant sections
(params/returns/raises) as required by the project guidelines so the docstring
is complete and unambiguous.
🪄 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: 38b8078d-df86-49cc-aa0d-4467b87af689
📒 Files selected for processing (9)
cpp/src/routing/node/distance_node.cuhcpp/src/routing/route/distance_route.cuhcpp/src/routing/util_kernels/set_nodes_data.cuhcpp/tests/routing/unit_tests/distance_breaks.cudocs/cuopt/source/cuopt-python/routing/routing-examples.rstdocs/cuopt/source/routing-features.rstpython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/routing/test_distance_breaks.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
✅ Files skipped from review due to trivial changes (2)
- docs/cuopt/source/routing-features.rst
- docs/cuopt/source/cuopt-python/routing/routing-examples.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
- python/cuopt/cuopt/tests/routing/test_distance_breaks.py
- cpp/src/routing/route/distance_route.cuh
Hi @rg20! Looks better and cleaner than forbidding local search moves. Though practically it currently has some limitations due to this. Ideally I'd want to model this way: Everything else LGTM. Hope to fix this and I am very happy with this feature and approach. Thanks 👍 |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
rg20
left a comment
rg20
left a comment
There was a problem hiding this comment.
Thanks for the PR. You do not need to check for distance dimension feasibility everywhere. We do that for time dimension just to rule out a lot of moves. For distance breaks, this filteration does not help as it is mostly needed only for break nodes and also only when distance breaks are used. This is a very special scenario.
| node_stack.s_route.vehicle_info()) && | ||
| node_stack.delivery_node.distance_dim.forward_feasible( | ||
| node_stack.s_route.vehicle_info()); | ||
| node_t<i_t, f_t, REQUEST>::window_forward_feasible(node_stack.delivery_node, |
| "Backward forward mismatch!"); | ||
| "Time backward/forward mismatch!"); | ||
| cuopt_assert(!(orginal_route.dimensions_info().has_dimension(dim_t::DIST) && | ||
| orginal_route.dimensions_info().distance_dim.has_distance_window) || |
There was a problem hiding this comment.
| orginal_route.dimensions_info().distance_dim.has_distance_window) || | |
| orginal_route.dimensions_info().distance_dim.has_constraints()) || |
distance can have max_distance constraint as well
| return max(0., total_distance - vehicle_info.max_cost); | ||
| double arrival_window_f = prev.distance_window_forward + distance_between; | ||
| double upper_excess = max(0., arrival_window_f - next.distance_window_backward); | ||
| double lower_excess = max(0., next.distance_window_backward_min - arrival_window_f); |
There was a problem hiding this comment.
I don't think we need lower_excess. Check the time dimension for how we do time combine
There was a problem hiding this comment.
Hi there.
I have read HY paper, so far I understand they don't treat early arrival as infeasible.
In case of time windows a vehcile may wait until time window opens. In distance case it is undesirable to arrive to early. If you ignore lower_excess, the vehicle may end up arriving at 20 KM when you want it to visit dist breaks in [60..80] km range.
I removed lower excess but in my opinion it is useful if you want to consider arriving to early as violating constraints.
There was a problem hiding this comment.
I think that should be okay, since the final objective is to minimize the distance, the solver will avoid such scenarios.
If we make this as a hard constraint, we may not find feasible solutions for some trivial cases.
|
Moving this out of 26.06 scope as this is in review process still. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
1 similar comment
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
|
Hi @rg20, thanks for review. I considered your comments. Probably I am wrong somewhere, would be happy if you point out an error in my reasoning. |
Signed-off-by: Nikolai Poperechnyi <n.poperechnyi@gmail.com>
7b6e43d to
ce2e403
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
cpp/tests/routing/unit_tests/distance_breaks.cu (1)
250-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
has_distance_break_costwithouthas_distance_window.Every node-level test sets
has_distance_windowandhas_distance_break_costtogether, here and at Lines 100-101 and Lines 132-133. No test exercises the break-cost flag on its own.That combination is the one that reads an unallocated buffer in
cpp/src/routing/route/distance_route.cuh(compute_cost, Line 207). A test that sets onlyhas_distance_break_costwould pin the intended behavior, whether that is "the combination is impossible" or "the combination must produce zero cost".🤖 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 `@cpp/tests/routing/unit_tests/distance_breaks.cu` around lines 250 - 253, Add a node-level test covering cost_dimension_info_t with has_distance_break_cost enabled and has_distance_window disabled, alongside the existing tests near the referenced flag combinations. Assert the intended compute_cost behavior for this configuration—either reject the impossible combination or return zero cost—and ensure the test exercises distance_route.cuh’s compute_cost path without requiring a distance-window buffer.Source: Path instructions
python/cuopt_server/cuopt_server/tests/test_server.py (1)
887-890: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
pytest.mark.parametrizeinstead of an in-test loop.The two scenarios run in one test function. If the first iteration fails, the second never runs, and the failure output does not show which iteration failed.
Parametrizing gives two independent test cases and self-describing failure output.
♻️ Proposed change
+@pytest.mark.parametrize( + "objectives,expected_cost", + [(None, 11.0), ({"distance_break_cost": 0}, 3.0)], +) `@pytest.mark.usefixtures`("cuoptproc") -def test_distance_break_cost_default(): +def test_distance_break_cost_default(objectives, expected_cost): cost_matrix = {0: [[0, 1, 1], [1, 0, 1], [1, 1, 0]]}Then remove the
forstatement and unindent its body.🤖 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/cuopt_server/tests/test_server.py` around lines 887 - 890, Replace the in-test objectives/expected_cost loop in the affected server test with pytest.mark.parametrize, naming both parameters and supplying the existing two scenario tuples. Remove the for statement and unindent its body so each scenario executes as an independent, self-describing test case.cpp/src/routing/node/distance_node.cuh (1)
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
1e18unconstrained sentinel.
1e18is the "no window" sentinel fordistance_window_backwardandwindow_end. The same literal is repeated incpp/tests/routing/unit_tests/distance_breaks.cu(inmake_routecall sites and the header comment) and is implied by the buffer initialization incpp/src/routing/route/distance_route.cuh. If the sentinel ever changes, the sites can diverge silently.Define one
constexpr doublein this header and reference it everywhere.♻️ Proposed change
namespace detail { +// Sentinel for an unconstrained distance-window upper bound. +constexpr double unconstrained_distance = 1e18; + // Distance dimension. Tracks the cumulative route distance (for vehicle.max_cost) and, when- double distance_window_backward = 1e18; + double distance_window_backward = unconstrained_distance; double window_start = 0.0; - double window_end = 1e18; + double window_end = unconstrained_distance;🤖 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 `@cpp/src/routing/node/distance_node.cuh` around lines 36 - 45, Define a shared constexpr double in distance_node.cuh for the unconstrained distance-window sentinel, and replace the 1e18 initializers for distance_window_backward and window_end with it. Update the distance_breaks.cu make_route call sites and header comment, plus the corresponding initialization in distance_route.cuh, to reference the same named constant instead of duplicating the literal.
🤖 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 `@cpp/src/routing/route/distance_route.cuh`:
- Around line 201-216: Update distance_route.cuh in compute_cost so
distance_break_cost_forward is read only when both has_distance_window and
has_distance_break_cost are enabled, matching the allocation conditions. Add a
node-level test in cpp/tests/routing/unit_tests/distance_breaks.cu around lines
250-253 with has_distance_break_cost true and has_distance_window false to
verify this combination does not read the unallocated buffer.
In `@cpp/tests/routing/unit_tests/distance_breaks.cu`:
- Around line 295-307: Replace the namespace-scope mutable std::vector<float>
variables cost_matrix_3x3 and cost_matrix_5x5 with constexpr std::array
constants, adding the required array header include. Preserve their existing
dimensions and values so the cuopt::device_copy call sites continue accepting
them as contiguous ranges.
In `@python/cuopt_server/cuopt_server/utils/routing/data_definition.py`:
- Around line 134-191: Expand the VehicleDistanceBreak class docstring to
document all constructor fields, their validation constraints, and the
validation errors raised for invalid values. In
docs/cuopt/source/cuopt-python/routing/examples/distance_break_example.py lines
28-33, add complete type hints and substantive docstrings covering parameters,
returns, and raises for build_cost_matrix and main, or rename non-public helpers
with a leading underscore.
In `@python/cuopt/cuopt/routing/_serialize.py`:
- Around line 149-173: Update _distance_break to serialize each distance break
using the server restore schema: flat entries with min_range, max_range, and
n_cycles instead of distance_min, distance_max, and duration. Preserve the
existing vehicle grouping while ensuring appended breaks retain their input
cycle order.
In `@python/cuopt/cuopt/tests/routing/test_distance_breaks.py`:
- Around line 195-201: Extend test_invalid_n_cycles_wrong_type to include True
and ensure it raises ValueError. In DataModel.add_distance_break, explicitly
reject boolean n_cycles before the integer validation so True and False cannot
be treated as integers or passed to range().
---
Nitpick comments:
In `@cpp/src/routing/node/distance_node.cuh`:
- Around line 36-45: Define a shared constexpr double in distance_node.cuh for
the unconstrained distance-window sentinel, and replace the 1e18 initializers
for distance_window_backward and window_end with it. Update the
distance_breaks.cu make_route call sites and header comment, plus the
corresponding initialization in distance_route.cuh, to reference the same named
constant instead of duplicating the literal.
In `@cpp/tests/routing/unit_tests/distance_breaks.cu`:
- Around line 250-253: Add a node-level test covering cost_dimension_info_t with
has_distance_break_cost enabled and has_distance_window disabled, alongside the
existing tests near the referenced flag combinations. Assert the intended
compute_cost behavior for this configuration—either reject the impossible
combination or return zero cost—and ensure the test exercises
distance_route.cuh’s compute_cost path without requiring a distance-window
buffer.
In `@python/cuopt_server/cuopt_server/tests/test_server.py`:
- Around line 887-890: Replace the in-test objectives/expected_cost loop in the
affected server test with pytest.mark.parametrize, naming both parameters and
supplying the existing two scenario tuples. Remove the for statement and
unindent its body so each scenario executes as an independent, self-describing
test case.
🪄 Autofix
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: 8ac0a0dd-50ff-4a35-91c2-f5c2cb60f554
📒 Files selected for processing (38)
cpp/include/cuopt/routing/data_model_view.hppcpp/include/cuopt/routing/routing_structures.hppcpp/src/routing/data_model_view.cucpp/src/routing/dimensions.cuhcpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/src/routing/node/distance_node.cuhcpp/src/routing/node/node.cuhcpp/src/routing/problem/problem.cucpp/src/routing/problem/special_nodes.cuhcpp/src/routing/route/distance_route.cuhcpp/src/routing/solution/solution.cuhcpp/src/routing/util_kernels/set_nodes_data.cuhcpp/tests/routing/CMakeLists.txtcpp/tests/routing/level1/l1_routing_test.cucpp/tests/routing/routing_test.cuhcpp/tests/routing/unit_tests/distance_breaks.cudocs/cuopt/source/cuopt-python/routing/examples/distance_break_example.pydocs/cuopt/source/cuopt-python/routing/routing-examples.rstdocs/cuopt/source/routing-features.rstpython/cuopt/cuopt/routing/_deferred.pypython/cuopt/cuopt/routing/_serialize.pypython/cuopt/cuopt/routing/structure/routing_utilities.pxdpython/cuopt/cuopt/routing/vehicle_routing.pxdpython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_data_model.pypython/cuopt/cuopt/tests/routing/test_distance_breaks.pypython/cuopt/cuopt/tests/routing/test_serialize.pypython/cuopt/cuopt/tests/routing/test_warnings_exceptions.pypython/cuopt_server/cuopt_server/tests/test_server.pypython/cuopt_server/cuopt_server/tests/test_set_solver_config.pypython/cuopt_server/cuopt_server/tests/utils/utils.pypython/cuopt_server/cuopt_server/utils/routing/data_definition.pypython/cuopt_server/cuopt_server/utils/routing/optimization_data_model.pypython/cuopt_server/cuopt_server/utils/routing/solver.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.pypython/cuopt_server/cuopt_server/utils/solver.py
🚧 Files skipped from review as they are similar to previous changes (15)
- python/cuopt_server/cuopt_server/tests/utils/utils.py
- cpp/src/routing/ges/squeeze.cuh
- cpp/src/routing/local_search/breaks_insertion.cu
- cpp/src/routing/solution/solution.cuh
- python/cuopt_server/cuopt_server/utils/solver.py
- cpp/src/routing/util_kernels/set_nodes_data.cuh
- cpp/tests/routing/CMakeLists.txt
- python/cuopt_server/cuopt_server/utils/routing/solver.py
- cpp/src/routing/dimensions.cuh
- python/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
- python/cuopt/cuopt/routing/vehicle_routing_wrapper.pyx
- cpp/src/routing/problem/special_nodes.cuh
- cpp/src/routing/problem/problem.cu
- cpp/src/routing/data_model_view.cu
- python/cuopt/cuopt/routing/vehicle_routing.py
Signed-off-by: Nikolai Poperechnyi <n.poperechnyi@gmail.com>
|
/ok to test 2560a85 |
CI Test Summary⏭️ All 5 test job(s) skipped. |
|
🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
Resolve modify/delete conflicts on the old host-export serialize path by dropping it, and wire add_distance_break through the gRPC routing problem so remote solves keep the new setter. Signed-off-by: Rajesh Gandham <rgandham@nvidia.com>
|
/ok to test 7ac1f36 |
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Awesome work @np96 , I have few comments on API structure and options, please let me know if you need help on any.
| data_model.set_order_locations(cudf.Series([1, 2], dtype=np.int32)) | ||
|
|
||
| data_model.add_distance_break( | ||
| vehicle_ids=0, |
There was a problem hiding this comment.
Should it be just vehicle_id ?
| vehicle_ids=0, | |
| vehicle_id=0, |
There was a problem hiding this comment.
Also we may need to support locations option with numpy and pandas input as well since we recently moved to supporting those input options as well.
| default weight is ``1.0``; configure another positive weight to change the | ||
| tradeoff, or explicitly set it to ``0.0`` to disable the early-break penalty. | ||
|
|
||
| The example below sets up one vehicle, two customers, and a single break |
There was a problem hiding this comment.
I would suggest moving the example details within the file so both are in one place.
|
|
||
| Parameters | ||
| ---------- | ||
| vehicle_ids : int or sequence of int |
There was a problem hiding this comment.
Should this be configured per vehicle at a time like we do for vehicle break? @rg20
Match the per-vehicle time-window break API (distance_min/distance_max instead of earliest/latest) across Python, the service, gRPC, and C++. Signed-off-by: Rajesh Gandham <rgandham@nvidia.com>
|
@ramakrishnap-nv I followed your recommendation and made the distance breaks consistent with per vehicle time window based breaks. Can you please re-review? @np96 for viz! |
|
/ok to test 30cacd3 |
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Minor doc suggestion rest looks good.
| tradeoff, or explicitly set it to ``0.0`` to disable the early-break penalty. | ||
|
|
||
| The example below sets up one vehicle, two customers, and a single break | ||
| location. With ``distance_max=75``, the solver must insert one break within |
There was a problem hiding this comment.
Since these details are already covered as part of comment in example, may be we can remove it.
There was a problem hiding this comment.
Hi there. Fixed this one. Also updated the PR description.
Thanks.
Signed-off-by: Nikolai Poperechnyi <n.poperechnyi@gmail.com>
|
/ok to test 4aebba5 |
Raise the CUDA 12 libcuopt compressed wheel allowance to 725 MiB and apply the repository formatters to the distance-break changes. Signed-off-by: Rajesh Gandham <rgandham@nvidia.com>
|
/ok to test 149b600 |
7 participants
Description
Adds per-vehicle distance-based breaks to routing, enabling charging, refueling, or rest stops to be constrained by cumulative travel distance rather than time.
DataModel.add_vehicle_distance_break(vehicle_id, distance_min, distance_max, duration, locations=None)), the server, and gRPC. Repeat calls or entries for additional vehicles or breaks.distance_maxas a hard cumulative-distance limit anddistance_minas a soft target. Early arrival contributes toObjective.DISTANCE_BREAK_COST; its default weight is1.0, and0.0disables only this penalty.Distance breaks cannot be combined with uniform breaks configured through
add_break_dimension. They also inherit the existing exactly-N-breaks-per-route behavior tracked in #1195.Issue
Addresses #589
Checklist