Skip to content

feat(token-id-capture): resolve each call's parent at request time - #2180

Open
ananthsub wants to merge 12 commits into
mainfrom
ananthsub/tokidcap/parent-index
Open

feat(token-id-capture): resolve each call's parent at request time#2180
ananthsub wants to merge 12 commits into
mainfrom
ananthsub/tokidcap/parent-index

Conversation

@ananthsub

@ananthsub ananthsub commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Resolves each model call's parent at request time from entries already committed by TokenSink.put. It does not create a second lineage write path.

Multi-worker publication and resolution

sequenceDiagram
    participant H as Harness
    participant W1 as Model worker 1
    participant B as Shared token backend
    participant W2 as Model worker 2

    H->>W1: first request without model-authored history
    W1->>B: resolve(request items)
    B-->>W1: ROOT
    W1->>W1: run inference and stamp ROOT and continuation metadata
    W1->>B: TokenSink.put(TokenEntry)
    B-->>W1: record durable and resolver-visible
    W1-->>H: response A

    H->>W2: continuation echoing response A
    W2->>B: fetch newly committed entry metadata
    W2->>W2: refresh bounded metadata-only index
    W2->>W2: fingerprint lookup and context-digest verification
    alt one token-identity candidate
        W2->>B: lazily load winning call tokens
        B-->>W2: winning TokenEntry and ancestors when needed
        W2->>W2: verify cumulative digest and mark record RESOLVED
    else no safe unique candidate
        W2->>W2: record UNRESOLVED(reason)
    end
    W2->>W2: run inference and stamp the child decision
    W2->>B: TokenSink.put(child TokenEntry)
    W2-->>H: response B
Loading

ROOT, RESOLVED, and UNRESOLVED are the only outcome stages. Diagnostic reasons are persisted for analysis but do not weaken reconstruction behavior.

Summary

  • Persists the request-time resolution outcome, verified parent identity, diagnostic reason, fingerprint version, and compact continuation lookup metadata on each TokenEntry (schema version 3).
  • Keeps TokenSink.put(entry) as the single durable publication boundary. LineageStore is a read-only view over sink-committed entries and has no served-path record() operation.
  • Extracts IncrementalLineageStore for file, queue, KV, or service adapters. Backends implement entry refresh and lazy load hooks while inheriting Gym's canonical matcher, locking, bounded LRU, and digest checks.
  • Maintains metadata-only lineage nodes and loads cumulative token arrays only for the winning parent. Cache eviction causes a cold refetch rather than loss of lineage correctness.
  • Uses a fixed set of striped in-process locks to serialize same-rollout refresh without growing lock metadata per rollout. The file backend also uses the token store's per-rollout flock for cross-process visibility.
  • Collapses retries only when candidates have identical cumulative token identity. Candidates with different digests remain ambiguous and resolve UNRESOLVED.
  • Canonically hashes structured request content, tool-call and result identities, and multimodal blocks with versioned, length-delimited fields. Unsupported items and mismatched fingerprint versions fail closed; golden vectors pin the cross-dialect contract.
  • Reconstructs ROOT as a valid chain start and follows only verified RESOLVED links. UNRESOLVED, a digest conflict, or missing resolution metadata creates a masked fragment. Prefix matching is retained only to recover a recorded RESOLVED link whose direct parent is absent from the current build.
  • Requires a custom sink to have a resolver over the same backend namespace unless the operator explicitly accepts unresolved continuations. Multi-worker startup also requires process-shared resolver behavior.
  • Exposes worker-level resolution and capture-failure counters and preserves per-record reasons for offline diagnosis.

Depends on #2341. Required by #2181.

@copy-pr-bot

copy-pr-bot Bot commented Jul 29, 2026

Copy link
Copy Markdown

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.

@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch from ecdccec to 3d810f7 Compare July 29, 2026 12:38
@ananthsub
ananthsub changed the base branch from ananthsub/tokidcap/side-calls to ananthsub/tokidcap/delivery July 29, 2026 12:40
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch from 3d810f7 to 2dd3cc3 Compare July 29, 2026 13:14
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch from 2dd3cc3 to 5815504 Compare July 29, 2026 13:41
@ananthsub
ananthsub marked this pull request as ready for review July 29, 2026 13:57
@ananthsub
ananthsub requested a review from pthombre July 29, 2026 13:59
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch from 5815504 to a9a579d Compare July 29, 2026 16:34
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Jul 30, 2026
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch from a9a579d to 7219fc9 Compare July 30, 2026 22:19
@ananthsub
ananthsub force-pushed the ananthsub/tokidcap/parent-index branch 2 times, most recently from 32711d6 to b117be3 Compare July 31, 2026 01:57
@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

1 similar comment
@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Comment thread nemo_gym/token_id_capture/lineage.py Outdated
cmunley1
cmunley1 previously approved these changes Aug 6, 2026

@cmunley1 cmunley1 left a comment

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.

left a comment but dont think it is blocking, more of a unlikely edge case

