Skip to content

fix(experimental): validate concat parent token prefixes - #1553

Open
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:fix/concat-parent-token-prefix
Open

fix(experimental): validate concat parent token prefixes#1553
EazyReal wants to merge 1 commit into
areal-project:mainfrom
EazyReal:fix/concat-parent-token-prefix

Conversation

@EazyReal

@EazyReal EazyReal commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

The experimental concat path inherits parent loss masks, sampling logprobs, and model versions by position. This change requires the child prompt to preserve the parent's exact token prefix before generation and applies the same invariant before inherited parent fields are concatenated into the child tensors.

The early check prevents an inference request from being issued with an invalid parent binding. Because cache insertion is provisionally required to resolve the parent relationship, a rejected concat prompt also rolls that child entry back before the exception escapes. The projection check remains a backstop for callers that construct interactions directly. Exact prefixes and the existing equal-or-shorter fallback remain unchanged.

This intentionally does not choose a PAD/EOS policy. Chat rendering may normalize a boundary, but a normalized projection must not silently flow back into positional sampled evidence.

Related Issue

Fixes #1551

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 💥 Breaking change
  • 📝 Documentation update
  • ♻️ Refactoring
  • ⚡ Performance improvement
  • ✅ Test coverage improvement

Checklist

  • I have read the Contributing Guide
  • Pre-commit hooks pass (pre-commit run --all-files)
  • Relevant tests pass; new tests added for new functionality
  • Documentation updated (not applicable; no public interface changed)
  • Branch is up to date with main
  • Self-reviewed via /review-pr command
  • This PR was created by a coding agent via /create-pr
  • This PR is a breaking change

Breaking Change Details (if applicable):

Not applicable.

Additional Context

Validation on main at the branch's merge-base (4d7ee114):

  • uv run pytest tests/experimental/openai/test_client_cache_lifecycle.py tests/experimental/openai/test_types.py tests/experimental/openai/test_retry_orphan.py tests/test_http_generation_result.py tests/test_generation_response_parsing.py -q — 35 passed
  • existing tests/experimental/openai/test_concat_prompt.py module with an in-process, local-cache-only model-path shim — 8 passed
  • uv run pre-commit run --all-files — passed
  • git diff --check upstream/main...HEAD — passed
  • independent final-diff review — passed with no findings

The concat module's normal import eagerly resolves unrelated model fixtures, including uncached 30B vision weights, so the local-cache-only shim was used to exercise its eight Qwen concat tests without downloading those weights. No GPU or live inference service is required for this invariant.

Re-verified against current main (2ee49047, 17 commits ahead of the merge-base): pytest tests/experimental/openai/test_client_cache_lifecycle.py tests/experimental/openai/test_types.py -q — 7 passed. Of the commits landed since the merge-base, two touch areal/experimental/openai/: #1516 adds an original_reward/original_rewards key to to_tensor_dict's output (types.py), and #1499 realigns proxy tool schemas in client.py, well clear of concat_prompt_token_ids_with_parent. Neither conflicts with this change; #1516's new key is additive and this PR's assertions only check their own expected dict.

Current main includes #1554 and #1498. They are complementary: #1554 validates provider evidence completeness, this PR preserves token identity across recursive projection, and #1498 prunes retry-generated duplicate leaves.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces validation to ensure that a child prompt preserves the exact parent token sequence before reusing parent token data. Specifically, it adds the ensure_parent_token_prefix helper function to verify token prefixes, integrates this check into both concat_prompt_token_ids_with_parent and to_tensor_dict, and adds corresponding unit tests to verify the behavior. I have no feedback to provide as there are no review comments.

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.

Reject a mismatched parent prefix before issuing generation and roll back
the provisional cache entry when prompt construction fails. Reuse the same
invariant before inherited masks, log probabilities, and versions are
concatenated into the child.

Signed-off-by: EazyReal <8047065+EazyReal@users.noreply.github.com>
@EazyReal
EazyReal force-pushed the fix/concat-parent-token-prefix branch from c129dab to cacbae2 Compare July 23, 2026 19:15
@EazyReal

Copy link
Copy Markdown
Contributor Author

Maintainer question for the follow-up stop-token behavior (not part of this PR):

The concat request currently submits both EOS and PAD as stop token IDs. If a distinct PAD is the first sampled terminal action, the existing path strips it and inserts EOS, which would bind the PAD logprob, mask, and version to a different token ID.

I see two coherent contracts:

  1. PAD is a sampled terminal action (my recommendation under the current positional-evidence representation): preserve the exact sampled tokens through the first terminal token, then append any template-required EOS as unsampled prompt structure.
  2. PAD is padding only: stop offering distinct PAD as a generation stop token, and remove any synthetic padding together with all of its token-owned evidence at the provider boundary.

Replacing only PAD with EOS while retaining PAD evidence is not coherent under either contract. Which meaning is intended for the experimental concat path? I am holding the dependent stop-token change until this is confirmed, and I would reject rather than preserve any provider suffix after the first terminal token.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity within the last 14 days.

Please add a comment or push new commits to keep it active.

Thank you for your contribution!

@github-actions github-actions Bot added the stale label Aug 7, 2026
@EazyReal

Copy link
Copy Markdown
Contributor Author

@sitabulaixizawaluduo re-raising the open question from July 23: the concat path submits both EOS and PAD as stop tokens, and today a PAD terminal action gets silently replaced by EOS, binding that token's logprob/mask/version to the wrong id. Two coherent contracts: treat PAD as a real sampled terminal token and preserve it, appending EOS only as unsampled prompt structure — or stop offering PAD as a stop token and strip its evidence at the provider boundary. Which one should this PR target? I'm holding the dependent stop-token change on the answer.

@github-actions github-actions Bot removed the stale label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Concat can reuse parent evidence across a token-prefix mismatch

1 participant