Skip to content

fix: restore cross-sequence ban for n returns#1177

Open
Xavier2024P wants to merge 2 commits into
alibaba:mainfrom
Xavier2024P:fix/cross-seq-ban-n-offset
Open

fix: restore cross-sequence ban for n returns#1177
Xavier2024P wants to merge 2 commits into
alibaba:mainfrom
Xavier2024P:fix/cross-seq-ban-n-offset

Conversation

@Xavier2024P

Copy link
Copy Markdown
Contributor

Summary

  • Route OpenAI n through GenerateConfig.update() so enable_cross_sequence_ban is revalidated and can be re-enabled after num_return_sequences is supplied.
  • Stop treating num_return_sequences > 1 as a full-position token offset signal in RecommendationLogitsProcessor; only beam search uses offset reads.
  • Add an offset bounds check and update recommendation logits processor tests for multi-return incremental tokens and beam offset behavior.

Why

When n=4 / num_return_sequences=4 is used together with enable_cross_sequence_ban=true, the OpenAI path can leave enable_cross_sequence_ban disabled because request.n was assigned directly instead of going through GenerateConfig.update().

Separately, RecommendationLogitsProcessor treated num_return_sequences > 1 as requiring full-position token offset reads. In non-beam multi-return generation, new_tokens is incremental ([N, num_new_tokens]), so the offset path can read out of bounds and cause a core dump.

Changes

  • Use config.update({"num_return_sequences": request.n}) in the OpenAI endpoint.
  • Change needs_token_offset to depend only on config->hasNumBeams().
  • Add bounds checking before offset-based token reads.
  • Update tests so:
    • non-beam num_return_sequences > 1 uses incremental token tensors and keeps cross-sequence ban enabled;
    • beam search still covers the full-position offset path.

@Xavier2024P Xavier2024P requested a review from LLLLKKKK as a code owner July 10, 2026 09:04
@LLLLKKKK

Copy link
Copy Markdown
Collaborator

AI Code Review - PR #1177

Status: BLOCKING

Summary: P0/0 · P1/3 · P2/0 · P3/0

Blocking Issues

P1

  • Variable beam 在扩展前会按错误布局读取 token @ rtp_llm/cpp/models/logits_processor/RecommendationLogitsProcessor.cc:46
    • 建议:让 token 布局成为本轮 update 的显式契约,例如对所有 hasNumBeams() 请求始终传完整 token 矩阵,或通过 StreamUpdateInfo 传递布局标志;不要用最大 beam 数推断当前 tensor。补充 variable_num_beams=[1,4] 在扩 beam 前后的回归测试。
  • 增量多返回布局只同步了一个 logits processor @ rtp_llm/cpp/models/logits_processor/RecommendationLogitsProcessor.cc:46
    • 建议:抽取并统一 logits processor 的 token 布局判断,同步修正 ThinkMode、Tree 和 Recommendation 三个消费者。增加经 LogitsProcessorFactoryGenerateStream::update 驱动的组合测试,传入生产形状 [N,1] 验证所有 processor。
  • OpenAI 入口的重新启用语义没有回归测试 @ rtp_llm/openai/openai_endpoint.py:184
    • 建议:在 OpenAI 配置测试中构造 n=3combo_token_size>=2、初始自动降级的 extra_configs,调用 _extract_generation_config 后断言 num_return_sequences==3 且跨序列 ban 已重新启用;同时覆盖 n<=1 和 beam 不兼容分支。

Checklist Violations (4 fail / 91 total)

General Principles Checklist

  • [6.1] Architecture — 状态不变量:创建/更新/失败/重试/回滚路径有效 → issue 增量多返回布局只同步了一个 logits processor
    GenerateStream 会把同一个 [N,1] 增量 new_tokens 交给所有 processor。本行修正了 recommendation,但 ThinkModeLogitsProcessorTreeLogitsProcessor 仍以 hasNumBeams() || num_return_sequences > 1 选择 full-position offset。启用 think/tree 且 n>1 时,它们仍从 input_length 后读取越界,组合场景没有覆盖。
  • [6.1] Tests — 新逻辑有聚焦单测 + 相关集成/smoke 测试 → issue OpenAI 入口的重新启用语义没有回归测试
    此处的关键语义不是赋值,而是依赖 GenerateConfig.update() 将先前自动降级的 enable_cross_sequence_ban 重新启用。现有 Python 测试直接调用 update(),变更的 C++ 用例也绕过 OpenAI 入口;没有使用 ChatCompletionRequest(n>1, extra_configs=...) 验证 _extract_generation_config,直接赋值回归时现有测试仍会通过。
  • [6.1] Tests — 边界 case 覆盖(空、单元素、最大值) → issue Variable beam 在扩展前会按错误布局读取 token
    hasNumBeams()variable_num_beams 的最大值;配置 [1, 4] 会立即令 needs_token_offset=true,但首步 nextNumBeams() 仍为 1,NormalOutputDispatcher 因而传入 [1,1] 增量 tensor。updateStatus 随后计算 offset=input_length,新增边界检查必然失败。当前测试只覆盖固定 num_beams=2

RTP-LLM Checklist

  • [I] 代码质量 — 同一功能用统一工具函数 → issue 增量多返回布局只同步了一个 logits processor
    GenerateStream 会把同一个 [N,1] 增量 new_tokens 交给所有 processor。本行修正了 recommendation,但 ThinkModeLogitsProcessorTreeLogitsProcessor 仍以 hasNumBeams() || num_return_sequences > 1 选择 full-position offset。启用 think/tree 且 n>1 时,它们仍从 input_length 后读取越界,组合场景没有覆盖。

Strengths

  • 普通 num_return_sequences 与固定 beam 的 token 布局被明确区分,并为 full-position 读取增加了边界检查。
  • C++ 用例同时验证了增量多返回、固定 beam offset 和跨序列广播结果。

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.

2 participants