Comment thread nemo_gym/token_id_capture/lineage.py Outdated
Comment thread nemo_gym/token_id_capture/records.py Outdated
@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

Comment thread nemo_gym/token_id_capture/builder.py
Comment thread nemo_gym/base_responses_api_model.py Outdated
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

SHIP WITH CARE

Request-time parent lineage for token capture: a shared lineage store resolves which recorded call a request continues (fingerprint over model-authored turns, verified by a conversation digest), stamps parent_call_id/cum_len/digest on each TokenEntry, and the builder prefers the verified link over prefix matching. The design is sound and the test coverage is genuinely strong — dialect-agnostic fingerprinting, ambiguous-retry refusal, tree-forks, eviction bounds, and cross-process file resolution are all exercised with assertions on real behavior. The verifier-adjacent contract ("degrade to fallback, never a wrong answer") is the right one, and parent_digest_mismatch correctly quarantines to avoid merging two attempts.

Two findings, both inline, neither a hard blocker but both worth resolving before this drives training data at scale:

  1. builder.py:152 (RISK)parent_call_id_missing quarantines the node + subtree instead of falling back to _infer_parent, contradicting the module docstring and silently dropping chains the old builder kept (e.g. when the recorded parent was an empty-generation call, which is filtered out of entries before by_call_id is built). Fall back on the missing-parent reason; reserve quarantine for digest-mismatch.

  2. base_responses_api_model.py:1410 (RISK) — the now-default FileLineageStore does read-all + full-file-rewrite + double-fsync per model call, embedding full cumulative_token_ids, giving O(calls² × context) write amplification on the hot path — on the shared/Lustre filesystems store.py already flags as a throughput cliff. And sink.parent_tokens (the only consumer of those persisted tokens) is set but never read. Drop cumulative_token_ids from the file records until a consumer exists; the builder verifies via the TokenEntry digest, not the store.

Also confirm the num_workers>1 guard has been exercised with a real multi-worker rollout, not just unit tests — the CLAUDE.md bar for capture/training changes is real rollouts, and the file-lock + read-after-write assumption across uvicorn workers is exactly the kind of thing green unit tests do not prove.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Review incomplete: the PR base SHA changed during review (captured merge-base 89b175cfa31ef13444421aeddf0767c6e63d4676, now b37601cc37cee71e153360f5722cf6613b4c1233). The immutable diff I analyzed no longer matches the current PR revision, so ./review-context/check-pr-revision.sh fails the pre-publish check. No inline findings are being posted to avoid anchoring comments to stale lines. Please re-trigger the review against the current head so it runs on an up-to-date diff.

@ananthsub

Copy link
Copy Markdown
Contributor Author

/claude review

Identifies which recorded call a request continues, from what the harness already
sends: hashing the model-authored turns finds the call that produced the last one,
and the conversation those turns arrived with is verified before the match is used,
so a compacted or rewritten history does not resolve.

The record gains parent_call_id, cum_len and digest, and the builder prefers a
verified link over inferring one from token prefixes, which makes a chain exact
rather than inferred and tells a retry from the call the harness kept.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
…t messages alone

The parent check read only the message list. Anthropic sends the system prompt as
`system` and the tool schema as `tools`, both siblings of that list, and the chat
template renders both into the prompt. A harness that changed either mid-rollout,
which Claude Code does on a plan-mode toggle or an MCP attach, still matched, so the
call could be handed a prefix rendered under instructions or tools the harness had
already replaced.

Carry both as a leading pseudo-turn so conversation_digest covers them. Its role is
not `assistant`, so the fingerprint the index keys on is unchanged. Tools are
normalized to plain data first, since they arrive as dicts on one call and as models
on the next and two strings for one schema would break a chain that never changed.

Also fail closed when a node has no recorded context digest rather than treating the
request as a continuation.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Expose a transport-neutral lineage protocol, use a locked file implementation for Gym-local capture, and require external multi-worker deployments to configure a shared adapter instead of silently losing parent links.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Require adapter implementations to fail closed on ambiguity and make repeated lineage publication idempotent across local and external stores.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Append shared lineage records once and cache only newly published tails in each worker. Fall back to verified token-prefix matching when a recorded parent was filtered from the build, while retaining quarantine for contradictory lineage.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Describe cross-worker consistency, append-only local storage, and the distinct missing-parent and digest-mismatch outcomes with short standalone comments.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Persist root, resolved, and unresolved outcomes at the token publication boundary so multi-worker readers reconstruct only verified lineage.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Use a metadata-only LRU resolver with bounded lock striping, canonical fingerprints, and fail-closed reconstruction so multi-worker continuations resolve without retaining token arrays in memory.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Keep operational comments concise while retaining the failure modes and invariants that explain why the resolver fails closed.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Keep deterministic wire-contract hashes in an allowlisted Python fixture and stamp synthetic rollout records with the explicit root decision required by schema v3.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Exercise the normal custom-sink configuration contract instead of opting collector-only tests into unresolved continuation handling.

Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants