Fix #903: Validate unique vehicle_ids in VRP fleet data#917
Open
anandhkb wants to merge 1 commit intoNVIDIA:mainfrom
Open
Fix #903: Validate unique vehicle_ids in VRP fleet data#917anandhkb wants to merge 1 commit intoNVIDIA:mainfrom
anandhkb wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
anandhkb
commented
Mar 1, 2026
anandhkb
commented
- Add uniqueness check in validate_fleet_data() for vehicle_ids
- Return clear error when duplicates detected
- Update FleetData.vehicle_ids docstring
- Add unit tests: duplicate rejection, unique acceptance, edge cases
anandhkb
added
non-breaking
- Add uniqueness check in validate_fleet_data() for vehicle_ids - Return clear error when duplicates detected - Update FleetData.vehicle_ids docstring - Add unit tests: duplicate rejection, unique acceptance, edge cases
📝 WalkthroughWalkthroughAdds uniqueness enforcement for fleet vehicle_ids: documentation updated to state no duplicates, validation now rejects duplicate vehicle_ids, and new unit and API tests exercise duplicate and non-duplicate scenarios, None handling, and single-vehicle cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
python/cuopt_server/cuopt_server/tests/test_set_fleet_data.py (1)
66-175: Consider adding an empty list edge case test.The current tests cover key scenarios well. For completeness, you might consider adding a test for
vehicle_ids=[](empty list), which should be considered unique (no duplicates in an empty set). This is a minor suggestion as the current test coverage is already solid.🧪 Optional: Add empty list test
# Test validate_fleet_data with empty vehicle_ids (no server required) def test_validate_fleet_data_empty_vehicle_ids(): vehicle_locations = [] is_valid, msg = validate_fleet_data( vehicle_ids=[], vehicle_locations=vehicle_locations, capacities=None, vehicle_time_windows=None, vehicle_breaks=None, vehicle_break_time_windows=None, vehicle_break_durations=None, vehicle_break_locations=None, vehicle_types=None, vehicle_types_dict={}, vehicle_order_match=None, skip_first_trips=None, drop_return_trips=None, min_vehicles=None, vehicle_max_costs=None, vehicle_max_times=None, vehicle_fixed_costs=None, ) assert is_valid is True assert msg == "Valid Fleet Data"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt_server/cuopt_server/tests/test_set_fleet_data.py` around lines 66 - 175, Add a new unit test function named test_validate_fleet_data_empty_vehicle_ids that calls validate_fleet_data with vehicle_ids=[] and vehicle_locations=[] (and all other parameters the same as existing tests: capacities=None, vehicle_time_windows=None, etc.), then assert is_valid is True and msg == "Valid Fleet Data"; this checks the empty-list edge case and verifies the function treats an empty vehicle_ids list as valid/unique.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuopt_server/cuopt_server/tests/test_set_fleet_data.py`:
- Around line 66-175: Add a new unit test function named
test_validate_fleet_data_empty_vehicle_ids that calls validate_fleet_data with
vehicle_ids=[] and vehicle_locations=[] (and all other parameters the same as
existing tests: capacities=None, vehicle_time_windows=None, etc.), then assert
is_valid is True and msg == "Valid Fleet Data"; this checks the empty-list edge
case and verifies the function treats an empty vehicle_ids list as valid/unique.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/cuopt_server/cuopt_server/tests/test_set_fleet_data.pypython/cuopt_server/cuopt_server/utils/routing/data_definition.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py`:
- Around line 88-92: The current hard-reject that returns False when
len(vehicle_ids) != len(set(vehicle_ids)) will break clients; replace it with a
deprecation-warning phase: instead of returning immediately in the
duplicate-check block that tests len(vehicle_ids) != len(set(vehicle_ids)), emit
a DeprecationWarning (via warnings.warn(..., DeprecationWarning) and/or a
structured server log) and allow the request to continue for now; add a
configurable flag or version-gated enforcement (e.g., check an
environment/config flag like ENFORCE_UNIQUE_VEHICLE_IDS or compare against a
target version) that, when enabled in a future release, flips the behavior to
the current hard-reject, and add/update unit tests to cover both warning-mode
and enforcement-mode.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/cuopt_server/cuopt_server/tests/test_set_fleet_data.pypython/cuopt_server/cuopt_server/utils/routing/data_definition.pypython/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuopt_server/cuopt_server/tests/test_set_fleet_data.py
Comment on lines
+88
to
+92
| if len(vehicle_ids) != len(set(vehicle_ids)): | ||
| return ( | ||
| False, | ||
| "vehicle_ids must be unique; duplicates are not allowed", | ||
| ) |
There was a problem hiding this comment.
Add a deprecation phase before hard-rejecting duplicate vehicle_ids.
Line 88 introduces an immediate server/API behavior break. Please gate this with a deprecation warning window first, then enforce rejection in a subsequent version.
As per coding guidelines, "maintain backward compatibility in Python and server APIs with deprecation warnings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuopt_server/cuopt_server/utils/routing/validation_fleet_data.py`
around lines 88 - 92, The current hard-reject that returns False when
len(vehicle_ids) != len(set(vehicle_ids)) will break clients; replace it with a
deprecation-warning phase: instead of returning immediately in the
duplicate-check block that tests len(vehicle_ids) != len(set(vehicle_ids)), emit
a DeprecationWarning (via warnings.warn(..., DeprecationWarning) and/or a
structured server log) and allow the request to continue for now; add a
configurable flag or version-gated enforcement (e.g., check an
environment/config flag like ENFORCE_UNIQUE_VEHICLE_IDS or compare against a
target version) that, when enabled in a future release, flips the behavior to
the current hard-reject, and add/update unit tests to cover both warning-mode
and enforcement-mode.
rg20
approved these changes
Mar 3, 2026
Iroy30
approved these changes
Mar 4, 2026
Contributor
Author
|
/merge |
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.