Repository navigation
Conversation
ananthsub
left a comment
There was a problem hiding this comment.
I think this should use the post-conversion finalization design from #3050 instead of inferring the served dialect inside chat_completions().
There is a concrete /v1/messages case that the current approach misses. When vLLM returns a refusal, as_responses=True makes _finalize_external_capture() record a Responses content block shaped like {"type": "refusal", "refusal": "..."}.
Anthropic egress serves that refusal as an ordinary {"type": "text", "text": "..."} block. When the client echoes the unchanged Messages response, ingress converts it back to assistant text. The fingerprints differ because content-block type participates in canonicalization, so the valid continuation becomes unresolved_parent. The new test matrix does not include refusals.
The final commit in #3050 retained the worker acknowledgement on the request-scoped capture context, then finalized capture after Chat, Responses, or Messages conversion had produced the object being returned to the client. That makes capture fingerprint the actual served representation, handles transformations such as refusal-to-text naturally, and avoids maintaining a second conversion path selected from request.url.path.
Could we port that finalization design onto current main, retain this PR's HTTP regression matrix, and add an Anthropic refusal continuation case? A conversion-failure case would also be useful to verify that a response which was never served does not commit a lineage row.
|
Thanks @ananthsub — you're right. I reproduced the unchanged Messages refusal continuation failing with Addressed in 612723a by adapting your post-conversion finalization design from #3050 (4c67a86), with attribution in the commit:
The original 24 HTTP reasoning/plain-text cases are retained. Added four Responses/Messages refusal continuation cases and five conversion/serialization-failure cases. The tests check single-commit behavior, served response IDs, and that a failure after worker staging leaves no successful lineage row and marks capture uncommitted. Validation: 811 tests passed, including all 35 focused integration tests; This ports only the finalization change, not #3050's inherited infrastructure. The fingerprint algorithm, admission rules, dependencies, and lockfiles remain unchanged. Could you take another look? |
|
/ok to test 612723a |
Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
Adapt the post-conversion finalization design from Ananth Subramaniam in NVIDIA-NeMo#3050, commit 4c67a86. Retain the worker acknowledgement in request-scoped context and finalize after conversion and serialization. Add refusal, failure, identity and single-commit HTTP regressions. Co-authored-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com>
612723a to
b5644eb
Compare
|
/ok to test b5644eb |
Resolve the conflict in responses_api_models/vllm_model/app.py between this PR's ExternalCaptureHandler extraction and main's NVIDIA-NeMo#3301 (preserve ancestry for reasoning Responses). The handler now owns main's two-phase lifecycle: a sync prepare_response strips transport fields and parks the worker acknowledgement on the capture context at chat-completion time, and finalize_response records lineage from the final served Chat, Responses, or Messages representation via _finalize_served_response. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
Extend #3301's finalization boundary to buffered SSE and the compaction adapter. Serialize the client-visible response before committing capture, and persist uncommitted-call failures before releasing terminal events. Restore namespaced tool output before finalization and preserve namespace identity in version 2 fingerprints. Add refusal, conversion, serialization, content-attribution, and failure-durability regressions. Validation: 1,084 Gym tests and 32 NeMo-RL boundary tests pass. A live Qwen3 rollout through the NeMo-RL worker, vLLM, and TransferQueue returns reward 1.0, reconstructs 238 tokens, and attributes the terminal call by content (Slurm job 3772908). Full pre-commit passes; Fern reports zero errors and an unauthenticated redirect-check warning. Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
## What does this PR do? Fix external token-capture ancestry by recording the final API response representation, after Chat, Responses, or Anthropic Messages conversion and JSON serialization succeed. The original reasoning continuation bug reproduces on main at `9506eb87c99ffa0f736b05f76b8f20dca8fb479b`: capture fingerprints an internal Chat assistant message with wrapped reasoning, while the Responses client echoes separate reasoning and answer items. Generation returns HTTP 200, but capture is poisoned with `unresolved_parent`. Following [Ananth's review](#3301 (review)), this revision replaces the initial URL-selected conversion workaround with the post-conversion design from [his final commit in #3050](4c67a86): - Retain worker commit coordinates privately in the existing request-scoped capture context; immediately strip transport fields from the internal response. - Finalize once at the outer Chat, Responses, or Messages dispatch boundary, after conversion and JSON serialization. - Normalize served Anthropic thinking blocks into standalone reasoning items when constructing capture output items. - Preserve the existing worker-acknowledgement validation, token-free ledger, response-ID association, and poison-and-serve capture behavior. This also fixes the reviewer's refusal example: Responses refusal content becomes ordinary text in Messages, so fingerprinting the intermediate Responses representation is wrong. Conversion/serialization failures now leave no successful lineage row; the existing middleware marks the admitted call uncommitted. Finalization before HTTP send is not a guarantee of client receipt. No fingerprint algorithm, admission rules, token-staging protocol, dependencies, lockfiles, or agent implementations change. Only the finalization portion of #3050 is adapted, not its inherited capture infrastructure. Refusal-only direct Chat ambiguity under the existing fingerprint rules is outside this change; the original direct Chat reasoning/plain-text controls remain covered. ## Validation - Retained the original 24 three-turn HTTP cases: Responses, Chat Completions, and Messages; plain answers and three reasoning representations; with and without tool calls. - Added four refusal continuation cases for Responses and Messages, with and without tool calls. - Added five failure cases: Responses/Messages conversion and JSON serialization on all three APIs. Worker staging must already have succeeded, no lineage commit may occur, and capture must be marked uncommitted. - Continuation cases check exact parent IDs and cumulative prefix lengths, one ledger commit per call, the served response ID, token-light output, and rejection of subsequently altered history. - Before the revision, the Messages refusal and post-staging conversion-failure regressions failed as reported. The original reasoning regressions also remain regression-tested against the original main baseline (12 failed, 12 passed before the original fix). - After the revision: **811 passed** across vLLM model tests, Responses/base-model/Anthropic converters, and token-capture suites. The focused integration file has **35 passing tests**. - `pre-commit run --all-files` passes. These tests use the real HTTP dispatch, middleware, converters, worker capture helper, file ledger, and parent admission. Generation and staging storage are CPU doubles; no live vLLM/GPU validation of this revision is claimed. ```bash python -m pytest responses_api_models/vllm_model/tests/test_token_capture_integration.py ``` ## Checklist - [x] Contributing guidelines followed; focused changes only. - [x] Regression tests and local pre-commit checks pass. - [x] DCO sign-off; adapted implementation credits Ananth Subramaniam. --------- Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com> Co-authored-by: Ananth Subramaniam <ansubramania@nvidia.com> (cherry picked from commit 295e2e6)
Pass immutable local tensor attachments through worker capture and preserve image descriptors in vLLM extras. Retain stage-before-ack ordering and token-only sink compatibility. Builds on the served-response ancestry fix from NVIDIA-NeMo#3301 and the VLM capture work in NVIDIA-NeMo#3368. Signed-off-by: Pranav Prashant Thombre <pthombre@nvidia.com>
…DIA-NeMo#3670) ## What does this PR do? Fixes external token capture when Gym serves an empty synthetic completion after a vLLM context/token-limit error or the sequential-reasoning guard. The served-response finalization introduced in NVIDIA-NeMo#3301 correctly commits fingerprints after API conversion, but it also runs for responses that did not come from a normal worker completion. Two existing paths hit this case: - A backend HTTP 400 for context length or `max_tokens` is converted into an empty completion with `finish_reason="length"`. - With `sequential_reasoning_allowed=false`, a continuation that hits the reasoning guard returns an empty completion with `finish_reason="content_filter"`. Neither path reaches the worker-response preparation hook, so neither has worker commit coordinates. Finalization currently records `worker_response_missing_commit_coordinates`, which can poison the entire rollout and discard valid tokens captured in earlier turns. For example, a successful tool-use turn followed by a context-limit completion loses its already-staged training row. This PR adds a request-scoped `external_worker_response_seen` flag and sets it only when a normal worker completion reaches `prepare_response()`. If no worker completion was received, the shared finalizer returns without committing or poisoning the call. Existing middleware still records `request_finished_without_staged_coordinates`; a consumer selecting the preceding committed terminal can retain the valid token chain. The fix lives in the shared handler introduced by NVIDIA-NeMo#2823 and covers both vLLM and Megatron worker capture. A real worker completion missing its acknowledgement still fails with `worker_response_missing_commit_coordinates`; malformed acknowledgements and explicit worker capture failures retain their existing behavior. Sending a request alone does not set the flag, because the backend may return a context-limit error. Adds 44 regression cases covering both backends, JSON and buffered SSE, Chat Completions/Responses/Messages/compaction routes, overflow before or after a committed call, and reasoning-only continuations. The tests verify lineage, preserved tokens/masks/logprobs, request isolation, and transport-field stripping. A synthetic response explicitly selected as the terminal remains unattributable; an initial overflow creates no trainable row. No separate issue is needed for this focused correction to the existing capture lifecycle; the reproduction and affected behavior are documented here. ## Validation - Slurm job **3966743**: **1,055 tests passed**, zero failures/errors/skips, using container-native Python 3.13.14 in the requested NeMo RL nightly image. Both changed test files were also overlaid on the unmodified base: all **44 new regression cases failed** with the missing-coordinates behavior before the production fix. - Ruff lint, formatting, and changed-file pre-commit passed in that job. Targeted coverage exercised every added production line and both outcomes of the new finalizer guard. - `pre-commit run --all-files` passed before publishing, and `git diff --check` passed. - CI classification is **full** because shared core code changes. The full core/sandbox coverage gate and eight-shard server suite were not run locally; the focused capture/model-server suite and real rollouts were used for this patch. Targeted coverage does not establish the repository-wide coverage threshold. Container used for both Slurm jobs: ```text /lustre/fsw/portfolios/nemotron/projects/nemotron_sw_post/users/pthombre/enroot-images/nvcr.io+nvidian+nemo-rl+nightly.squashfs ``` <details> <summary>Validation commands and local artifacts</summary> Job submissions from the fix worktree: ```bash sbatch cache/worker-bypass-validation/native-validation.sbatch sbatch cache/worker-bypass-validation/smoke.sbatch ``` The native validation runner invokes pytest through a bootstrap that selects the current checkout and asserts the imported Gym/model-server code comes from that checkout. Its passing test command uses this selection and configuration: ```bash python cache/worker-bypass-validation/pytest_bootstrap.py \ -o addopts= -o log_cli=false --import-mode=importlib -q --tb=short \ tests/unit_tests/test_external_capture_handlers.py \ tests/unit_tests/test_base_responses_api_model.py \ tests/unit_tests/test_chat_completions_streaming.py \ tests/unit_tests/test_responses_api_model_streaming.py \ tests/unit_tests/test_token_id_capture.py \ tests/unit_tests/test_token_capture*.py \ responses_api_models/vllm_model/tests \ responses_api_models/vllm_model_with_compaction/tests \ --cov=nemo_gym.token_id_capture.sink \ --cov=nemo_gym.token_id_capture.external_capture \ --cov-report=term-missing --cov-fail-under=0 \ --cov-config=cache/worker-bypass-validation/coverage.ini \ --cov-report=xml:cache/worker-bypass-validation/coverage.xml \ --junitxml=cache/worker-bypass-validation/regressions.xml ``` The temporary baseline checkout runs the two changed test files with `-k synthetic_completion`. The GPU smoke job runs `python cache/worker-bypass-validation/real_worker_smoke.py`. All-files hooks were invoked with: ```bash PRE_COMMIT_HOME="$PWD/cache/worker-bypass-validation/pr-pre-commit" \ UV_CACHE_DIR="$PWD/cache/worker-bypass-validation/uv-cache" \ uv tool run --python /cm/local/apps/python3/bin/python3 --from pre-commit \ pre-commit run --all-files git diff --check ``` Validation scripts, logs, JUnit XML, coverage, and rollout evidence are local artifacts, not committed source. They are retained under: ```text /scratch/fsw/portfolios/nemotron/projects/nemotron_sw_post/users/pthombre/GymPR/Gym/.worktrees/capture-worker-bypass/cache/worker-bypass-validation/ ``` Key artifacts: `results.json`, `baseline.xml`, `regressions.xml`, `pre-commit-all-files.log`, and `real-rollout/evidence.json`. </details> ## Rollouts Slurm GPU job **3966328** completed successfully using **Qwen/Qwen3-0.6B**, vLLM 0.25.1, and the real NeMo RL HTTP worker with local durable staging, served through Gym Responses SSE: - A two-turn `add(2, 2)` tool rollout returned `4`, passed the smoke verifier (reward **1.0**), and produced two chained committed calls: **234 total tokens**, including **27 training tokens**, with finite logprobs. - A subsequent real backend context-overflow error left those committed records and the reconstructed training row unchanged, adding only the uncommitted-call record. - A separate reasoning-only generation followed by the reasoning guard retained all **16 generated tokens**, again adding only the uncommitted-call record. This is a vLLM real-model smoke test; Megatron behavior is covered by the parameterized handler and route tests, not a live Megatron deployment. ## Compatibility and benchmark impact No public API, configuration, ledger/staging schema, or migration changes. Synthetic API responses keep their existing finish reasons. Valid earlier generations can remain trainable when the rollout ends in a synthetic completion, eliminating this source of dropped training rows. Missing acknowledgements from actual worker completions still fail closed. Benchmark scoring logic is unchanged. Documentation: N/A; this restores existing capture behavior without adding user configuration or a public interface. ## Checklist - [x] I have read the contributing guidelines. - [x] The change is focused; no unrelated edits. - [x] Tests added and the focused suite passes. - [x] Documentation assessed: N/A, as explained above. - [x] `pre-commit run --all-files` passes. - [x] No new source files; existing SPDX headers retained. - [x] All commits have DCO sign-off. --------- Signed-off-by: Pranav Thombre <pthombre@nvidia.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Integrate minimal CC onto NVIDIA-NeMo/Gym main 4402098. Includes post-conversion finalization from NVIDIA-NeMo#3301 (612723a), adapted from Ananth Subramaniam in NVIDIA-NeMo#3050. Preserve main retry bounds and error propagation. Accept framework attempt-scoped owner IDs without dropping their scope. No private validation machinery or lockfile changes. Co-authored-by: Ananth Subramaniam <ansubramania@nvidia.com> Signed-off-by: Ali Roshan Ghias <aroshanghias@nvidia.com> Signed-off-by: Linglin Jing <linglinj@oci-nrt-cs-001-vscode-01.cm.cluster>
… ids (NVIDIA-NeMo#4014) ## What does this PR do? Preserve token-capture ancestry when a harness echoes a complete served model turn but rewrites its tool arguments, as Claude Code does for Bash commands and Edit defaults. The fallback requires matching tool IDs, names, order and non-tool assistant content, plus the recorded request context; inserted, missing or changed model output and ambiguous token sequences remain unresolved. Both local token logs and external-staging custody ledgers now retain this complete-turn key, return `tool_call_turn` when it supplies the parent, and reuse the parent's original staged tokens. Existing custody rows without the new optional key remain strict-only, identical replays remain idempotent, and checkpoint import/export preserves new keys. This deliberately narrows NVIDIA-NeMo#3301's rejection of edited echoes to permit argument-only rewrites while continuing to reject text, tool-name, ID and item changes. Terminal attribution skips token-free custody rows for the argument-insensitive reading, rejects IDs reused in earlier transcript turns, and treats interleaved reasoning consistently. Strict witnesses still take precedence, conflicting witnesses fail closed, and the merge with NVIDIA-NeMo#3973 preserves the distinction between a uniquely identified serving and token-identical retries with different finish reasons. Custody-only terminal attribution still relies on its existing IDs/fingerprints; this PR does not add output items to custody records. The file-ledger implementation in NVIDIA-NeMo#4553 should be updated to use this complete-turn key when that branch integrates NVIDIA-NeMo#4014, rather than reintroducing its older ID-only match. No separate issue is needed for the defects addressed by the inline review. ## Validation - Source-bound native Python 3.13 run: **1,425 passed**, covering `test_tool_call_turn_fallback.py`, `test_tool_call_turn_lineage_store.py`, `test_token_id_capture.py`, `test_terminal_attribution.py`, `test_conversation_fingerprint.py`, `test_external_capture_handlers.py`, `test_trajectory_builder.py`, all `test_token_capture_*.py`, and the complete `responses_api_models/vllm_model/tests/` suite. - New regression controls: **9 failed before / passed after** for interleaved reasoning and reused terminal IDs; one additional finish-reason identity regression failed before the main-integration fix. Tests also exercise durable-store restart/checkpoint compatibility, strict precedence, ambiguity, identical retries, version guards, and HTTP continuations consuming the exact original staged token prefix. - `pre-commit run --all-files` and `git diff --check`: passed; the private guardwords catalog was unavailable, so that hook skipped its private-catalog scan. The formatter changed one test file; its 65 cases passed again on final head `a791afff6f875a53908420df5198cf30a9feeab7`. ## Rollouts and compatibility No fresh live-model rollout was run for this revision; HTTP routing, conversion, lineage, admission and worker capture helpers are exercised with CPU generation/staging doubles. The earlier report of several thousand Claude Code training rollouts is historical author-provided evidence, not validation of this revised head. Argument-only echoes may become trainable where they previously failed attribution; this changes sample eligibility, not verifier scores, and requires Ananth's review and required CI before merge. --------- Signed-off-by: Lin Jia <linj@nvidia.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Pranav Prashant Thombre <pthombre@nvidia.com>
What does this PR do?
Fix external token-capture ancestry by recording the final API response representation, after Chat, Responses, or Anthropic Messages conversion and JSON serialization succeed.
The original reasoning continuation bug reproduces on main at
9506eb87c99ffa0f736b05f76b8f20dca8fb479b: capture fingerprints an internal Chat assistant message with wrapped reasoning, while the Responses client echoes separate reasoning and answer items. Generation returns HTTP 200, but capture is poisoned withunresolved_parent.Following Ananth's review, this revision replaces the initial URL-selected conversion workaround with the post-conversion design from his final commit in #3050:
This also fixes the reviewer's refusal example: Responses refusal content becomes ordinary text in Messages, so fingerprinting the intermediate Responses representation is wrong. Conversion/serialization failures now leave no successful lineage row; the existing middleware marks the admitted call uncommitted. Finalization before HTTP send is not a guarantee of client receipt.
No fingerprint algorithm, admission rules, token-staging protocol, dependencies, lockfiles, or agent implementations change. Only the finalization portion of #3050 is adapted, not its inherited capture infrastructure. Refusal-only direct Chat ambiguity under the existing fingerprint rules is outside this change; the original direct Chat reasoning/plain-text controls remain covered.
Validation
pre-commit run --all-filespasses.These tests use the real HTTP dispatch, middleware, converters, worker capture helper, file ledger, and parent admission. Generation and staging storage are CPU doubles; no live vLLM/GPU validation of this revision is claimed.
Checklist