Skip to content

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

Merged
tzhouam merged 1 commit into
mainfrom
kb/vllm-omni/vllm-omni-refine-2321a749c5ca
Oct 26, 2026
Merged

tzhouam merged 1 commit into
mainfrom
kb/vllm-omni/vllm-omni-refine-2321a749c5ca

Conversation

@tzhouam

@tzhouam tzhouam commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Generated automatically by the Copilot knowledge service (changeset vllm-omni-refine-2321a749c5ca).

op page rule
add repos/vllm-omni/components/configuration/rules-environment-variables.md VLLM-OMNI-PR9374-CODE2WAV-TEST-SYNC

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 26, 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-26T05:39:29.642054Z bc07ce4 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.

@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: bc07ce4c18

ℹ️ 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".

Comment on lines +14 to +17
- 触发:PR 向 `ENVIRONMENT_VARIABLE_INVENTORY` 新增 `EnvironmentVariableCategory.PUBLIC_OMNI` 类环境变量(如 #9370 新增的 `VLLM_OMNI_CODE2WAV_WATCHDOG_TIMEOUT_S`);或改动 `MiniCPMO45Code2Wav` 的 watchdog 契约(`__init__` 建立的 `_watchdog` 属性或 `forward()` 对它的读取);或改动 `tests/config/test_environment_variables.py::test_inventory_matches_reviewed_snapshot_counts`、`tests/model_executor/models/minicpmo_4_5/test_cuda_graph_wrapper.py::test_vocoder_restores_tf32_policy`。
- 强制:`test_inventory_matches_reviewed_snapshot_counts` 以 `Counter` 按类别统计清单条目并与 expected 字典精确比对,docstring 即 “Make an inventory expansion an explicit review decision”;因此新增 PUBLIC_OMNI 条目的 PR 必须在同一 PR 内同步上调 expected 中该类计数——#9370 新增 `VLLM_OMNI_CODE2WAV_WATCHDOG_TIMEOUT_S` 使实际计数变为 34,AMD build #14633 以 34 vs 33 失败,PR #9374 将 expected 从 33 补到 34,同步不得留给 CI 红灯后的补丁 PR。`test_vocoder_restores_tf32_policy`(fail=True/False 两个参数化)以 `SimpleNamespace` 桩替身调用 `MiniCPMO45Code2Wav.forward()`,#9370 之后 `forward()` 读取 `_watchdog`,故该桩须在已解析的 `_tf32_mode` 之外携带 `_watchdog=Code2WavWatchdog()`(导入自 `vllm_omni.model_executor.models.minicpmo_4_5.code2wav_watchdog`;PR #9374 记录该默认构造与 `__init__` 建立的状态一致、不启动监控线程)。评审触发范围内的 PR 时须核对这两处测试假设已随契约变更一并更新。
- 禁止:在未同步 PUBLIC_OMNI expected 计数的情况下合并新增该类清单条目的 PR,或把计数同步推迟到 CI 失败后的后续 PR;在 `test_vocoder_restores_tf32_policy` 的桩中遗漏 `_watchdog`(CUDA READY #18082 与 AMD #14634 两个参数化失败的原因)。
- 验收:新增 PUBLIC_OMNI 条目或改动 watchdog 契约的 PR tip 上,`test_inventory_matches_reviewed_snapshot_counts` 的 PUBLIC_OMNI expected 计数与清单实际条目数一致(PR #9374 时点为 34);`pytest -q tests/config/test_environment_variables.py::test_inventory_matches_reviewed_snapshot_counts` 与 `pytest -q tests/model_executor/models/minicpmo_4_5/test_cuda_graph_wrapper.py::test_vocoder_restores_tf32_policy` 均通过;TF32 策略测试的桩同时含 `_tf32_mode` 与 `_watchdog=Code2WavWatchdog()`。^[PR #9374]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Split the MiniCPM watchdog rule into its model owner

For changes to MiniCPMO45Code2Wav.forward() or its TF32 test stub, the new watchdog requirement is stored only under configuration, despite being specific to MiniCPM-o 4.5. The existing model owner at models/minicpm-o-4-5/ already has rules-code2wav-runtime.md, and Direct routes model-named reviews through that model’s rules.md; neither model entry links to this new requirement. Consequently, model-scoped reviews can miss the watchdog check, while configuration reviews receive an unrelated model contract. Keep the inventory-count rule here, move the watchdog/test-stub rule to the MiniCPM owner, and update its routing entry. The root instructions explicitly require model-specific rules to remain with their model owner.

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

Useful? React with 👍 / 👎.

@tzhouam

tzhouam commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Knowledge quality gate · checked 2026-10-26 at bc07ce4c18e9 (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-PR9374-CODE2WAV-TEST-SYNC

@tzhouam
tzhouam merged commit c702892 into main Oct 26, 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