Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
521 changes: 508 additions & 13 deletions Cargo.lock

Large diffs are not rendered by default.

535 changes: 524 additions & 11 deletions lib/bindings/python/Cargo.lock

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions lib/llm/src/preprocessor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9025,7 +9025,8 @@ mod tests {
"cache_salt": "step_7",
"extra_fields": ["completion_token_ids"],
"metadata_upload": {
"url": "s3://bucket/root/rollouts"
"url": "s3://bucket/root/rollouts",
"fallback_url": "file:///var/tmp/rollouts"
}
}
}))
Expand All @@ -9041,7 +9042,8 @@ mod tests {
assert_eq!(
extra_args["nvext"]["metadata_upload"],
serde_json::json!({
"url": "s3://bucket/root/rollouts"
"url": "s3://bucket/root/rollouts",
"fallback_url": "file:///var/tmp/rollouts"
})
);
assert_eq!(extra_args["sampling_options"]["detokenize"], false);
Expand Down
47 changes: 42 additions & 5 deletions lib/llm/src/protocols/common/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,41 @@ pub struct RouterParams {
pub struct MetadataUpload {
#[serde(deserialize_with = "deserialize_metadata_upload_url")]
pub url: String,

#[serde(
default,
skip_serializing_if = "Option::is_none",
deserialize_with = "deserialize_metadata_upload_fallback_url"
)]
pub fallback_url: Option<String>,

@dmitry-tokarev-nv dmitry-tokarev-nv Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2. The protocol now accepts fallback_url, but the Python SGLang worker rejects it.

MetadataUpload has #[serde(deny_unknown_fields)] on both sides of this diff. Before this PR, the frontend rejected a request that carried nvext.metadata_upload.fallback_url. I did not rebuild the base tree for that half. The evidence is that the base struct has the attribute and no such field, and that this PR's own metadata_upload_parses_urls runs the identical rejection for an unknown format key. That test is green in rust-tests (.) at this SHA. This PR makes the frontend accept the field, and preprocessor.rs:1750-1757 serializes the whole struct into extra_args.nvext.metadata_upload. But 79e4ad8 reverted the matching Python change, and components/src/dynamo/common/metadata_upload.py:157-160 still allows exactly one key.

I ran this against the current file, with the exact payload the frontend forwards:

with fallback_url -> REJECTED: ValueError metadata_upload.fallback_url is not supported
control (url only)  ->  MetadataUploader(url='s3://bucket/root/rollouts')

decode_handler.py:338 calls MetadataUploader.from_backend_request(request) from generate(). So on an --enable-rl Python SGLang worker, which is still the mainline SGLang backend, the request now fails inside the worker instead of being rejected at the frontend. The wire schema advertises a field that the default backend cannot honour.

Unverified: I did not run the full frontend to worker path. I cannot state which status code the client ends up with. I only know that the worker raises.

Two options work, and either one is fine. Teach MetadataUploader.from_settings about fallback_url, because it can ignore the value. Or hold the protocol field back until the Python side lands.

One more point while you are in this file. deserialize_metadata_upload_url trims url and rejects an empty value. fallback_url gets neither. Only the sidecar validates it, so every other consumer sees it raw. The earlier revision of this PR trimmed it. The trim was lost in the scope reduction.

@Aphoh Aphoh Sep 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e818e97. fallback_url is again trimmed and rejected when empty by the shared Rust protocol. The Python parser now accepts and validates the shared field without changing its existing primary-only upload behavior, so the sidecar-only option no longer breaks the default worker.

@dmitry-tokarev-nv dmitry-tokarev-nv Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still reproduces at 66f1202529. The fix landed, and then the head commit reverted it.

e818e97754 did fix this. 66f1202529 ("keep fallback sidecar-only") reverted it:

$ git log --oneline 50485c0..66f1202 -- components/src/dynamo/common/metadata_upload.py
66f1202529 refactor(sglang): keep fallback sidecar-only     <-- removes the fix
e818e97754 fix(sglang): bound metadata upload latency       <-- adds the fix

I ran this against both trees with the exact payload the frontend forwards:

e818e97754  url + fallback_url -> ACCEPTED MetadataUploader(url='s3://b/r')
66f1202529  url + fallback_url -> REJECTED ValueError: metadata_upload.fallback_url is not supported
66f1202529  url only (control) -> ACCEPTED MetadataUploader(url='s3://bucket/root/rollouts')
66f1202529  url + format (ctl) -> REJECTED ValueError: metadata_upload.format is not supported

I also closed the half that I left unverified last round. The frontend really does forward the
field. Here is OpenAIPreprocessor::backend_extra_args at this SHA:

forwarded extra_args.nvext.metadata_upload = {"url":"s3://bucket/root/rollouts","fallback_url":"fs:///var/tmp/rollouts"}
control (url only)                         = {"url":"s3://bucket/root/rollouts"}

So on an --enable-rl Python SGLang worker, decode_handler.py:338 calls from_backend_request,
and that raises on a request the frontend accepted. I reopen the thread on that evidence.

Two notes, which are not new findings:

  • The other half of my comment is genuinely fixed. deserialize_metadata_upload_fallback_url
    trims the value and rejects an empty one. cargo test -p dynamo-llm --no-default-features --lib metadata_upload
    gives metadata_upload_parses_urls ... ok, and the probe above shows " fs:///var/tmp/rollouts "
    arriving trimmed.
  • 66f1202529 also removed the two test_sglang_decode_handler.py cases that covered
    fallback_url. That is why the Python suite is green at this SHA rather than catching this.
    Whichever way you resolve this, the contract of the parser for fallback_url wants a test.

Either direction still works. Let from_settings accept fallback_url and ignore it, or hold
fallback_url out of MetadataUpload until the Python side can honour it. The current split does
not work, because the wire schema advertises a field that the default backend rejects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still live at 66f1202529, and still live with today's main merged in. I merged main (4067d08f7e) into the head myself and measured both sides again.

Both sides of the wire on the merged tree, each with a control

Rust, from cargo test -p dynamo-llm --no-default-features:

[SUBJECT]  NvExt with fallback_url -> ACCEPTED Some(MetadataUpload { url: "s3://bucket/root/rollouts", fallback_url: Some("fs:///var/tmp/rollouts") })
[CONTROL-] NvExt with format       -> REJECTED unknown field `format`, expected `url` or `fallback_url`
[SUBJECT]  forwarded extra_args.nvext.metadata_upload = {"url":"s3://bucket/root/rollouts","fallback_url":"fs:///var/tmp/rollouts"}
[CONTROL+] forwarded extra_args.nvext.metadata_upload = {"url":"s3://bucket/root/rollouts"}

Python, from components/src/dynamo/common/metadata_upload.py. That file is the same 5334 bytes with md5 e071f0b4f8cbdc64fb5a724346ad2a8b at the merge base, at this head, and at main. So main did not change it, and this PR does not touch it.

url + fallback_url  [SUBJECT]  -> REJECTED ValueError: metadata_upload.fallback_url is not supported
url only            [CONTROL+] -> ACCEPTED MetadataUploader(url='s3://bucket/root/rollouts')
url + format        [CONTROL-] -> REJECTED ValueError: metadata_upload.format is not supported

No test covers this. The string fallback_url is in 0 Python files in the repo. I mutated the parser in both directions and ran pytest components/src/dynamo/sglang/tests/test_sglang_decode_handler.py -k metadata in the sglang runtime-test image:

parser result
unmutated 15 passed
accepts fallback_url 15 passed, nothing moved
rejects url (control) 4 failed, 11 passed

The control shows that the suite does react to a change in that function, so the middle row is a real gap and not a dead harness. I restored the file and its checksum matched the baseline.

The ask does not change. Either let from_settings accept fallback_url and ignore it, or hold the field out of MetadataUpload until the Python side can honor it.

}

