feat(vllm-model): supply the previous call's exact training tokens - #2181
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
e32bd85 to
3346a5b
Compare
3346a5b to
bca52ad
Compare
dca43c5 to
93decff
Compare
0954c70 to
35ef310
Compare
35ef310 to
ccce798
Compare
93be757 to
c9d74e7
Compare
c9d74e7 to
79a1883
Compare
|
/claude review |
|
SHIP — no correctness or async-safety concerns. Reviewed the full diff plus the PR-head What I checked and why it's safe:
Two NOTEs (author's call, both fail safe, not merge-blocking):
Test coverage is real behavioral assertions, not pass-throughs. LGTM. |
|
/claude review |
|
Review incomplete: the PR revision changed during analysis. The base SHA captured at the start of this review ( |
|
/claude review |
1 similar comment
|
/claude review |
|
SHIP WITH CARE — one reliability risk worth resolving before this runs on deep rollouts. This adds delta (parent-relative suffix) token storage and vLLM prefix supply, both fail-closed by design: prefix supply requires generation-time One finding (inline, RISK):
No async-client, |
| def cum_of(call_id: str, walking: set[str]) -> list[int] | None: | ||
| if call_id in cumulative: | ||
| return cumulative[call_id] | ||
| entry = by_id.get(call_id) | ||
| if entry is None or call_id in walking: | ||
| cumulative[call_id] = None | ||
| return None | ||
| walking.add(call_id) | ||
| if not entry.prompt_is_delta: | ||
| value = list(entry.prompt_token_ids) + list(entry.generation_token_ids) | ||
| else: | ||
| parent_cum = cum_of(entry.parent_call_id, walking) if entry.parent_call_id else None | ||
| value = ( | ||
| None | ||
| if parent_cum is None | ||
| else parent_cum + list(entry.prompt_token_ids) + list(entry.generation_token_ids) | ||
| ) | ||
| walking.discard(call_id) | ||
| cumulative[call_id] = value | ||
| return value |
There was a problem hiding this comment.
RISK — cum_of recurses once per delta ancestor, so recursion depth equals the delta-chain length. With delta_records=true, every RESOLVED continuation in a rollout is a delta, so a long multi-call agent rollout (hundreds+ of sequential model calls) produces a chain deep enough to hit Python's recursion limit (~1000).
What breaks: RecursionError (a RuntimeError) raised here. _assemble (consumer.py:126) only catches (AssertionError, ValueError, KeyError, IndexError, TypeError), and trajectories_from_source runs _assemble via asyncio.to_thread outside its try/except (consumer.py:261-270). So RecursionError escapes the "mask instead of fail" contract and fails the whole rollout / training batch — the exact outcome consumer.py:115 warns against.
Blast radius: any run with delta_records enabled and deep sequential rollouts; silent until a long trajectory trips it, then it takes down the batch rather than masking one sample.
This is a known foot-gun in this very file: path_to (line 358) was made iterative precisely because "agent rollouts can exceed the recursion limit," and the sibling delta-walk in FileLineageStore._materialize (lineage.py:619-636) is iterative with an explicit depth > 10_000 cap. cum_of should follow the same iterative pattern with a depth guard.
There was a problem hiding this comment.
Fixed in 57ef86c. Delta materialization now walks parent links iteratively with cycle detection and the same 10,000-link depth bound used by the lineage store. A 1,100-turn delta-chain regression test verifies this path no longer depends on Python recursion.
|
🌿 Preview your docs: https://nvidia-preview-ananthsub-tokidcap-supplying.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
Supply a verified parent's cumulative tokens to compatible generation backends and require generation-time proof before recording successful application. Store resolved prompts as bounded, reconstructible deltas so long external-harness trajectories remain exact without quadratic storage or materialization. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Supplies a request-time resolved parent's exact cumulative tokens to compatible vLLM generation backends, verifies generation-time proof, and adds delta storage in the initial schema-v1 contract for long multi-call rollouts.
Prefix supply and compact publication
sequenceDiagram participant H as Harness participant M as Gym vLLM model server participant L as LineageStore participant V as vLLM backend participant S as TokenSink H->>M: continuation request M->>L: resolve(request items as received) alt RESOLVED L-->>M: parent call and exact cumulative tokens M->>V: generation request with required_prefix_token_ids V-->>M: response and generation-time prompt_token_ids M->>M: verify observed prompt starts with requested prefix M->>M: retain full cumulative digest and replace prompt with parent-relative suffix when enabled M->>S: put(TokenEntry, prefix_requested=true, prefix_supplied=true, prompt_is_delta=optional) else ROOT or UNRESOLVED L-->>M: no proven parent tokens M->>V: ordinary generation request V-->>M: response M->>S: put(full-prompt TokenEntry, prefix_requested=false) endPrefix intent and proof are separate persisted facts. Setting
required_prefix_token_idsis not proof that the engine used them.prefix_suppliedbecomes true only after the generation response demonstrates that the served prompt extended the exact requested tokens.Summary
RESOLVEDlineage result computed before request conversion.ROOT,UNRESOLVED, and lookup failures leave the ordinary generation request unchanged.prefix_requestedseparately fromprefix_suppliedand keeps concurrency-safe eligible, requested, and successful supply diagnostics.TokenEntryat initial schema version 1; prefix evidence and parent-relative prompt deltas are part of that unreleased contract. Roots and unresolved calls remain full-prompt anchors.cum_lenanddigestdefined over the full cumulative sequence even when storage contains only a suffix. The builder and incremental resolver walk parent links and verify each reconstructed sequence before use.Builds on the lineage resolver merged in #2180. Final integration documentation and adapter coverage are in #2349.