perf: transcode Python request plane directly - #11104
Conversation
WalkthroughThis PR adds a configurable request-plane payload codec defaulting to MessagePack (JSON optional), generalizes the Ingress pipeline with pluggable request/response payload adapters, and introduces a Python-owned payload path (PythonPayload/PythonResponseItem/PythonIngressPayloadAdapter) replacing serde_json::Value-based conversion in Python bindings, with supporting docs and tests. ChangesRequest-Plane Payload Codec and Adapter Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/bindings/python/tests/test_request_plane_python_payload.py (1)
55-95: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout marker for this network-waiting test.
The test performs
client.wait_for_instances()and iterates network response streams (including malformed/error cases that depend on backend error propagation). Per pytest guidelines for this path, tests with polling/network waits should carry@pytest.mark.timeout(...)to prevent CI hangs if the stream never resolves (e.g., a regression causing the error/malformed cases to hang instead of raising).As per path instructions: "Add
@pytest.mark.timeout(...)for any test expected to run >30s (or any polling/network waits/subprocess waits) to prevent CI hangs."⏱️ Suggested fix
`@pytest.mark.asyncio` +@pytest.mark.timeout(30) `@pytest.mark.parametrize`("request_plane", ["tcp"], indirect=True) async def test_python_request_plane_plain_annotated_error_and_malformed_frames(🤖 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 `@lib/bindings/python/tests/test_request_plane_python_payload.py` around lines 55 - 95, This test in test_python_request_plane_plain_annotated_error_and_malformed_frames performs network waiting and stream iteration that can hang CI, so add a pytest timeout marker to the test. Apply `@pytest.mark.timeout` to the existing async test alongside the current asyncio/parametrize markers, using a reasonable limit for the request_plane_client.generate flow and the malformed/error cases so failures surface instead of stalling.Source: Path instructions
🤖 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 `@lib/bindings/python/rust/python_payload.rs`:
- Around line 255-266: Treat explicit None as absent in the Python payload
parsing logic, because PythonPayload fields like data, id, event, comment, and
error currently use PyDict::get_item()/extract_optional in a way that still
treats present None as a value and can break annotated optional handling. Update
the shared helper used by python_payload.rs to detect PyAny::is_none() and
return None for those keys, then reuse that helper for all envelope optionals so
the behavior matches missing-field semantics.
In `@lib/runtime/src/pipeline/network/ingress/push_handler.rs`:
- Around line 172-181: The response-encoding failure path in
`PushHandler::handle` / `encode_response` currently breaks out while
`send_complete_final` remains enabled, so a clean terminal frame can still be
emitted later from the final-send path. Update the `Err(err)` branch to also
disable the clean-final behavior (or otherwise record that the final frame must
be an error/omitted) before breaking, and make the final frame emission logic
that follows respect that flag so no success terminal frame is sent after an
`encode_response` failure.
- Around line 350-354: The adapter-backed ingress impls still impose the old
serde response bounds on U, which blocks non-serde response types even though
encoding now belongs to IngressResponseEncoder. Update the relevant
IngressDispatch and PushWorkHandler impls for Ingress<SingleIn<T>, ManyOut<U>,
Adapter> / related variants to remove U: Serialize + MaybeError and keep only
the bounds required by Adapter and the handler flow. Make sure the signatures in
the impacted impl blocks use the new adapter-based response contract
consistently.
---
Nitpick comments:
In `@lib/bindings/python/tests/test_request_plane_python_payload.py`:
- Around line 55-95: This test in
test_python_request_plane_plain_annotated_error_and_malformed_frames performs
network waiting and stream iteration that can hang CI, so add a pytest timeout
marker to the test. Apply `@pytest.mark.timeout` to the existing async test
alongside the current asyncio/parametrize markers, using a reasonable limit for
the request_plane_client.generate flow and the malformed/error cases so failures
surface instead of stalling.
🪄 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: 351843c1-e077-46ec-933a-21ed55eb7dbf
⛔ Files ignored due to path filters (1)
lib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
docs/design-docs/request-plane.mdlib/bindings/python/Cargo.tomllib/bindings/python/rust/engine.rslib/bindings/python/rust/lib.rslib/bindings/python/rust/python_payload.rslib/bindings/python/tests/test_request_plane_python_payload.pylib/runtime/src/pipeline/network.rslib/runtime/src/pipeline/network/ingress/push_handler.rslib/runtime/tests/bidirectional_e2e.rslib/runtime/tests/bidirectional_e2e_json.rs
jthomson04
left a comment
There was a problem hiding this comment.
Automated review (medium effort). The core refactor looks sound — I separately confirmed that the codec default asymmetry (serde #[default]=Json for absent-field old senders vs. configured()=Msgpack for new senders) is intentional and correct, that all existing Ingress call sites still compile via the defaulted type param, and that the deleted unary error-mapping is faithfully preserved in map_python_exception.
5 inline findings below, most-severe first: 2 correctness, 3 altitude/cleanup.
5284bad to
08efd11
Compare
08efd11 to
8aecb7c
Compare
8aecb7c to
b0ce41f
Compare
b0ce41f to
bcc221c
Compare
bcc221c to
ad0db97
Compare
|
/ok to test ad0db97 |
GuanLuo
left a comment
There was a problem hiding this comment.
left question, approve to unblock
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
fa9e01d to
99b7dc6
Compare
|
/ok to test 99b7dc6 |
Overview
Make MessagePack the configured request-plane payload default and remove the
serde_json::Valuematerialization from network-facing Python workers. JSONremains available as an explicit compatibility fallback, and the payload wire
shape remains JSON-compatible.
Details
Noneannotated fields identically while retaining the original nested Python payload objectPython ↔ Rust boundary
PR #10437 added MessagePack request-plane framing, but the Python worker path still materialized every request and response as a JSON-shaped Rust value tree. MessagePack was therefore the wire codec, while
serde_json::Valueremained the in-process interchange type at the Python/Rust boundary.serde_json::Valuetree →pythonize→ Python objectserde_transcode+Pythonizer→ Python objectdepythonize→serde_json::Valuetree → JSON/MessagePack serializer → payload bytesDepythonizer+serde_transcode→ JSON/MessagePack serializer → payload bytesThe network-only
PythonPayload(Py<PyAny>)andPythonResponseItemtypes keep the payload Python-owned until the selected wire serializer consumes it. No intermediateserde_json::Value,rmpv::Value, or other Rust value tree is constructed on this network path. Annotated envelopes are inspected in place, and their nesteddataobject is passed through without rebuilding it.This is intentionally scoped to request/response payloads. Control messages, response prologues, and framing remain JSON; the supported payload domain remains JSON-compatible; and the generic typed
PythonAsyncEngineremains in place for health checks, LoRA operations, and other in-process integrations.Where should reviewer start?
lib/runtime/src/pipeline/network.rsfor the codec default and ingress adapter contract.lib/bindings/python/rust/python_payload.rsfor direct Python transcoding and annotated response handling.lib/bindings/python/rust/engine.rsfor the network-only Python engines and demand-driven generator polling.lib/runtime/src/pipeline/network/ingress/push_handler.rsfor adapter dispatch and response-stream error handling.Related Issues
No tracking issue. This is a follow-up to PR #10437.
Compatibility
Validation
Noneannotated metadata under MessagePack and JSON: passedPerformance
Controlled three-run comparison
AgentX 060526 Weka trace, concurrency 512, four typed mock workers behind a network-only Python proxy, fixed jemalloc and CPU topology, 45-second load after 32 warmups, three clean process-restarted repetitions per ref/codec pair, and zero errors across all 12 runs:
On-CPU profile impact
Matched low-overhead MessagePack profiles used
cpu-clock:uat 99 Hz with frame-pointer call graphs. Both runs completed with zero errors at essentially identical throughput (main 140.45 req/s, branch 140.08 req/s).serde_json::ValueThe profile matches the intended mechanism: removing the intermediate Rust value tree substantially reduces
serde_json::Valueconstruction/traversal and the associated allocation/deallocation work. Payload conversion remains because bytes still have to be decoded into Python objects and Python objects still have to be encoded onto the wire; the improvement is eliminating the extra Rust tree and second conversion pass. The GIL-related share is effectively flat, so the gain is primarily conversion and allocator work rather than avoiding the GIL.The controlled matrix was captured on base
fa6894c9a8before the final conflict-free rebase toccc835a80c; the four intervening main commits did not touch the request-plane or Python binding files changed here. Scripts, raw records, logs, and profile artifacts are retained locally under.bench/request-plane-msgpack-python/.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests