routing: add distance breaks#1196
Open
np96 wants to merge 4 commits into
Open
Conversation
Signed-off-by: Nikolai Poperechnyi <n.poperechnyi@gmail.com>
📝 WalkthroughWalkthroughThis PR adds distance-windowed mandatory breaks: new data structures and flags, node/route propagation of distance windows and excess, unified window feasibility checks combining time+distance, API/binding methods to register distance breaks, server/fleet-data plumbing, unit/integration tests, and docs/examples. ChangesDistance-Based Charging Breaks Implementation
🎯 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: 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
Comment on lines
+106
to
108
| 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
Contributor
|
@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? |
Signed-off-by: Nikolai Poperechnyi <n.poperechnyi@gmail.com>
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
Comment on lines
+595
to
+629
| void check_distance_break_windows(host_assignment_t<i_t> const& h_routing_solution, | ||
| f_t min_range, | ||
| f_t max_range) | ||
| { | ||
| auto const& truck_id = h_routing_solution.truck_id; | ||
| auto const& locations = h_routing_solution.locations; | ||
| auto const& node_types = h_routing_solution.node_types; | ||
|
|
||
| auto cost_matrix_h = matrices_h.get_cost_matrix(0); | ||
| i_t prev_loc = -1; | ||
| i_t curr_truck = -1; | ||
| f_t cumulative = 0.f; | ||
| size_t break_count = 0; | ||
|
|
||
| for (size_t i = 0; i < truck_id.size(); ++i) { | ||
| if (truck_id[i] != curr_truck) { | ||
| curr_truck = truck_id[i]; | ||
| cumulative = 0.f; | ||
| prev_loc = locations[i]; | ||
| continue; | ||
| } | ||
| i_t loc = locations[i]; | ||
| cumulative += cost_matrix_h[prev_loc * n_locations + loc]; | ||
| prev_loc = loc; | ||
| if (static_cast<node_type_t>(node_types[i]) == node_type_t::BREAK) { | ||
| ++break_count; | ||
| ASSERT_GE(cumulative, min_range - 1e-3f) | ||
| << "break at cumulative distance " << cumulative << " is below min_range " << min_range; | ||
| ASSERT_LE(cumulative, max_range + 1e-3f) | ||
| << "break at cumulative distance " << cumulative << " exceeds max_range " << max_range; | ||
| } | ||
| } | ||
| ASSERT_GT(break_count, 0u) | ||
| << "expected at least one BREAK node in the solution, none were emitted"; | ||
| } |
There was a problem hiding this comment.
Don't treat min_range as a hard feasibility bound in the regression helper.
Distance-break lower bounds are soft in this feature: an early break contributes infeasibility cost, but it is still a valid solution. This helper currently fails any emitted BREAK below min_range, so the new regression can reject correct solver behavior and bias the implementation toward the wrong model. Keep the hard max_range assertion, but validate lower-bound behavior via infeasibility/objective checks on a controlled case instead.
As per coding guidelines, .github/.coderabbit_review_guide.md: "Correctness—algorithm/constraint handling bugs ... especially around new routing feasibility/infeasibility logic for distance-window breaks and reuse of existing infeasibility cost dimensions (no misclassification)."
🤖 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/routing_test.cuh` around lines 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.
Contributor
Author
There was a problem hiding this comment.
Indeed, min_range is considered hard bound (i.e. under-driving introduces large penalty and the solution is considered infeasible).
If you think that should be treated differently, let me know.
There was a problem hiding this comment.
@np96, thanks for the clarification! If the solver applies a large enough penalty that solutions with breaks before min_range are effectively infeasible (i.e., the solver avoids them in practice), then the ASSERT_GE is a valid correctness guard — it verifies the solver respects the constraint as intended.
One small observation: the documentation summary for routing-features.rst describes min_range as "not a hard feasibility cutoff" and notes that early breaks "incur a window-violation excess minimized by the solver." If the practical behavior is that violations are large enough to be treated as infeasible, it may be worth clarifying the docs to reflect that — e.g., noting that while the bound is enforced via penalty rather than hard rejection, the penalty magnitude is calibrated to make violations practically infeasible. That would prevent future contributors from misinterpreting the semantics the same way I did.
Happy to withdraw the concern otherwise — the test assertion is correct given your implementation intent. ✅
✏️ Learnings added
Learnt from: np96
Repo: NVIDIA/cuopt PR: 1196
File: cpp/tests/routing/routing_test.cuh:595-629
Timestamp: 2026-05-11T15:43:26.942Z
Learning: In NVIDIA/cuopt routing (cpp/tests/routing/routing_test.cuh), `check_distance_break_windows` treats `min_range` as a hard lower bound assertion (ASSERT_GE). This is intentional: in the distance-break feature, violating the lower bound (under-driving before min_range) incurs a large penalty that makes such solutions effectively infeasible, so the solver never emits breaks below min_range. Do NOT flag this ASSERT_GE as incorrect — it correctly validates the intended solver behavior even though the underlying implementation uses a penalty model rather than hard constraint rejection.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 986
File: cpp/src/branch_and_bound/branch_and_bound.cpp:8-8
Timestamp: 2026-03-23T11:33:23.998Z
Learning: In this repo (NVIDIA/cuopt), treat nvcc as the supported CUDA toolchain; clang-based compilation/support is not required and may fail/break. During code reviews, do NOT request code changes or add blocking comments for errors that appear only under clang (e.g., header-resolution failures such as 'utilities/determinism_log.hpp not found')—these can be toolchain-related rather than real source issues.
Learnt from: bdice
Repo: NVIDIA/cuopt PR: 1035
File: cpp/tests/utilities/base_fixture.hpp:29-47
Timestamp: 2026-04-19T16:49:17.616Z
Learning: In NVIDIA/cuopt (and the upstream rapidsai/rmm) after the RMM → CCCL migration, the rmm::mr resource adaptors (e.g., rmm::mr::pool_memory_resource and rmm::mr::binning_memory_resource) are now owning: they take/hold their upstream resources by value instead of non-owning references. Therefore, direct construction of adaptor chains from temporaries/local values (e.g., pool_memory_resource(make_async(), size)) is safe and should NOT be flagged as potential dangling/lifetime bugs. Also, rmm::mr::make_owning_wrapper is no longer needed/available for this owning design, so do not suggest it in this codepath.
Contributor
Author
There was a problem hiding this comment.
Thanks. I missed that, will update the documentation.
There was a problem hiding this comment.
@np96, sounds great! A small note in routing-features.rst clarifying that the min_range penalty is calibrated to make under-driving practically infeasible (even though it flows through the infeasibility cost model rather than hard rejection) will help future contributors understand the intended semantics. Thanks for the follow-up!
🧠 Learnings used
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 986
File: cpp/src/branch_and_bound/branch_and_bound.cpp:8-8
Timestamp: 2026-03-23T11:33:23.998Z
Learning: In this repo (NVIDIA/cuopt), treat nvcc as the supported CUDA toolchain; clang-based compilation/support is not required and may fail/break. During code reviews, do NOT request code changes or add blocking comments for errors that appear only under clang (e.g., header-resolution failures such as 'utilities/determinism_log.hpp not found')—these can be toolchain-related rather than real source issues.
Learnt from: bdice
Repo: NVIDIA/cuopt PR: 1035
File: cpp/tests/utilities/base_fixture.hpp:29-47
Timestamp: 2026-04-19T16:49:17.616Z
Learning: In NVIDIA/cuopt (and the upstream rapidsai/rmm) after the RMM → CCCL migration, the rmm::mr resource adaptors (e.g., rmm::mr::pool_memory_resource and rmm::mr::binning_memory_resource) are now owning: they take/hold their upstream resources by value instead of non-owning references. Therefore, direct construction of adaptor chains from temporaries/local values (e.g., pool_memory_resource(make_async(), size)) is safe and should NOT be flagged as potential dangling/lifetime bugs. Also, rmm::mr::make_owning_wrapper is no longer needed/available for this owning design, so do not suggest it in this codepath.
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
Comment on lines
+415
to
+419
| Both window endpoints are hard feasibility constraints. Unlike | ||
| time-based breaks the distance dimension has no "wait" analogue, so a | ||
| break that lands before ``min_range`` or after ``max_range`` is | ||
| infeasible — the solver cannot stall the vehicle to shift the | ||
| cumulative distance. |
There was a problem hiding this comment.
Docstring semantics for distance-window violations look overstated.
This block currently states early/late distance breaks are strictly infeasible. That conflicts with the PR behavior description where distance-window violations are penalized via infeasibility cost. Please align wording to avoid misleading API users.
Proposed wording adjustment
- Both window endpoints are hard feasibility constraints. Unlike
- time-based breaks the distance dimension has no "wait" analogue, so a
- break that lands before ``min_range`` or after ``max_range`` is
- infeasible — the solver cannot stall the vehicle to shift the
- cumulative distance.
+ Unlike time-based breaks, the distance dimension has no "wait"
+ analogue. If a break lands before ``min_range`` or after
+ ``max_range``, the solver cannot stall to shift cumulative distance,
+ so the route incurs a distance-window violation in infeasibility 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 `@python/cuopt/cuopt/routing/vehicle_routing.py` around lines 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.
Contributor
Author
There was a problem hiding this comment.
infeasible = penalized via large infeasibility cost...
There was a problem hiding this comment.
@np96, understood — in cuOpt's solver model, "infeasible" means the route accumulates a large infeasibility-cost penalty rather than being outright rejected. Thanks for the clarification.
That said, API users unfamiliar with cuOpt's penalty-based approach might read "hard feasibility constraints" as meaning the solver will refuse to produce any solution that violates those bounds. If you'd like to proactively avoid that confusion, a one-sentence qualifier like "(enforced via infeasibility cost penalty)" could help. But if the existing phrasing aligns with how infeasibility is described elsewhere in the docs/codebase, it's perfectly reasonable to keep it consistent.
Happy to leave the comment as-is if you prefer no change.
✏️ Learnings added
Learnt from: np96
Repo: NVIDIA/cuopt PR: 1196
File: python/cuopt/cuopt/routing/vehicle_routing.py:415-419
Timestamp: 2026-05-11T16:22:58.142Z
Learning: In cuOpt's routing solver (python/cuopt/cuopt/routing/vehicle_routing.py and related C++ code), "infeasible" means the route incurs a large infeasibility cost penalty rather than the solution being hard-rejected. Distance-window violations for distance-based breaks flow through the existing infeasibility cost mechanism with no new weight. The terms "infeasible" and "hard feasibility constraint" are used intentionally in this codebase to mean penalty-based infeasibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
Author
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 👍 |
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
3 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
Adds distance-based vehicle breaks to cuOpt routing. A distance break is a mandatory charging stop the vehicle must take within a cumulative-distance window
[distance_min, distance_max], optionally restricted to a set of charging-station locations. The mechanism mirrors time-based breaks with one deliberate semantic difference: there is no "distance wait" analogue, so visiting a break location before the window's lower bound incurs a window-violation penalty rather than being silently relaxed.Public API
data_model_view_t::add_distance_break(vehicle_id, distance_min, distance_max, charge_duration, break_locations, num_break_locations, validate_input=true).DataModel.add_distance_break(vehicle_ids, max_range, charge_duration, charging_stations=None, min_range=0.0, n_cycles=None). Each call expands ton_cyclesconsecutive cycles[k * max_range + min_range, (k+1) * max_range]fork = 0 … n_cycles - 1.vehicle_break_tnow has a distance-based constructor and carriesis_distance_based_,distance_min_,distance_max_.DIST dimension extension
Rather than introduce a new infeasibility dimension, the existing DIST dimension is extended:
cost_dimension_info_t::has_distance_windowgates the new state.distance_node_t/distance_route_t:distance_window_forward,distance_window_backward,distance_window_backward_min,excess_forward,excess_backward.Operator updates
sliding_window,lexicographic_search,permutation_helper,delivery_insertion,compute_delivery_insertions,compute_ejections,compute_insertions,breaks_insertion, andsqueezeare all updated to respect the window state. To collapse the repeated(TIME excess) || (DIST window excess)check,node_t::window_forward_feasible/window_backward_feasiblehelpers anddimensions_info::has_window_dimension()were added.Server
VehicleDistanceBreakpydantic schema with field validation (max_range,charge_duration,n_cycles, optionalcharging_stationsandmin_range).FleetData.vehicle_distance_breaks: Optional[List[VehicleDistanceBreak]].validation_fleet_data.py,optimization_data_model.py,solver.py.Tests
cpp/tests/routing/unit_tests/distance_breaks.cu— 12 unit tests: propagation correctness, the combine invariant,get_cost/combine/compute_costconsistency, feasible and infeasible cases, edge cases.cpp/tests/routing/level1/l1_routing_test.cu— newCVRPTW_DISTANCE_BREAKSsuite (regression_routing_test_distance_breaks_t) parametrised ondatasets/ref/l1_25.txt(6 Solomon-25 instances).python/cuopt/cuopt/tests/routing/test_distance_breaks.py— Python integration tests: feasibility, multi-cycle, mixed-vehicle (some constrained, some not), and a no-regression test where the feature is unused.Documentation
docs/cuopt/source/cuopt-python/routing/examples/distance_break_example.pyreferenced fromrouting-examples.rst.routing-features.rst.Known limitation (out of scope)
The break-insertion framework still uses the "exactly N breaks per route" invariant (see related issue Distance breaks inherit it: when
n_cyclesexceeds a route's actual need, the surplus cycles are still inserted.Issue
#589
Checklist