Repository navigation
Conversation
Do you have examples of those? I think in @adil-a's PR #2059, he mentioned Streamed HTTP, and I also have seen SSE (from anthropic messages/v1). Did you check the differences between those options? How does vllm handle it? |
The motivation is supporting the OpenClaw agent that PinchBench runs. Currently it points to a non-gym-based model URL so we cannot capture model call records |
Yeah, I was curious about the specific repro tool example, functionally, but it seems it is not in the issue either. I was curious if you used that as a test case |
ffrujeri
left a comment
There was a problem hiding this comment.
Background
- OpenAI Chat Completions streaming (chunk objects,
data: [DONE],stream_options.include_usage): API reference; pinned chunk schema for the installed client version: openai-python v2.7.2chat_completion_chunk.py - Server-sent events: MDN primer
- Related work: #2099 (motivating issue), #2095 (the
/v1/responsessynthesized-SSE analog for Codex), #2059 (MCP streamable-HTTP — a different streaming concern: MCP tool transport, not model-inference SSE)
Where this PR sits
This changes the core model-server base class: SimpleResponsesAPIModel is extended by every model server (openai_model, vllm_model + its two local variants, azure_openai_model, inference_provider), and the /v1/chat/completions route is rebound from the subclass's chat_completions to the new chat_completions_dispatch on the hot request path.
flowchart LR
Harness[Streaming CC harness<br/>OpenClaw agent / PinchBench] -->|"POST /v1/chat/completions<br/>stream: true"| Dispatch[chat_completions_dispatch<br/>nemo_gym/base_responses_api_model.py]
Dispatch -->|sanitize + validate| CS[chat_streaming.py<br/>NEW module]
Dispatch -->|strict params| Impl["subclass chat_completions()<br/>responses_api_models/*"]
Impl -->|non-streaming call| Backend[(OpenAI / vLLM / Azure / NIM)]
Impl -->|complete reply| CS
CS -->|"synthesized SSE chunks<br/>data: [DONE]"| Harness
Dispatch -.->|observed by| Capture[model-call capture<br/>_reconstruct_chat_sse]
classDef touched fill:#ffe08a,stroke:#d48806;
class Dispatch,CS touched
Summary
Nice, well-scoped change — the buffer-then-replay approach mirrors the /v1/messages path and #2095, the new module is thoroughly documented, and the 23-test suite (with the capture reconstructor used as a fidelity oracle) is a great touch. I verified all six concrete model servers are handled by the signature dispatch, the non-streaming 200 response body is byte-for-byte unchanged, and the sync generator in StreamingResponse doesn't block the event loop (Starlette iterates it in a threadpool).
The inline comments cover one small contract regression (an extra url key in 422 details — one-line fix in two places), a question about the reachability of the extra_forbidden pruning machinery, and a capture/observability question about usage accounting on the new streaming path.
Design notes (non-blocking)
- Buffer-then-replay vs true streaming. Since the backend call completes before the first byte is emitted, clients get no incremental token timing — which seems fine for the stated goal (capturing model-call records for OpenClaw/PinchBench). Worth one line in the PR description making explicit that this is deliberately buffer-then-replay, so future readers don't expect token-level streaming semantics. Is it confirmed that OpenClaw doesn't rely on incremental chunk timing (e.g. inter-token timeouts)?
- Synthesized SSE vs proxying vLLM's native stream. vLLM supports
stream: truenatively; synthesizing from a non-streaming call instead keeps retry/error-normalization and stays consistent with/v1/messagesand #2095. Reading the PR description this looks intentional — flagging only to confirm the consistency choice over proxying was deliberate.
Other observations (no action needed)
- The
else dict(completion)branch in_invoke_chat_completions's caller (base_responses_api_model.py:141) is unreached today — all shipped servers return a validatedNeMoGymChatCompletion(whoseid/created/modelare required). A custom subclass returning a raw dict missing those keys would synthesize chunks withnullid/model/created that strict SDK clients reject mid-stream; not worth guarding now, just noting for the record. - Minor optional polish: the streaming path deep-copies the body twice (
sanitize_streaming_chat_bodyandvalidate_streaming_chat_paramseach take a defensive copy), and_invoke_chat_completionsre-inspects thechat_completionssignature per request — both consistent with the existing/v1/messagesprecedent, so fine as is. fern/doesn't document the new streaming support, but the/v1/messagesstreaming dialect is likewise undocumented, so this is consistent with current practice; could be a follow-up.
| try: | ||
| params = validate_streaming_chat_params(cleaned) | ||
| except ValidationError as exc: | ||
| raise RequestValidationError([{**error, "loc": ("body", *error["loc"])} for error in exc.errors()]) |
There was a problem hiding this comment.
nemo_gym/base_responses_api_model.py:139
Same include_url point as in _validate_chat_params below — this streaming-path 422 also gains the extra url key:
| raise RequestValidationError([{**error, "loc": ("body", *error["loc"])} for error in exc.errors()]) | |
| raise RequestValidationError( | |
| [{**error, "loc": ("body", *error["loc"])} for error in exc.errors(include_url=False)] | |
| ) |
| yield _sse_data(_chunk(completion, [delta])) | ||
|
|
||
| usage = completion.get("usage") | ||
| if include_usage and usage is not None: |
There was a problem hiding this comment.
nemo_gym/chat_streaming.py:200
Was it intentional that captured streaming records lose token accounting unless the client opts in? The usage chunk is only emitted when the client sets stream_options.include_usage (correct per the OpenAI contract), but model-call capture reconstructs the record from the outbound SSE — so for clients that don't set it (OpenClaw-style harnesses typically don't), the captured record ends up with tokens_in/out/total = None even though the backend returned usage. Since capturing model-call records is the motivating use case, it might be worth capturing usage out-of-band from the backend response rather than from the synthesized stream (always emitting the usage chunk would deviate from the OpenAI contract, so that path seems worse). A documented limitation would also be fine.
Related wording nit: the module docstring's "token-id / logprob capture on the backend response is unaffected" reads a bit stronger than what happens — captured streaming records won't include token_ids/logprobs (they don't ride in the chunk schema, as the docstring itself notes). Maybe rephrase to say this path is intended for eval-only streaming clients that don't consume those fields.
|
Tested this PR locally with curl, both with regular responses and forcing the model to use a tool calla, the SSE stream worked as expected. |
There was a problem hiding this comment.
Should we update these the docs now that Gym model servers support streaming for chat completions with this change?
responses_api_agents/pinchbench/README.md
Model wiring: OpenClaw requires a streaming-capable Chat Completions endpoint.
model_base_urlmay point directly to an upstream endpoint or to a sandbox-reachable
Gym model server, which adapts its non-streaming backend response into OpenAI-compatible SSE.
responses_api_agents/pinchbench/app.py
Policy model OpenClaw runs against. May point directly to an upstream streaming endpoint or to a sandbox-reachable Gym model server — see README.
responses_api_agents/pinchbench/configs/pinchbench.yaml
Policy model OpenClaw runs against: a direct streaming endpoint or a sandbox-reachable Gym model server. Secrets are supplied via CLI/env overrides at run time — never committed.
Blackbox harnesses that speak the OpenAI Chat Completions API over SSE (e.g. the OpenClaw agent PinchBench runs) send stream:true, which the strict NeMoGymChatCompletionCreateParamsNonStreaming model rejects. Mirror the /v1/responses streaming seam (NVIDIA-NeMo#2095) for chat/completions: a shared chat_completions_dispatch route validates requests through the same strict model (unchanged 422 behavior, including byte-for-byte detail parity via include_url=False) and, for a genuine stream:true, sanitizes the wire body, delegates to the server's existing non-streaming chat_completions(), and re-emits the complete response as chat.completion.chunk SSE terminated by data: [DONE]. The backend call stays non-streaming, so retries and error handling are unaffected. This path is for eval-only streaming clients: token ids/logprobs are not carried in the chunk schema, and a client that does not request usage (stream_options.include_usage) yields a captured record without token counts. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
5303688 to
adde70c
Compare
…VIDIA-NeMo#2130) ## Summary fixes NVIDIA-NeMo#2099 Some tools that run a model through Gym only work if the model server accepts streaming requests. They send `stream: true` on `/v1/chat/completions` and expect the reply back as a series of small events. Today Gym model servers reject `stream: true` — the request fails validation — so these tools can't point at a Gym model server and have to call the model endpoint directly. When they do that, they lose what the Gym model server adds: one consistent request format across providers, and per-call capture. The OpenClaw agent that PinchBench runs is the case that prompted this. This change makes `/v1/chat/completions` accept `stream: true`. The model server still calls its backend exactly as it does now: one normal request, wait for the complete reply. Once it has the full reply, it sends it back to the caller as a stream of standard `chat.completion.chunk` events ending with `data: [DONE]`. This is the same approach already used for `/v1/messages` (Claude Code) and, in NVIDIA-NeMo#2095, for `/v1/responses` (Codex). Because the server still waits for the whole reply before sending anything, this does not make responses arrive sooner — the only goal is to accept the streaming request shape these tools require. Two useful consequences of keeping the backend call non-streaming: the existing retry and error handling still apply (nothing has been sent to the caller yet when a failure happens), and token id / logprob capture is unchanged (streaming here is for eval-only tools, and those fields aren't part of the chunk format anyway). ## How it works - Non-streaming requests are unchanged, including the same validation errors. - For `stream: true`, the server removes the streaming-only fields (`stream`, `stream_options`) and any unknown fields the tool added, validates the rest against the normal request model, calls the server's existing `chat_completions()`, then turns the complete reply into chunks: the role, then reasoning / content / tool calls when present, then a final chunk carrying the finish reason, then a usage chunk if the caller asked for usage, then `data: [DONE]`. New file `nemo_gym/chat_streaming.py` does the request cleanup, validation, and chunk generation. The routing change is in `SimpleResponsesAPIModel`. This covers only `/v1/chat/completions`; streaming for `/v1/responses` is a separate change (NVIDIA-NeMo#2095). ## Test plan - New `tests/unit_tests/test_chat_completions_streaming.py` (23 tests): request cleanup, validation, chunk generation (content, reasoning, tool calls, usage, multiple choices, empty choices), and the route over an HTTP test client (non-streaming JSON, strict 422, streaming SSE, dropping unknown fields, invalid params → 422, and servers whose `chat_completions()` takes a leading `request` argument). Fidelity is checked by feeding the generated stream back through the capture code that rebuilds a reply from chunks. - No regressions: `test_base_responses_api_model.py` and `test_openai_utils.py` (71 passed), and the `openai_model` and `vllm_model` server suites (80 passed), which exercise both `chat_completions()` argument shapes over real HTTP. - Live check: a real uvicorn server driven by the real `openai` Python client with `stream: true` and `include_usage` rebuilt the full reply (content, reasoning, tool call, finish reason, usage) from the streamed chunks. - ruff and pre-commit are clean. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Summary
fixes #2099
Some tools that run a model through Gym only work if the model server accepts streaming requests. They send
stream: trueon/v1/chat/completionsand expect the reply back as a series of small events. Today Gym model servers rejectstream: true— the request fails validation — so these tools can't point at a Gym model server and have to call the model endpoint directly. When they do that, they lose what the Gym model server adds: one consistent request format across providers, and per-call capture. The OpenClaw agent that PinchBench runs is the case that prompted this.This change makes
/v1/chat/completionsacceptstream: true. The model server still calls its backend exactly as it does now: one normal request, wait for the complete reply. Once it has the full reply, it sends it back to the caller as a stream of standardchat.completion.chunkevents ending withdata: [DONE]. This is the same approach already used for/v1/messages(Claude Code) and, in #2095, for/v1/responses(Codex).Because the server still waits for the whole reply before sending anything, this does not make responses arrive sooner — the only goal is to accept the streaming request shape these tools require. Two useful consequences of keeping the backend call non-streaming: the existing retry and error handling still apply (nothing has been sent to the caller yet when a failure happens), and token id / logprob capture is unchanged (streaming here is for eval-only tools, and those fields aren't part of the chunk format anyway).
How it works
stream: true, the server removes the streaming-only fields (stream,stream_options) and any unknown fields the tool added, validates the rest against the normal request model, calls the server's existingchat_completions(), then turns the complete reply into chunks: the role, then reasoning / content / tool calls when present, then a final chunk carrying the finish reason, then a usage chunk if the caller asked for usage, thendata: [DONE].New file
nemo_gym/chat_streaming.pydoes the request cleanup, validation, and chunk generation. The routing change is inSimpleResponsesAPIModel. This covers only/v1/chat/completions; streaming for/v1/responsesis a separate change (#2095).Test plan
tests/unit_tests/test_chat_completions_streaming.py(23 tests): request cleanup, validation, chunk generation (content, reasoning, tool calls, usage, multiple choices, empty choices), and the route over an HTTP test client (non-streaming JSON, strict 422, streaming SSE, dropping unknown fields, invalid params → 422, and servers whosechat_completions()takes a leadingrequestargument). Fidelity is checked by feeding the generated stream back through the capture code that rebuilds a reply from chunks.test_base_responses_api_model.pyandtest_openai_utils.py(71 passed), and theopenai_modelandvllm_modelserver suites (80 passed), which exercise bothchat_completions()argument shapes over real HTTP.openaiPython client withstream: trueandinclude_usagerebuilt the full reply (content, reasoning, tool call, finish reason, usage) from the streamed chunks.