Routing over gRPC: VRP server + compiled C++/Cython client#1597
Routing over gRPC: VRP server + compiled C++/Cython client#1597ramakrishnap-nv wants to merge 10 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
The routing gRPC path will use one architecture, consistent with LP/MILP: a compiled C++/Cython client that serializes a host cpu_routing_problem_t in C++ (map_routing_problem_to_proto) rather than a parallel pure-Python protobuf path. Remove the Phase 3 Python export layer (to_host_problem / _serialize.py and its test): the store-then-build IR is instead walked in the Cython client to fill cpu_routing_problem_t, so the mapping lives in one place and there is no second serializer to maintain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
A pure-Python client would need a second serialization path; instead follow the LP/MILP pattern so routing has one architecture: - C++: cpu_routing_solution_t + map_proto_to_routing_solution (client-side parse of the RoutingSolution blob); submit_vrp/get_vrp_result/solve_vrp on grpc_client_t (unary-only, mirroring submit_lp); accept routing_solution in the unary GetResult path. - cython shim (grpc_python_client_t): submit_vrp/result_vrp exposing the plain host structs to Cython (no grpc/protobuf in the extension build). - Cython client (cuopt.grpc.routing.RoutingClient): walks the DataModel store-then-build IR to fill cpu_routing_problem_t -- the single mapping site that replaces to_host_problem -- then drives submit/wait/result and returns the parsed solution. Validated end-to-end against cuopt_grpc_server: remote solve matches local (objective, route, vehicle count) for cost-matrix, capacity, and time-window problems. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
problem_summary() runs the exact _populate path without a server; the tests assert every cuopt.routing._deferred._SETTERS name is mapped (fail-loud on a new setter), exercise each field category, cover numpy/pandas/cuDF inputs, and check the fail-loud KeyError on an unmapped setter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
added
non-breaking
0cdcaf6 to
bca7e01
Compare
|
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:
📝 WalkthroughWalkthroughAdds unary VRP gRPC schemas, C++ server/client execution, host-to-device routing conversion, Cython/Python bindings, a standalone C++ driver, and Python serialization/integration tests. The previous Python host serializer and its tests are removed. ChangesUnary VRP gRPC integration
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
CI Test Summary✅ All 31 test job(s) passed. |
- Remove grpc_client_t::solve_vrp: the Cython client drives submit/wait/result through the shim, so the C++ one-shot convenience had no caller. - Trim unused .pxd declarations (TLS options, status(), last_error(), and unused result fields) to only what the client uses. - _add_matrix: drop the redundant to_host/astype/ascontiguousarray/ravel that _fill_f32 already performs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/grpc/server/grpc_worker.cpp (1)
570-577: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winVRP result size isn't accounted for, defeating the oversized-result guard.
result_total_bytesonly sumssr.arrays, butrun_vrp_solve(lines 535-565) never populatessr.arrays— the routing solution bytes live insr.header.routing_solution(). For VRP jobs this leavesresult_total_bytes == 0, soresult.data_sizebecomesstd::max(0, 1) == 1regardless of how large the actual serializedRoutingSolutionis.This silently breaks the size guard in
grpc_service_impl.cpp'sGetResult(theif (max_bytes > 0 && total_result_bytes > max_bytes)check around line 353), which relies on this sameresult_size_bytesto redirect oversized results to chunked download with a clear message. Since chunked VRP download is unsupported (rejected both client- and server-side), a large VRP solution will instead attempt to be sent as a single oversized unary gRPC response and fail with an opaque transport-level error instead of the intended gracefulRESOURCE_EXHAUSTEDmessage.🐛 Suggested fix
int64_t result_total_bytes = 0; if (sr.success) { for (const auto& [fid, data] : sr.arrays) { result_total_bytes += data.size(); } + if (sr.header.is_vrp()) { + result_total_bytes += static_cast<int64_t>(sr.header.routing_solution().size()); + } }🤖 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/grpc/server/grpc_worker.cpp` around lines 570 - 577, Update publish_result to include the serialized routing solution size from sr.header.routing_solution() when calculating result_total_bytes, while preserving the existing sr.arrays accounting for non-VRP results. Ensure result_size_bytes reflects the actual VRP payload so grpc_service_impl.cpp’s GetResult oversized-result guard can return the intended RESOURCE_EXHAUSTED response.
🧹 Nitpick comments (7)
cpp/include/cuopt/routing/cpu_routing_problem.hpp (1)
63-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM overall; minor doc gap on
to_device().Struct layout and RAII-friendly
device_data_ptrpattern are solid. Theto_device()docstring doesn't mention that it throwsstd::invalid_argumentfor null handle, non-positive dimensions, empty cost matrices, or incomplete capacity/break/uniform-break data (per the.cuimplementation). Callers likerun_vrp_solveingrpc_worker.cppcatch broad exception types, so documenting the specific failure conditions here would help future maintainers reason about error handling without cross-referencing the.cufile.As per path instructions, "New public functions/classes need Doxygen-style documentation" and to "Suggest documenting thread-safety, GPU requirements, numerical behavior" for public headers under
cpp/include/cuopt/**/*.📝 Suggested doc improvement
/** - * `@brief` Copy host data to the GPU and return a non-owning data_model_view_t. - * The returned device_data_t must outlive the view. + * `@brief` Copy host data to the GPU and return a non-owning data_model_view_t. + * + * `@param` handle Non-null RAFT handle whose stream is used for all H2D copies. + * `@return` A pair of the device-backed view and the owning device buffers; + * the returned device_data_t must outlive the view. + * `@throws` std::invalid_argument if handle is null, num_locations/fleet_size + * are non-positive, cost_matrices is empty, or any dependent + * array group (capacity dimensions, uniform breaks) is incomplete. */🤖 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/include/cuopt/routing/cpu_routing_problem.hpp` around lines 63 - 124, Update the Doxygen comment for cpu_routing_problem_t::to_device to document that it requires a valid GPU handle and throws std::invalid_argument for a null handle, non-positive dimensions, empty cost matrices, or incomplete capacity, break, and uniform-break data. Retain the existing ownership and view-lifetime documentation, and mention any relevant GPU/thread-safety or numerical behavior only if established by the implementation.Source: Path instructions
cpp/src/grpc/grpc_routing_problem_mapper.cpp (1)
24-32: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSwitch to bulk
RepeatedFieldinsertion.dst->Add(src.begin(), src.end())is supported and avoids the per-element loop for large vectors.🤖 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/grpc/grpc_routing_problem_mapper.cpp` around lines 24 - 32, Update copy_vector_to_repeated to replace the per-element Add loop with the bulk RepeatedField insertion using src.begin() and src.end(), while preserving the existing Clear and Reserve behavior.python/cuopt/cuopt/grpc/routing/grpc_client.pyx (3)
363-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
wait()discardserror_message, sosolve()'s failure message loses the actual reason.
wait()only returns<int>st.status;st.error_message/st.successare dropped. On a timeout or connection failure,solve()(388-390) raisesf"job {job_id} did not complete (status {status})", which shows only the numeric status and not the underlying reason (e.g., "Timeout waiting for job completion" vs. "Not connected to server") that the C++ layer already computed.Also applies to: 383-396
🤖 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/grpc/routing/grpc_client.pyx` around lines 363 - 369, Update grpc_client.wait to preserve and return the complete wait result, including st.status, st.success, and st.error_message, then update solve to consume that result and include the underlying error message when raising for timeout or connection failure while preserving the existing successful completion behavior.
324-396: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlocking gRPC calls hold the GIL for the whole call (including long solves).
connect,submit,wait,result, anddeleteall call intogrpc_python_client_tmethods that can block for the entire RPC/poll duration (notablywait, which can run up totimeoutseconds spanning the full solve). None of these are wrapped inwith nogil:, so the GIL is held the whole time, starving any other Python threads in the process. Per path instructions, blocking C calls should release the GIL unless it's actually needed.Fixing this requires marking the corresponding
.pxdextern declarationsnogiland precomputing any Python-dependent inputs (e.g.,job_id.encode(...)) before entering thewith nogil:block.As per path instructions, "Use `nogil` on blocking C calls unless Python GIL access is required."♻️ Example for wait()
def wait(self, str job_id, int timeout=0): """Block until the job finishes; return the terminal status int.""" - cdef grpc_status_result_t st = self._client.get().wait( - job_id.encode("utf-8"), timeout - ) + cdef string job_id_cpp = job_id.encode("utf-8") + cdef grpc_status_result_t st + with nogil: + st = self._client.get().wait(job_id_cpp, timeout) return <int>st.status(and mark
wait(...) except + nogilingrpc_client.pxd)🤖 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/grpc/routing/grpc_client.pyx` around lines 324 - 396, Release the GIL around the blocking grpc_python_client_t calls in __cinit__, submit, wait, result, and delete, while computing Python-dependent values such as encoded job IDs before each nogil block. Mark the corresponding extern methods in grpc_client.pxd as except + nogil, and preserve existing error handling and return behavior.Source: Path instructions
296-317: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
_solution_to_pycopies the whole solution struct by value.
cdef _solution_to_py(cpu_routing_solution_t s)takessby value, so every call (_solution_to_py(out.solution)at line 377) performs a full deep copy of all vectors/maps incpu_routing_solution_t(route, truck_id, locations, node_types, arrival_stamp, objective_values, etc.) before the function even starts converting to numpy. For larger VRP solutions this is avoidable overhead.♻️ Proposed fix — take by reference
-cdef _solution_to_py(cpu_routing_solution_t s): +cdef _solution_to_py(cpu_routing_solution_t& s):🤖 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/grpc/routing/grpc_client.pyx` around lines 296 - 317, Change _solution_to_py to accept cpu_routing_solution_t by const reference instead of by value, preserving all existing field conversions and return behavior while avoiding a deep copy when _solution_to_py(out.solution) is called.cpp/tests/routing/grpc/grpc_vrp_test_driver.cpp (1)
227-228: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest driver channel has no message-size limits, unlike the production client.
grpc_client_t::connect()explicitly raisesSetMaxReceiveMessageSize/SetMaxSendMessageSizefromconfig_.max_message_bytes(up to ~2GiB). This driver'sgrpc::CreateChannel(...)uses defaultChannelArguments, so any moderately large routing JSON dataset used for manual testing will hit gRPC's default (~4MB) message-size ceiling and fail with an opaqueRESOURCE_EXHAUSTED.🔧 Proposed fix
- auto channel = grpc::CreateChannel(server, grpc::InsecureChannelCredentials()); + grpc::ChannelArguments args; + args.SetMaxReceiveMessageSize(-1); + args.SetMaxSendMessageSize(-1); + auto channel = + grpc::CreateCustomChannel(server, grpc::InsecureChannelCredentials(), args);🤖 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/grpc/grpc_vrp_test_driver.cpp` around lines 227 - 228, Update the channel setup in the test driver to use grpc::ChannelArguments with maximum receive and send message sizes configured to match grpc_client_t::connect(), using the driver's applicable max-message-bytes configuration (up to the production limit), then pass those arguments to grpc::CreateCustomChannel instead of the default CreateChannel.cpp/src/grpc/client/grpc_client.cpp (1)
769-793: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo pre-flight size check before unary VRP submission.
submit_lp/submit_mipestimate proto size and fall back to chunked upload when it exceedschunked_array_threshold_bytes_.submit_vrpalways goes straight tosubmit_unarywith no size estimate — consistent with VRP being unary-only per the PR description, but it means a large routing problem (many locations/vehicles/cost-matrix entries) will fail deep inside the RPC with an opaqueRESOURCE_EXHAUSTED/serialization error rather than a clear, early message telling the caller the problem exceeds the unsupported-chunking limit.🤖 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/grpc/client/grpc_client.cpp` around lines 769 - 793, Add a pre-flight serialized-size check in grpc_client_t::submit_vrp after building submit_request and before submit_unary. Reuse the existing size-estimation and chunked_array_threshold_bytes_ logic used by submit_lp/submit_mip; if the VRP request exceeds the threshold, return unsuccessfully with a clear error stating that chunked upload is unsupported for VRP, otherwise preserve the current unary submission flow.
🤖 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/grpc/client/cython_grpc_client.cpp`:
- Around line 171-183: Update grpc_python_client_t::result_vrp to perform the
same in-flight status check as result(), setting
grpc_vrp_result_outcome_t::not_ready and returning before calling get_vrp_result
when the job is still running. Preserve the existing remote error and successful
solution handling for completed jobs.
In `@cpp/src/grpc/client/grpc_client.cpp`:
- Around line 824-845: Update grpc_client_t::solve_vrp to wrap
poll_for_completion with the same start_log_streaming, drain_log_streaming, and
stop_log_streaming lifecycle used by solve_lp and solve_mip. Preserve the
existing success/error handling and ensure streaming is applied according to
config_.stream_logs and config_.log_callback.
In `@cpp/src/grpc/grpc_routing_problem_mapper.cpp`:
- Around line 304-332: Sanitize the error text in map_routing_solution_to_proto
before assigning it to pb->set_error_message, matching the established
format_cuopt_error behavior used by the gRPC server. Parse cuopt::logic_error
structured payloads into a clean type-and-message string and provide a safe
fallback for unrecognized or failed parsing, while preserving the existing
generic routing solve error fallback.
In `@cpp/src/routing/cpu_routing_problem.cu`:
- Around line 81-87: Update copy_u8_as_bool() and the types temporary feeding
cuopt::device_copy(...) so their host buffers remain alive until stream
completion; ensure their lifetimes extend through handle->sync_stream() rather
than ending immediately after queuing the H2D transfer.
- Around line 91-126: The to_device method must validate capacity buffer sizes
before constructing data_model_view_t: ensure each capacity dimension contains
exactly num_orders demand values and fleet_size capacity values before
populate_demand_container can consume them. Add these checks in
cpu_routing_problem_t::to_device alongside the existing input validation,
rejecting undersized or mismatched buffers before building the device view.
In `@python/cuopt/cuopt/grpc/routing/grpc_client.pyx`:
- Around line 336-348: Update _apply_settings so both dictionary and
object-based time_limit handling checks whether tl is None rather than relying
on truthiness, ensuring an explicit value of 0 is converted and passed to
s.set_time_limit while an absent value remains ignored.
---
Outside diff comments:
In `@cpp/src/grpc/server/grpc_worker.cpp`:
- Around line 570-577: Update publish_result to include the serialized routing
solution size from sr.header.routing_solution() when calculating
result_total_bytes, while preserving the existing sr.arrays accounting for
non-VRP results. Ensure result_size_bytes reflects the actual VRP payload so
grpc_service_impl.cpp’s GetResult oversized-result guard can return the intended
RESOURCE_EXHAUSTED response.
---
Nitpick comments:
In `@cpp/include/cuopt/routing/cpu_routing_problem.hpp`:
- Around line 63-124: Update the Doxygen comment for
cpu_routing_problem_t::to_device to document that it requires a valid GPU handle
and throws std::invalid_argument for a null handle, non-positive dimensions,
empty cost matrices, or incomplete capacity, break, and uniform-break data.
Retain the existing ownership and view-lifetime documentation, and mention any
relevant GPU/thread-safety or numerical behavior only if established by the
implementation.
In `@cpp/src/grpc/client/grpc_client.cpp`:
- Around line 769-793: Add a pre-flight serialized-size check in
grpc_client_t::submit_vrp after building submit_request and before submit_unary.
Reuse the existing size-estimation and chunked_array_threshold_bytes_ logic used
by submit_lp/submit_mip; if the VRP request exceeds the threshold, return
unsuccessfully with a clear error stating that chunked upload is unsupported for
VRP, otherwise preserve the current unary submission flow.
In `@cpp/src/grpc/grpc_routing_problem_mapper.cpp`:
- Around line 24-32: Update copy_vector_to_repeated to replace the per-element
Add loop with the bulk RepeatedField insertion using src.begin() and src.end(),
while preserving the existing Clear and Reserve behavior.
In `@cpp/tests/routing/grpc/grpc_vrp_test_driver.cpp`:
- Around line 227-228: Update the channel setup in the test driver to use
grpc::ChannelArguments with maximum receive and send message sizes configured to
match grpc_client_t::connect(), using the driver's applicable max-message-bytes
configuration (up to the production limit), then pass those arguments to
grpc::CreateCustomChannel instead of the default CreateChannel.
In `@python/cuopt/cuopt/grpc/routing/grpc_client.pyx`:
- Around line 363-369: Update grpc_client.wait to preserve and return the
complete wait result, including st.status, st.success, and st.error_message,
then update solve to consume that result and include the underlying error
message when raising for timeout or connection failure while preserving the
existing successful completion behavior.
- Around line 324-396: Release the GIL around the blocking grpc_python_client_t
calls in __cinit__, submit, wait, result, and delete, while computing
Python-dependent values such as encoded job IDs before each nogil block. Mark
the corresponding extern methods in grpc_client.pxd as except + nogil, and
preserve existing error handling and return behavior.
- Around line 296-317: Change _solution_to_py to accept cpu_routing_solution_t
by const reference instead of by value, preserving all existing field
conversions and return behavior while avoiding a deep copy when
_solution_to_py(out.solution) is called.
🪄 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: 2bd3d9dd-5ab6-4aee-94e7-6c2f8e08d89c
⛔ Files ignored due to path filters (2)
cpp/src/grpc/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_enum_converters_problem.incis excluded by!**/generated/**
📒 Files selected for processing (28)
cpp/CMakeLists.txtcpp/include/cuopt/grpc/cython_grpc_client.hppcpp/include/cuopt/routing/cpu_routing_problem.hppcpp/src/grpc/client/cython_grpc_client.cppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/client/grpc_client.hppcpp/src/grpc/codegen/field_registry.yamlcpp/src/grpc/cuopt_remote.protocpp/src/grpc/cuopt_remote_service.protocpp/src/grpc/cuopt_routing.protocpp/src/grpc/grpc_routing_problem_mapper.cppcpp/src/grpc/grpc_routing_problem_mapper.hppcpp/src/grpc/server/grpc_service_impl.cppcpp/src/grpc/server/grpc_worker.cppcpp/src/routing/CMakeLists.txtcpp/src/routing/cpu_routing_problem.cucpp/tests/routing/CMakeLists.txtcpp/tests/routing/grpc/CMakeLists.txtcpp/tests/routing/grpc/grpc_vrp_test_driver.cpppython/cuopt/cuopt/grpc/CMakeLists.txtpython/cuopt/cuopt/grpc/routing/CMakeLists.txtpython/cuopt/cuopt/grpc/routing/__init__.pypython/cuopt/cuopt/grpc/routing/grpc_client.pxdpython/cuopt/cuopt/grpc/routing/grpc_client.pyxpython/cuopt/cuopt/routing/_serialize.pypython/cuopt/cuopt/tests/routing/test_grpc_client.pypython/cuopt/cuopt/tests/routing/test_grpc_serialization.pypython/cuopt/cuopt/tests/routing/test_serialize.py
💤 Files with no reviewable changes (2)
- python/cuopt/cuopt/routing/_serialize.py
- python/cuopt/cuopt/tests/routing/test_serialize.py
| remote_vrp_result_t grpc_client_t::solve_vrp( | ||
| const cuopt::routing::cpu_routing_problem_t& problem, | ||
| const cuopt::routing::solver_settings_t<int, float>& settings) | ||
| { | ||
| remote_vrp_result_t result; | ||
|
|
||
| auto sub = submit_vrp(problem, settings); | ||
| if (!sub.success) { | ||
| result.error_message = sub.error_message; | ||
| return result; | ||
| } | ||
|
|
||
| auto poll = poll_for_completion(sub.job_id); | ||
| if (!poll.completed) { | ||
| result.error_message = poll.error_message; | ||
| return result; | ||
| } | ||
|
|
||
| result = get_vrp_result(sub.job_id); | ||
| if (result.success) { delete_job(sub.job_id); } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
solve_vrp drops log streaming that solve_lp/solve_mip provide.
solve_lp (956-964) and solve_mip (989-995) wrap poll_for_completion with start_log_streaming/drain_log_streaming/stop_log_streaming. solve_vrp calls poll_for_completion directly with none of these, so a caller with config_.stream_logs/config_.log_callback set gets silent VRP solves while LP/MIP solves stream logs — an inconsistent, easy-to-miss feature gap.
🔧 Proposed fix to add log streaming parity
remote_vrp_result_t grpc_client_t::solve_vrp(
const cuopt::routing::cpu_routing_problem_t& problem,
const cuopt::routing::solver_settings_t<int, float>& settings)
{
remote_vrp_result_t result;
auto sub = submit_vrp(problem, settings);
if (!sub.success) {
result.error_message = sub.error_message;
return result;
}
+ start_log_streaming(sub.job_id);
auto poll = poll_for_completion(sub.job_id);
+ if (poll.completed) {
+ drain_log_streaming();
+ } else {
+ stop_log_streaming();
+ }
if (!poll.completed) {
result.error_message = poll.error_message;
return result;
}
result = get_vrp_result(sub.job_id);
if (result.success) { delete_job(sub.job_id); }
return result;
}🤖 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/grpc/client/grpc_client.cpp` around lines 824 - 845, Update
grpc_client_t::solve_vrp to wrap poll_for_completion with the same
start_log_streaming, drain_log_streaming, and stop_log_streaming lifecycle used
by solve_lp and solve_mip. Preserve the existing success/error handling and
ensure streaming is applied according to config_.stream_logs and
config_.log_callback.
- _apply_settings: use `is not None` so an explicit time_limit=0 is applied rather than silently dropped (0 is falsy). - result_vrp: add the in-flight status pre-check that result() does, so polling result() on a running job returns a structured not_ready signal (RoutingClient .result() now returns None) instead of a generic GetResult failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Thanks for the review — all findings addressed. Client (002c971):
Server (7a61e65):
Stale: the Verified: clean rebuild; valid capacity/bool-array/time-window solves match locally; undersized capacity is rejected; 8 serialization tests + GPU-less container path still pass. Full async parity ( |
…e + validate capacity sizes - map_routing_solution_to_proto: run get_error_status() through a sanitizer (mirrors format_cuopt_error) so raw internal error text is not sent to remote clients, without pulling the heavy server header into the client-linked mapper. - to_device: drain the stream before the transient host buffers in copy_u8_as_bool() and the initial-solutions types conversion go out of scope, so their async H2D copies do not read freed memory. - to_device: validate each capacity dimension has num_orders demand and fleet_size capacity values before building the device view, preventing an out-of-bounds read from an undersized (e.g. raw-proto) request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The VRP server commit hand-edited two generated files, which broke the "verify gRPC codegen" check. Make both codegen-driven: - chunked_result_header: support scalars declared directly on the section (emit into the proto verbatim, no LP/MIP conversion). Declares is_vrp/routing_solution. - enum codegen: support per-value divergence between the C++ and proto enums -- `proto_only` (proto value with no C++ enumerator; from_proto throws) and `cpp_aliases` (C++-only enumerator serialized as another proto value). Declares problem_category as VRP (proto-only) + IP->MIP, so problem_category_t (LP/MIP/IP) and ProblemCategory (LP/MIP/VRP) stay decoupled without hand-editing the output. Regenerated files now match the registry; verify_grpc_codegen.sh passes and the server/converter compile. Other enums regenerate byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
tests/routing/test_grpc_client.py collided with the existing tests/linear_programming/test_grpc_client.py under pytest's default prepend import mode (no __init__.py in the test dirs), causing an import-file-mismatch collection error when the whole tests tree is collected (wheel tests). Give the routing grpc tests routing-prefixed basenames so they are unique. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
2 participants
End-to-end VRP (vehicle routing) over the existing gRPC server, using a single architecture consistent with LP/MILP (compiled C++/Cython client, no pure-Python protobuf path). Rebased on
main(which now includes #1591).feature/grpc-server-vrp, rebased onto main):cuopt_routing.proto,vrp_requestarm, server dispatch +grpc_routing_problem_mapper+cpu_routing_problem_t.cpu_routing_solution_t+ client-side parse;submit_vrp/get_vrp_result/solve_vrpongrpc_client_t(unary-only); agrpc_python_client_tshim; andcuopt.grpc.routing.RoutingClient(Cython) that walks the DataModel store-then-build IR intocpu_routing_problem_t— the single mapping site that replacesto_host_problem(removed).Testing
test_grpc_serializationasserts everycuopt.routing._deferred._SETTERSname is mapped (fail-loud on a new setter), exercises each field category via a no-serverproblem_summaryprobe, and covers numpy / pandas / cuDF inputs.cuopt_grpc_server: remote solve matches local (objective, route, vehicle count) for cost-matrix, capacity, and time-window problems.docker runwithout--gpus) — a local solve fails (CUDARuntimeError), while remote solves with numpy and pandas inputs succeed against a host GPU server.VRP is unary-only (no >2 GiB chunking yet), matching the server POC.
🤖 Generated with Claude Code