Repository navigation
feat(llm): add generation artifact delivery protocol (1/6) - #14977
biswapanda wants to merge 1 commit into
Conversation
Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change adds typed generation-artifact request and response support, validates delivery settings, forwards ChangesGeneration artifact flow
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable correctness, security, or availability risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description thoroughly explains the purpose, protocol changes, reviewer entry points, validation, and PR stack. However, it omits the template's required Related Issues section and does not confirm whether the PR is linked to an issue. Resolution Add the required Related Issues section. Either specify the linked issue, such as "Closes
Comment |
| pub struct ManagedFsspecTarget { | ||
| #[serde(deserialize_with = "deserialize_non_empty_string")] | ||
| pub profile: String, | ||
| #[serde(deserialize_with = "deserialize_non_empty_string")] |
There was a problem hiding this comment.
object_key is validated only as non-empty, so absolute paths and traversal keys such as /artifact or ../artifact are accepted and forwarded despite the managed-fsspec contract requiring a relative object key.
🤖 AI Fix
Add a dedicated object-key deserializer that rejects absolute paths and traversal components, and apply it to ManagedFsspecTarget::object_key.
| #[serde(deserialize_with = "deserialize_non_empty_string")] | ||
| pub profile: String, | ||
| #[serde(deserialize_with = "deserialize_non_empty_string")] | ||
| pub object_key: String, |
There was a problem hiding this comment.
managed_fsspec.object_key is documented as relative but only checked for non-emptiness. Inputs such as /other/profile/object or ../../other-profile/object are accepted and forwarded as storage control data, allowing a managed-storage backend to escape its configured profile prefix. Reject absolute keys and keys containing parent-directory traversal when deserializing this target.
🤖 AI Fix
Replace the non-empty deserializer on object_key with validation that requires a normalized relative object key and rejects absolute and parent-directory components.
| None | ||
| }; | ||
|
|
||
| let generation_artifact = if let Some(artifact) = self.generation_artifact.as_ref() { |
There was a problem hiding this comment.
The missing-receipt fallback is decided from only the current delta. A backend may emit engine_data.generation_artifact in a metadata-only chunk before its terminal finish chunk, which this frontend otherwise supports. The receipt is emitted on that earlier chunk, but the terminal chunk has no receipt and emits artifact_receipt_missing; non-streaming aggregation then overwrites the valid receipt with that synthetic failure. Retain an observed artifact receipt across deltas and synthesize the failure only when the whole response reaches a terminal finish without one.
🤖 AI Fix
Track the requested generation-artifact receipt in the response generator across deltas and emit artifact_receipt_missing only if no receipt was observed by terminal completion.
| serde_json::json!(["moe_routes"]) | ||
| ); | ||
|
|
||
| for invalid in [ |
There was a problem hiding this comment.
These invalid-contract cases combine the field being checked with an empty contents array, while the generation artifact contract is for non-empty requested contents. That makes the codec, format, URL, and byte-limit cases non-discriminating against the contents validator; the duplicate-contents case already protects the content-specific rejection. Use valid non-empty contents in the non-content invalid cases so each case exercises one observable contract.
🤖 AI Fix
Change the codec, format, presigned URL, and max_bytes invalid cases to use a representative non-empty contents array, retaining the duplicate-contents case as the content validation case.
| "nvext": { | ||
| "generation_artifact": { | ||
| "format": "generation_artifact_v1", | ||
| "contents": [], |
There was a problem hiding this comment.
This redaction test builds the artifact request with empty contents, which is outside the supported non-empty generation-artifact contract and is unrelated to the URL/header redaction behavior it verifies. The same redaction coverage remains protected with a representative requested content.
🤖 AI Fix
Replace the empty contents array with a representative non-empty content name in this test fixture.
|
will has #11470. feels similar |
Purpose
Establish the backend-neutral control-plane contract for delivering large generation artifacts out of band. Dynamo validates and forwards the delivery request and returns an opaque receipt; it does not interpret the uploaded artifact bytes or enumerate backend-specific content schemas.
What changes
nvext.generation_artifactrequest support for OpenAI requests and the Generate API.format, unique non-emptycontents,zstdcodec,object_storedelivery, and a tagged target.presigned_http_putwith URL, expiry, byte limit, required signed headers, and opaque object ID; andmanaged_fsspecwith an operator profile and relative object key.extra_args.nvext.generation_artifactwithout converting the artifact payload schema into Rust types.engine_data.generation_artifactinto the public response. Chat/completions expose it asnvext.generation_artifact; Generate exposes it asgeneration_artifact.artifact_receipt_missingfailure receipt if a backend accepted the request but returned no receipt.Contract boundary
Dynamo core is aware of delivery metadata, requested content names as opaque strings, and receipt state. It is not aware of vLLM routed-expert tensor layouts, selected-logprob arrays, or any training-experience schema. Backend-specific encoding starts in #14979, while storage I/O starts in #14980.
Response behavior
A successful backend receipt can contain
format,contents,state=ready,actual_bytes,sha256, andobject_id. Failure receipts remain structured and safe to return without exposing a presigned URL, credentials, or provider exception text.Reviewer guide
lib/llm/src/protocols/common/extensions.rs: request types, response selection, validation, and redaction.lib/llm/src/preprocessor.rs: backend passthrough.lib/llm/src/protocols/openai/generate.rs: Generate request and receipt aggregation.lib/llm/src/request_trace/payload.rs: observability redaction.Validation
cargo fmt --checkcargo test -p dynamo-llm --no-default-features generation_artifact --libPR stack
Review and merge bottom-up. Each PR is based on the preceding stack branch, so its diff contains only that layer. The original umbrella #14385 remains open and unchanged as a reference.