[AgentGateway]feat: Prefix Trie Storage for Multi Trajectories #65
[AgentGateway]feat: Prefix Trie Storage for Multi Trajectories #65gxlvera wants to merge 10 commits into
Conversation
Add a per-session prefix trie to replace the single linear active trajectory (issue verl-project#51): every committed assistant turn becomes a node that may carry a checkpoint, and an incoming request longest-prefix matches any path and continues from the nearest checkpoint. - session/trie.py: MessageKey (+canonicalize_content/make_message_key), BranchCheckpoint, TrieNode, PrefixTrie (prepare/commit/finalize-traversal). MessageKey excludes reasoning_content (M1) and hashes multimodal content via a stable image/video digest (data:/long strings are hashed for compactness). - session/types.py: home TrajectoryBuffer here so trie stays free of the verl-importing codec; re-exported via session/__init__. - CPU tests cover the RFC appendix fork types (sequential / system-split / condensation / best-of-N + idempotent retry / warm-start) plus pending-node, clone-isolation, export, multimodal-digest, and regression guards that fail on the pre-review behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wire the trie into GatewaySession end-to-end, gated by GatewayActorConfig.gateway_trie_enabled (default False; flag-off reproduces the legacy single-active behavior exactly, covered by a parity test). - session.py: trie-mode prepare routes through trie.prepare (longest-prefix match + nearest-checkpoint clone) and reuses the existing encode_full/encode_incremental paths; commit attaches the assistant child; finalize traverses the trie -> one branch-aware trajectory per terminal checkpoint. Per-branch multimodal stored on the node; failed generations abandon the pending node; snapshot_state reports num_branches/num_inflight. tools-change gate mirrors the legacy active_tool_schemas gate. - config.py/gateway.py: thread gateway_trie_enabled into sessions. - session-integration tests: flag parity, best-of-N fan-out, multi-turn reattach. Tokenization stays on remove_system_prompt (TITO -> M2); generation_lock kept (concurrency -> M3); no GC (trie dies with the session). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- _prepare_generation_inputs_trie: abandon the pending node if encoding fails or is cancelled after trie.prepare (try/except BaseException around the encode step), preventing in-flight bookkeeping leaks. Added a session test that fails without the guard. - _media_digest: hash repr(_freeze(value)) in the scalar fallback so nested dict/list ordering is canonical across processes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- session.py: on a full re-encode mid-branch (e.g. tools change), store only this turn's delta media on the committed node instead of the whole prompt's media, so collect_multi_modal does not double-count media carried on ancestor checkpoints. Added a regression test (multimodal + tools-change mid-branch) that fails without the fix. - trie.py: defensively shallow-copy message dicts when storing them in nodes (materialize_prompt_suffix, upsert_assistant) and in the checkpoint's covered_messages, isolating trie state from external payload mutation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- session.py: catch BaseException around backend.generate so a cancellation (asyncio.CancelledError, which is not an Exception) still abandons the trie pending node before propagating. Added a regression test that fails without the guard. - trie.py: move 'import json' to module top (PEP 8) instead of inline. Co-Authored-By: Changyi Yang <changyiyang2023@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing multi trajectory test
There was a problem hiding this comment.
Code Review
This pull request introduces a prefix trie (PrefixTrie) to support multi-trajectory session storage in the gateway, enabling features like multi-branch reattachment, best-of-N, sub-agents, and warm-starts. The changes integrate the trie into GatewaySession under a new configuration flag and add comprehensive CPU-only unit and integration tests. The review feedback focuses on a key memory efficiency improvement: removing the redundant messages list from BranchCheckpoint to avoid O(N^2) space complexity, as the full conversation history can already be reconstructed on demand using rebuild_messages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| trajectory_buffer: TrajectoryBuffer | ||
| request_tools: list[dict[str, Any]] | None = None | ||
| chat_template_kwargs_key: tuple | None = None | ||
| messages: list[dict[str, Any]] | None = None | ||
| image_data: list[Any] | None = None | ||
| video_data: list[Any] | None = None | ||
| extra_fields: dict[str, Any] | None = None |
There was a problem hiding this comment.
Memory Efficiency Improvement: Redundant messages in BranchCheckpoint
Storing messages in BranchCheckpoint introduces O(N^2) space complexity for message storage over a session of N turns, because each checkpoint stores a copy of the entire conversation history up to that point.
Since PrefixTrie already implements rebuild_messages(node) (which reconstructs the exact same message list by walking up the parent pointers in O(N) time), storing messages on the checkpoint is redundant.
By removing messages from BranchCheckpoint, we can reduce the space complexity to O(N) and avoid significant memory bloat in long conversations (especially those with large system prompts or tool outputs).
| trajectory_buffer: TrajectoryBuffer | |
| request_tools: list[dict[str, Any]] | None = None | |
| chat_template_kwargs_key: tuple | None = None | |
| messages: list[dict[str, Any]] | None = None | |
| image_data: list[Any] | None = None | |
| video_data: list[Any] | None = None | |
| extra_fields: dict[str, Any] | None = None | |
| trajectory_buffer: TrajectoryBuffer | |
| request_tools: list[dict[str, Any]] | None = None | |
| chat_template_kwargs_key: tuple | None = None | |
| image_data: list[Any] | None = None | |
| video_data: list[Any] | None = None | |
| extra_fields: dict[str, Any] | None = None |
| video_data=None, | ||
| ) | ||
|
|
||
| checkpoint_messages = ckpt.messages or self.rebuild_messages(ckpt_node) |
There was a problem hiding this comment.
| # The checkpoint lives on the assistant node and its buffer covers the | ||
| # request prompt PLUS the generated assistant turn, so the stored prefix | ||
| # must include ``assistant_msg`` (this is what the next turn's | ||
| # ``checkpoint_messages`` slices against). | ||
| covered_messages = [dict(m) for m in messages] + [dict(assistant_msg)] if messages is not None else None | ||
| checkpoint = BranchCheckpoint( | ||
| trajectory_buffer=trajectory_buffer, | ||
| request_tools=request_tools, | ||
| chat_template_kwargs_key=chat_template_kwargs_key, | ||
| messages=covered_messages, | ||
| image_data=image_data, | ||
| video_data=video_data, | ||
| extra_fields=extra_fields, | ||
| ) |
There was a problem hiding this comment.
Redundant messages in BranchCheckpoint (Commit Step)
As noted in the BranchCheckpoint definition, storing the full conversation history at each checkpoint is redundant because rebuild_messages can reconstruct it on demand. Removing covered_messages here avoids O(N^2) memory overhead.
checkpoint = BranchCheckpoint(
trajectory_buffer=trajectory_buffer,
request_tools=request_tools,
chat_template_kwargs_key=chat_template_kwargs_key,
image_data=image_data,
video_data=video_data,
extra_fields=extra_fields,
)| trajectories: list[Trajectory] = [] | ||
| for node in self._trie.iter_export_nodes(): | ||
| checkpoint = node.checkpoint | ||
| messages = checkpoint.messages or self._trie.rebuild_messages(node) |
There was a problem hiding this comment.
|
With prefix trie, I think maybe we can get rid of generation lock but only use request lock? the reason is stated in details in: #51 as |
zhanghuiyao
left a comment
There was a problem hiding this comment.
Two trajectory parity comments from local review.
| # Reuse the cloned checkpoint only when its tools match this request; | ||
| # a tools change forces a full re-encode (mirrors the legacy | ||
| # ``active_tool_schemas != tools`` gate). | ||
| use_incremental = prepared.trajectory_buffer is not None and prepared.request_tools == tools |
There was a problem hiding this comment.
P1: when prepared.request_tools != tools, this forces a full re-encode but does not materialize/export the previous checkpoint. The new checkpoint is still committed as a descendant, while finalize exports only terminal checkpoints, so the parent trajectory's trainable response tokens are dropped. This should either create an explicit export boundary, or be documented/tested as an intentional flag-on/off parity exception.
There was a problem hiding this comment.
Good catch — confirmed this is a real correctness gap, not just a parity nit. Here's the fix we landed for it.
Root cause. finalize exported only terminal checkpoints (a committed node with no committed descendant), which silently assumes a deeper checkpoint contains all of its ancestors' trainable tokens. That holds for an incremental continuation (the child buffer is clone(parent) + delta, so the parent's mask=1 tokens are carried forward), but not for a full re-encode: on a tools change the child re-tokenizes the whole history, so the parent's generated tokens become non-trainable prompt in the child and remain mask=1 only on the parent. Since the parent isn't terminal, it was dropped — exactly the trainable tokens the legacy flow preserves via materialized_trajectory.
Fix (absorb-by-incremental-edge). Instead of inferring "absorbed" from tree depth, track it from how the buffer was actually built:
TrieNode.absorbed: boolcommit(..., incremental: bool): whenincremental(buffer cloned from the nearest ancestor checkpoint and extended), mark that ancestorabsorbed = True. A full re-encode marks nothing.iter_export_nodesyields every checkpoint withabsorbed == False(the old_collect_terminalspost-order pass is removed).
use_incremental — the same prepared.request_tools == tools gate flagged here — is threaded from the session into commit.
Why it handles the case. On a mid-branch tools change the re-encoded child does not mark its prefix-ancestor absorbed, so both export as independent trajectories: the pre-change turn keeps its mask=1 tokens (== legacy's materialize-on-context-break), and the re-encoded turn carries the prior turns as mask=0 prompt. It's also robust to the mixed case (a node with both an incremental child and a full-re-encode child): the incremental child absorbs the ancestor, the re-encode child doesn't, so the ancestor's trainable tokens land in exactly one exported trajectory — no drop, no double-count.
Added a regression test (test_trie_exports_pre_reencode_branch_on_tools_change) that is red on the old deepest-terminal rule (1 trajectory, turn-1 generation lost) and green after (2 trajectories, turn-1 mask=1 preserved). Full gateway suite stays green.
Scoped this to the export-correctness issue (this comment); the retry-vs-best-of-N distinction in your other comment is a separate axis and we're treating it independently.
There was a problem hiding this comment.
I think Changyi's explanation makes sense
| differing output creates a new sibling (best-of-N). | ||
| """ | ||
| key = make_message_key(assistant_msg) | ||
| child = parent.children.get(key) |
There was a problem hiding this comment.
P1/P2: using only MessageKey here conflates idempotent retry with independent best-of-N samples. If two sampled completions are token-identical, the second commit reuses this child and overwrites the checkpoint, so trie mode exports 1 trajectory where legacy keeps 2 samples. Training semantics should distinguish retry from duplicate samples, or document/test this as an intentional contract.
There was a problem hiding this comment.
I have a question, for best-of-N samples, is it really necessary to keep the two identical trajectory?
There was a problem hiding this comment.
Also, what do you mean by "Training semantics should distinguish retry from duplicate samples"? Thank you~
There was a problem hiding this comment.
By "distinguish retry from duplicate samples", I mean:
- retry: the same logical generation attempt should be idempotent and may reuse/overwrite the same node;
- duplicate sample: a separate best-of-N attempt should preserve multiplicity, even if the generated tokens are identical, unless we explicitly define trie mode as deduplicating outputs.
There was a problem hiding this comment.
But according to our discussion, a better approach might be to only keep the selected one 😊
finalize() exported only the deepest terminal checkpoint, which assumes a deeper checkpoint contains all of its ancestors' trainable tokens. That holds for an incremental continuation (buffer = clone(parent) + delta) but not for a full re-encode: on a mid-session tools change the child re-tokenizes the whole history, so the parent's generated tokens become non-trainable prompt in the child and are mask=1 only on the parent. Since the parent was not terminal it was dropped -- exactly the trainable tokens the legacy flow keeps via materialized_trajectory. Fix (absorb-by-incremental-edge): track "absorbed" from how the buffer was built rather than from tree depth. - TrieNode.absorbed - commit(incremental=...): mark the nearest ancestor checkpoint absorbed only when this branch cloned it and extended incrementally; a full re-encode marks nothing. - iter_export_nodes yields every checkpoint with absorbed=False (removes the _collect_terminals post-order pass). - EncodedData.use_incremental threads the existing request_tools==tools gate from the session into commit. Robust to the mixed case (a node with both an incremental and a full-re-encode child): the incremental child absorbs the ancestor, the re-encode child does not, so the ancestor's trainable tokens land in exactly one exported trajectory -- no drop, no double-count. Adds a regression test that is red on the old deepest-terminal rule (1 trajectory, turn-1 generation lost) and green after (2 trajectories, turn-1 mask=1 preserved). Full gateway suite stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Implements the per-session prefix trie that replaces the single linear
active_trajectory, per RFC #51. Today a session keeps onemessage_history+ one active trajectory, so only the latest branch can be re-attached; this generalizes it into a trie where an incoming request longest-prefix matches any path and continues from the nearest checkpoint — enabling sub-agent / best-of-N / context-condensation / warm-start.Fully gated behind
GatewayActorConfig.gateway_trie_enabled(default off); flag-off reproduces the legacy single-active behavior exactly (covered by a parity test).What's in this PR (full M1)
Data model —
gateway/trie.py(new)MessageKey(+canonicalize_content/make_message_key): hashable per-message identity; excludesreasoning_content(M1; TODO thinking-model replay) and keys multimodal content via a stable image/video digest (pixels live on the node, not the key).BranchCheckpoint,TrieNode,PrefixTriewith theprepare → commitlifecycle and afinalizetraversal (iter_export_nodes→ terminal checkpoints only by default).preparematerializes incoming prompt-side messages as pending/structural nodes (no checkpoint, never exported) and clones the nearest checkpoint into a request-local buffer.TrajectoryBufferre-homed togateway/types.pyso the trie stays free of the verl-importing codec.Session integration —
gateway/session.py_prepare_generation_inputs_trie: routes throughtrie.prepare(longest-prefix match + nearest-checkpoint clone) and reuses the existingencode_full/encode_incrementalpaths — only the state model changes.trie.commit;finalizetraverses the trie → one branch-awareTrajectoryper terminal checkpoint (reward stamped per the current one-session-one-reward contract).abandonthe pending node.snapshot_statereportsnum_branches/num_inflight_generationsin trie mode.active_tool_schemasgate.Wiring:
config.py/gateway.pythreadgateway_trie_enabledinto sessions.Tests (CPU-only)
tests/uni_agent/gateway/test_trie_on_cpu.py— 16 trie-structural tests covering the RFC appendix fork types: A sequential extension, B system-keyed split (parallel sub-agents), C context condensation, D best-of-N + idempotent retry, E warm-start; plus pending-node/no-export, clone isolation, terminal-vs-all export, multimodal digest + per-branch reconstruction.tests/uni_agent/gateway/test_session_trie_on_cpu.py— 3 session-integration tests: flag parity (trie-on yields identical backend prompts + identical finalized trajectory as trie-off on a linear multi-turn conversation — the compatibility gate), best-of-N fan-out (N siblings → N trajectories), multi-turn reattach (single continuous branch).Scope / deferrals
remove_system_prompt(text via tokenizer, multimodal via processor). Per-model seam handling (Qwen trailing\n, GLM 4.7 ambiguous bos/eos) + a token-sequence verifier → M2 (TITO-style merge).generation_lockkept (serial) → M3.Note that this PR is a more complete version of the closed #64 and this description is produced by @ChangyiYang.