Repository navigation
Document Qwen4-Exp MTP forward contract - #777
titaiwangms with Copilot wants to merge 3 commits into
Conversation
|
|
Co-authored-by: titaiwangms <18010845+titaiwangms@users.noreply.github.com>
Performance Comparison
|
There was a problem hiding this comment.
🟢 Approval recommended
The documentation clearly identifies unresolved semantics as blockers and satisfies the documentation-only scope.
0 open findings
What changed in this PR
Documents a proposed, source-grounded Qwen4-Exp MTP contract while preserving ordinary generation and explicitly blocking unsupported semantics.
Changes:
- Defines candidate equations, graph seams, cache behavior, and weight mappings.
- Specifies independent reference tests and human approval gates.
- Separates pinned observations from unresolved proposals.
| File | Description |
|---|---|
docs/design/qwen4-exp-mtp.md |
Adds the proposed MTP forward contract and qualification plan. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
titaiwangms
left a comment
titaiwangms
left a comment
There was a problem hiding this comment.
Static design review: acceptable as an explicitly blocked proposal, not implementation approval
Reviewed head: f48f9bc5074abba0dbdfde721811d0f3c4cf3660.
Authorities: Mobius 954e8f1053d64d377d8893e5c700d1b8380d584a, GenAI 45f9adfea9b499aef16672493260a8a50706f0bc, and Transformers 598d8ba8baaec7fec5a22da0e2844c7bf4ea20e1.
This is the synthesis of a Copilot-assisted, five-role static review of this PR and companion #776. No Major or Critical defect was confirmed in this document. It is acceptable as a proposal that accurately retains unresolved semantic blockers, subject to the Minor traceability correction below. It is not implementation-ready, and this comment is not a formal approval or permission to start M1.
No tests, builds, model execution, repository imports, or source changes were performed.
Minor: correct the pinned Mobius source ranges
Location: docs/design/qwen4-exp-mtp.md:27,32
Raised by: Deep; Readability also questioned source ranges
Confidence: High
The fixed source revision is appropriate, but some ranges do not establish the facts attributed to them:
- The ordinary main's final mixer return is at
qwen4_exp.py:1375-1379, and the LM-head return at1404-1413, rather than the ranges currently cited for the absence of a public pre-final tap. - MTP streaming exclusion is at
1443-1449, and ordinary weight preprocessing exclusion at1603-1607.
Minimal revision: Correct these ranges so reviewers can follow the actual tap/output and MTP-exclusion evidence. This does not overturn the broad claims themselves.
Major-finding ledger and minority positions
| Issue | Raised by | Disposition |
|---|---|---|
Four-axis MTP positions at 143-160 versus text-main's two-dimensional positions |
Integration: Major; Readability: Minor; Deep did not identify a corresponding Major | Rejected as an established Major ABI defect; retained as an open design decision. The text/VLM rank distinction is real, but this table expressly proposes a new representation and leaves incompatible ABI details blocked. A candidate rank is not itself a demonstrated numerical/spec violation. |
| Cross-root/nested-MTP sharing, coordinated with #776 | Readability: Major; Code and Integration: open question | Rejected as an established merge blocker; retained as an implementation gate. The document explicitly requires approval of sharing/selection integration. It does not establish that the existing manifest already supports cross-package endpoints. |
These are lead synthesis decisions, not unanimous reviewer agreement. In particular, the Integration reviewer's stronger position-rank objection remains recorded above.
Open decisions before M1
- Deep / Integration / Readability: Explicitly choose the text versus VLM position representation and its mapping to main and GenAI. The four-axis candidate should not be read as the existing text-main ABI.
- Deep: Establish the shifted token/tap/target relationship, first draft and prefill behavior, position assignment, cache initialization/advancement, and supported repeated invocation depth from an authoritative behavior or explicitly approved independent reference. A self-consistent eager implementation alone cannot prove the trained/serving alignment.
- Deep: Approve normalization offsets, intermediate rounding, projection shapes/orientation, and accumulation semantics. HF's FP32 offset/scaling and GenAI's model-dtype initializer construction must not be assumed equivalent. This review did not measure an actual custom-kernel discrepancy.
- Readability / Code / Integration: Resolve the shared token-table/head identities and canonical ownership across the root package and nested MTP sidecar, using the same selected-loading contract as #776.
- Code / Integration: Name the portable tiny/offline entry point required by #772 (
src/mobius/models/qwen4_exp_mtp_test.py) and distinguish locally generated synthetic reference tensors from resource-gated official-weight fixtures. Missing mandatory reference capability must remain blocked, not become a passing skip. - Readability: Coordinate the explicit request mechanism for the main pre-final tap with #776, without silently selecting or executing MTP during ordinary generation.
What the document gets right
The candidate equations correctly describe the pinned Qwen4-specific projection wiring and the final mixer's distinct scaling operations, while leaving precision equivalence unproven. The document does not treat ordinary HF CausalLM, a custom-op builder, or the generated graph as an independent MTP oracle. It also correctly separates one trained layer from arbitrary drafting depth, dedicated MTP state from main state, and forward qualification from speculative generation.
Exclusions: No runtime numerical parity, kernel-rounding experiment, cache/fault-injection execution, performance/memory measurement, official-weight qualification, native GenAI execution, or Olive validation. Existing QSA cutoff-tie limitations and BF16 vision restrictions remain. Merging this document must not be interpreted as resolving its semantic blockers or satisfying #772's predecessor/base gates.
Correct pinned source ranges, distinguish text and VLM position candidates, and make the unresolved main-tap and nested-weight-sharing contracts explicit. Specify the future offline synthetic-reference test entry point separately from the resource-gated official campaign. Preserve all semantic approvals. Validation: documentation-only change; git diff --check and targeted static document assertions passed. No model tests or numerical qualification ran. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d24442a-b68c-452f-8538-12f34b5bc4ea Signed-off-by: titaiwang <titaiwang@microsoft.com>
|
Addressed the documentation review in 6369b81.
Validation: Token alignment, rounding/accumulation, cache advancement, repeated proposal depth, sharing endpoints, and the independent reference still require the documented authority/approval gates. This update does not approve M1 implementation or imply that ordinary HF CausalLM is an MTP oracle. |
Propose a source-grounded, independently testable Qwen4-Exp MTP contract while preserving ordinary generation. The document distinguishes pinned behavior from proposals and leaves unsupported semantics as explicit blockers for maintainer approval.
Candidate pairing for review—not an approved semantic rule: