While reviewing python/cuopt/routing.py, several logical and structural issues were identified in the construct_rerouting_model function. These issues range from critical logic errors (incorrect dictionary keys) to runtime safety hazards (unsafe None handling) and API misuse.
These issues affect the correctness and robustness of the re-routing pipeline.
1. [Critical] Wrong dictionary key used for base capacity dimension
Description:
The code incorrectly uses a loop variable (name) that leaks from a previous loop scope to store the base capacity dimension. This results in the capacity data being stored under an arbitrary key (the last element of vehicle_order_matching_constraints) instead of the correct demand_name.
Vulnerable Code:
# Previous loop where 'name' is defined
for name in vehicle_order_matching_constraints:
if name in capacity_dimensions:
del capacity_dimensions[name]
# ... later in the code ...
# 'name' here holds the last value from the loop above!
capacity_dimensions_h[name] = {
"demand": order_demand,
"capacity": vehicle_cap,
}Expected Behavior:
The base demand dimension should be stored using the correct key variable demand_name derived earlier in the function.
Suggested Fix:
capacity_dimensions_h[demand_name] = { # Use demand_name instead of name
"demand": order_demand,
"capacity": vehicle_cap,
}
2. Unsafe new_order_data handling
Description:
The function treats new_order_data as optional at the beginning but later accesses it unconditionally. If new_order_data is None, the function will crash with a TypeError.
Vulnerable Code:
# Check is optional
if new_order_data is not None:
# validation logic...
# ... later ...
# Unconditional access -> Crash if new_order_data is None
new_order_locations_h.extend(new_order_data["order_locations"])Suggested Fix:
Either make new_order_data mandatory check at the start:
if new_order_data is None:
raise ValueError("new_order_data must be provided for rerouting")
3. Unsafe type assumption on get_order_service_times
Description:
The code assumes original_model.get_order_service_times() always returns a cudf.Series (or similar object with .to_arrow()). However, the API can return a dict for vehicle-specific service times. If a dict is returned, the code raises an AttributeError.
Vulnerable Code:
service_time = original_model.get_order_service_times()
# If service_time is a dict, this fails:
service_time_h = service_time.to_arrow().to_pylist()
Suggested Fix:
Add an isinstance check or handle the dictionary return type explicitly.
4. API Mismatch: Integer used instead of Boolean
Description:
The set_drop_return_trips API expects a cudf.Series of bool. The current implementation passes integers (1), relying on implicit casting which may vary by backend.
Vulnerable Code:
drop_return = [1] * vehicle_num # Should be True
drop_return = cudf.Series(drop_return)
new_d.set_drop_return_trips(drop_return)
Suggested Fix:
drop_return = cudf.Series([True] * vehicle_num)
Environment
- File:
python/cuopt/routing.py
- Function:
construct_rerouting_model
Impact
While reviewing
python/cuopt/routing.py, several logical and structural issues were identified in theconstruct_rerouting_modelfunction. These issues range from critical logic errors (incorrect dictionary keys) to runtime safety hazards (unsafeNonehandling) and API misuse.These issues affect the correctness and robustness of the re-routing pipeline.
1. [Critical] Wrong dictionary key used for base capacity dimension
Description:
The code incorrectly uses a loop variable (
name) that leaks from a previous loop scope to store the base capacity dimension. This results in the capacity data being stored under an arbitrary key (the last element ofvehicle_order_matching_constraints) instead of the correctdemand_name.Vulnerable Code:
Expected Behavior:
The base demand dimension should be stored using the correct key variable
demand_namederived earlier in the function.Suggested Fix:
2. Unsafe
new_order_datahandlingDescription:
The function treats
new_order_dataas optional at the beginning but later accesses it unconditionally. Ifnew_order_dataisNone, the function will crash with aTypeError.Vulnerable Code:
Suggested Fix:
Either make
new_order_datamandatory check at the start:3. Unsafe type assumption on
get_order_service_timesDescription:
The code assumes
original_model.get_order_service_times()always returns acudf.Series(or similar object with.to_arrow()). However, the API can return adictfor vehicle-specific service times. If a dict is returned, the code raises anAttributeError.Vulnerable Code:
Suggested Fix:
Add an
isinstancecheck or handle the dictionary return type explicitly.4. API Mismatch: Integer used instead of Boolean
Description:
The
set_drop_return_tripsAPI expects acudf.Seriesofbool. The current implementation passes integers (1), relying on implicit casting which may vary by backend.Vulnerable Code:
Suggested Fix:
Environment
python/cuopt/routing.pyconstruct_rerouting_modelImpact