Skip to content

knowledge(vllm-omni): 1 change(s) from refine - #353

Merged
tzhouam merged 1 commit into
mainfrom
kb/vllm-omni/vllm-omni-refine-88822faaeb38
Oct 12, 2026
Merged

tzhouam merged 1 commit into
mainfrom
kb/vllm-omni/vllm-omni-refine-88822faaeb38

Conversation

@tzhouam

@tzhouam tzhouam commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Generated automatically by the Copilot knowledge service (changeset vllm-omni-refine-88822faaeb38).

op page rule
add repos/vllm-omni/models/omnivoice/rules.md VLLM-OMNI-PR6105-INSTRUCT-VALIDATION

Generator: zcode:GLM-5.3 · judge: codex:gpt-6-sol:medium.
The publisher merges it only if its local gate passes on the exact merge result.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T09:04:48.320539Z c29b937 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@tzhouam

tzhouam commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

Knowledge quality gate · checked 2026-10-12 at c29b9379edc8 (checked again once a day)

Result: not passed. This PR will not be merged until the problems below are fixed; push a fix and it is checked again at the next daily pass.

Problems:

  • L2 was not sure about VLLM-OMNI-PR6105-INSTRUCT-VALIDATION

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c29b9379ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


- 触发:修改 OmniVoice speech 的 `instructions` 输入、`tts_adapters/omnivoice.py` 的校验接线、`model_executor/models/omnivoice/prompt_utils.py` 的 `validate_instruction`/`prepare_instruct`,或 `instruct_constants.py` 的有效词表。
- 强制:instructions 是逗号分隔的封闭词表,有效项以模型目录的 `instruct_constants.py` 为唯一来源。分隔符不是输入校验的硬性要求:解析同时接受半角与全角逗号,“英文 comma + space、中文全角逗号”只是报错提示中的建议写法。校验在 request-facing 的 TTS adapter 完成,非法请求在进入 engine 前返回结构化 400。三类非法各自可识别:不支持/不存在的项返回 "Unsupported instruct items found",且只有该错误附带两种语言的有效项列表与分隔符提示;同类别冲突项(如 teenager 与 middle-aged)返回 "Conflicting instruct items within the same category";中文方言与英文口音混用返回 "Cannot mix Chinese dialect and English accent";后两类报错不包含有效项列表或分隔符提示。校验通过后只把 `prepare_instruct` 的产物传给下游;offline 示例在写入 `mm_processor_kwargs["instruct"]` 前执行同一 validate→prepare 流程。
- 禁止:把未校验的原始 instructions 直接送入 engine 或 `mm_processor_kwargs`;在 serving/adapter 内复制第二份有效词表;静默丢弃非法项或让 free-form 文本进入模型;以分隔符写法不符为由拒绝本可解析的请求;diffusion speech server 因 voice 未知而缺少 adapter(未知 voice 时 `_adapter` 应为 OmniVoiceAdapter 而非 None)。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire the stale pure-diffusion adapter requirement

When reviewing OmniVoice through the pure-diffusion for_diffusion() path, this clause requires _adapter to be an OmniVoiceAdapter, but the current serving-owner contract in knowledge/repos/vllm-omni/components/serving/rules-engine-lifecycle.md:97-102 requires that path to short-circuit before adapter resolution, return None, and never read _adapter; lines 73-74 also state that DiffusionTTSAdapter has no production subclass. Keeping the older PR #6105 behavior active would direct reviewers to undo PR #6121's factory-path fix and could reintroduce the resulting AttributeError or incorrect AR routing. Remove or retire this requirement and its line-46 acceptance assertion, or reconcile the behavior in the serving owner rather than the model page.

AGENTS.md reference: AGENTS.md:L12-L17

Useful? React with 👍 / 👎.

@tzhouam
tzhouam merged commit a549d0d into main Oct 12, 2026
3 checks passed
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.

1 participant