fn deserialize_metadata_upload_url<'de, D>(deserializer: D) -> Result<String, D::Error>
where
D: serde::Deserializer<'de>,
{
let url = String::deserialize(deserializer)?;
normalize_metadata_upload_url(url, "metadata_upload.url")
}

fn deserialize_metadata_upload_fallback_url<'de, D>(
deserializer: D,
) -> Result<Option<String>, D::Error>
where
D: serde::Deserializer<'de>,
{
Option::<String>::deserialize(deserializer)?
.map(|url| normalize_metadata_upload_url(url, "metadata_upload.fallback_url"))
.transpose()
}

fn normalize_metadata_upload_url<E>(url: String, field: &str) -> Result<String, E>
where
E: serde::de::Error,
{
let url = url.trim();
if url.is_empty() {
return Err(serde::de::Error::custom(
"metadata_upload.url must not be empty",
));
return Err(E::custom(format!("{field} must not be empty")));
}
Ok(url.to_string())
}
Expand Down Expand Up @@ -992,16 +1015,21 @@ mod tests {
}

#[test]
fn metadata_upload_parses_url() {
fn metadata_upload_parses_urls() {
let nvext: NvExt = serde_json::from_value(serde_json::json!({
"metadata_upload": {
"url": " s3://bucket/root/rollouts "
"url": " s3://bucket/root/rollouts ",
"fallback_url": " file:///var/tmp/rollouts "
}
}))
.unwrap();

let upload = nvext.metadata_upload.as_ref().unwrap();
assert_eq!(upload.url, "s3://bucket/root/rollouts");
assert_eq!(
upload.fallback_url.as_deref(),
Some("file:///var/tmp/rollouts")
);
assert!(!NvExtResponseFieldSelection::from_nvext(Some(&nvext)).engine_data);

assert!(
Expand All @@ -1018,6 +1046,15 @@ mod tests {
}))
.is_err()
);
assert!(
serde_json::from_value::<NvExt>(serde_json::json!({
"metadata_upload": {
"url": "s3://bucket/root/rollouts",
"fallback_url": ""
}
}))
.is_err()
);
assert!(
serde_json::from_value::<NvExt>(serde_json::json!({
"metadata_upload": {
Expand Down
11 changes: 11 additions & 0 deletions lib/sidecar/sglang/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,23 @@ async-stream = { workspace = true }
async-trait = { workspace = true }
clap = { version = "4", features = ["derive", "env"] }
futures = { workspace = true }
opendal = { version = "0.59.1", default-features = false, features = [
Comment thread
Aphoh marked this conversation as resolved.
Resolved
"http-transport-reqwest",
"services-azblob",
"services-fs",
"services-gcs",
"services-s3",
] }
rand = { workspace = true }
reqwest = { workspace = true }
rmp-serde = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
tokio = { workspace = true }
tokio-util = { workspace = true }
tracing = { workspace = true }
url = { workspace = true }
zstd = "0.13"

prost = { workspace = true }
tonic = { workspace = true }
Expand All @@ -45,3 +55,4 @@ tonic-build = { workspace = true }

[dev-dependencies]
dynamo-backend-common = { workspace = true, features = ["testing"] }
tempfile = { workspace = true }
51 changes: 51 additions & 0 deletions lib/sidecar/sglang/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,57 @@ follow-up.

Use `DYN_SIDECAR_GRPC_ENDPOINT` instead of `--grpc-endpoint` when the endpoint is provided through the environment.

## Upload response metadata

Start the sidecar with `--enable-rl` or `DYN_ENABLE_RL=true` to allow response
metadata uploads. Uploads remain disabled for requests that omit
`nvext.metadata_upload`.

Set `nvext.metadata_upload.url` in an OpenAI-compatible generation request:

```json
{
"nvext": {
"metadata_upload": {
"url": "s3://bucket/root/rollout-1",
"fallback_url": "file:///var/tmp/rollout-1"
}
}
}
```

The sidecar writes `choice_0.msgpack.zst` under this URL.
The artifact uses the same MessagePack and Zstandard schema as the Python SGLang bindings.
The supported URL schemes are `file://`, `s3://`, `gs://`, and `az://`.
OpenDAL loads credentials from the standard provider configuration for the selected service.
The optional `fallback_url` can use a different supported scheme.
If the primary upload fails, the sidecar writes the same artifact to the fallback URL.

OpenDAL selects the storage service from each URL scheme.
The packaged sidecar includes the file, S3, Google Cloud Storage, and Azure Blob services.
A downstream binary can register a custom OpenDAL service before it starts the sidecar.
The custom service must use the compatible OpenDAL registry and link into that binary.
The metadata upload code does not need a change for the custom scheme.
The packaged executable does not load service plug-ins at run time.

The sidecar uploads metadata only for aggregated and decode requests.
It does not return the uploaded metadata inline.
If the final upload fails, the sidecar stops the terminal response.
Thus, the caller cannot accept a rollout without its metadata.

| Frontend API or worker role | Metadata upload support |
|---|---|
| `POST /v1/chat/completions` | Supported |
| `POST /v1/completions` | Supported |
| `POST /v1/responses` | Supported |
| Native SGLang `POST` or `PUT /generate` | Not supported; the native request adapter does not project `nvext.metadata_upload` into Dynamo request metadata |
| Aggregated worker | Uploads final `meta_info` |
| Disaggregated prefill worker | Does not upload |
| Disaggregated decode worker | Uploads final `meta_info` |

The sidecar supports one output choice, so each request writes
`choice_0.msgpack.zst`.

Native Dynamo `/generate` requests are forwarded opaquely to SGLang's HTTP
endpoint using the gRPC host and the HTTP port returned by `GetServerInfo`.
The sidecar advertises this capability only after the HTTP health probe passes
Expand Down
50 changes: 39 additions & 11 deletions lib/sidecar/sglang/src/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use tokio_util::sync::CancellationToken;

use crate::args::Args;
use crate::client::{self, Client, Discovery, Pool};
use crate::metadata_upload::{MetadataUploader, grpc_metadata};
use crate::native_http::{self, NativeHttp};
use crate::proto as pb;
use crate::protocol::{
Expand All @@ -35,6 +36,7 @@ pub struct SglangSidecarEngine {
disaggregation_mode: DisaggregationMode,
bootstrap_host: Option<String>,
bootstrap_port: Option<u16>,
metadata_upload_enabled: bool,
state: OnceCell<StartedState>,
cancel: CancellationToken,
}
Expand Down Expand Up @@ -102,6 +104,7 @@ impl SglangSidecarEngine {
);

let common = args.sidecar.common;
let metadata_upload_enabled = common.enable_rl;
let config = WorkerConfig {
namespace: common.namespace,
component: if disaggregation_mode == DisaggregationMode::Aggregated {
Expand Down Expand Up @@ -135,6 +138,7 @@ impl SglangSidecarEngine {
disaggregation_mode,
bootstrap_host,
bootstrap_port,
metadata_upload_enabled,
state: OnceCell::new(),
cancel: CancellationToken::new(),
},
Expand Down Expand Up @@ -249,6 +253,10 @@ impl LLMEngine for SglangSidecarEngine {
.state
.get()
.ok_or_else(|| client::engine_shutdown("generate called before start"))?;
let metadata_uploader = MetadataUploader::from_request(
&request,
self.metadata_upload_enabled && !self.disaggregation_mode.is_prefill(),
)?;
if let Some(native_request) = native_http::request(
&request,
ctx.id(),
Expand All @@ -261,7 +269,12 @@ impl LLMEngine for SglangSidecarEngine {
"native SGLang Generate is unavailable because no ready incremental HTTP endpoint was discovered",
)
})?;
return Ok(native_http.generate(native_request, ctx, self.cancel.clone()));
return Ok(native_http.generate(
native_request,
ctx,
self.cancel.clone(),
metadata_uploader,
));
}
let mut grpc_client = state.pool.stream_client();

Expand Down Expand Up @@ -353,14 +366,17 @@ impl LLMEngine for SglangSidecarEngine {
break;
}
};
let (log_probs, top_logprobs) =
let (log_probs, top_logprobs) = if metadata_uploader.is_some() {
(None, None)
} else {
match extract_logprobs(&response.meta_info, return_tokens_as_ids) {
Ok(values) => values,
Err(err) => {
yield Err(err);
break;
}
};
}
};

if is_prefill {
if response.finished {
Expand Down Expand Up @@ -395,12 +411,20 @@ impl LLMEngine for SglangSidecarEngine {
break;
}
};
let engine_data = match engine_data_from_meta(&response.meta_info, true) {
Ok(engine_data) => engine_data,
Err(error) => {
let engine_data = if let Some(uploader) = metadata_uploader.as_ref() {
if let Err(error) = uploader.upload(grpc_metadata(&response.meta_info)).await {
Comment thread
Aphoh marked this conversation as resolved.
Resolved
yield Err(error);
break;
}
None
} else {
match engine_data_from_meta(&response.meta_info, true) {
Ok(engine_data) => engine_data,
Err(error) => {
yield Err(error);
break;
}
}
};
terminal.token_ids = token_ids;
terminal.log_probs = log_probs;
Expand All @@ -411,11 +435,15 @@ impl LLMEngine for SglangSidecarEngine {
}

if !token_ids.is_empty() {
let engine_data = match engine_data_from_meta(&response.meta_info, false) {
Ok(engine_data) => engine_data,
Err(error) => {
yield Err(error);
break;
let engine_data = if metadata_uploader.is_some() {
None
} else {
match engine_data_from_meta(&response.meta_info, false) {
Ok(engine_data) => engine_data,
Err(error) => {
yield Err(error);
break;
}
}
};
yield Ok(LLMEngineOutput {
Expand Down
5 changes: 3 additions & 2 deletions lib/sidecar/sglang/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
//! role, parallelism, KV block sizing, and context length are discovered from
//! the engine's gRPC metadata RPCs.
//!
//! The crate never depends on `sglang` or any engine crate — only
//! `dynamo-backend-common`, `tonic`/`prost`, `clap`, and tokio.
Comment thread
Aphoh marked this conversation as resolved.
Resolved
//! The crate never depends on `sglang` or another engine crate.
//! It uses the generated protocol contract and OpenDAL storage services.

pub mod args;
pub mod client;
pub mod engine;
mod metadata_upload;
mod native_http;

/// Generated SGLang gRPC types, temporarily exposed for the Mocker server
Expand Down
Loading
Loading