Skip to content

Migrate op_multi_out to __getattr__ dispatch with _outputs - #294

Merged
justinchuby merged 2 commits into
mainfrom
migrate-op-multi-out
May 8, 2026
Merged

Migrate op_multi_out to __getattr__ dispatch with _outputs#294
justinchuby merged 2 commits into
mainfrom
migrate-op-multi-out

Conversation

@justinchuby

Copy link
Copy Markdown
Member

Problem

op.op_multi_out() is being removed from onnxscript/onnx_ir. 9 call sites across 5 rewrite rule files use this API.

Fix

Replace all op.op_multi_out() calls with the standard __getattr__ dispatch pattern using _outputs=N:

# Before
outputs = op.op_multi_out(
    'GroupQueryAttention',
    inputs=[q, k, v, past_k, past_v, seqlens, total_seq, cos, sin],
    domain='com.microsoft',
    attributes=gqa_attrs,
    num_outputs=3,
)

# After
outputs = op.GroupQueryAttention(
    q, k, v, past_k, past_v, seqlens, total_seq, cos, sin,
    _domain='com.microsoft',
    _outputs=3,
    **gqa_attrs,
)

Files Changed (5 files, 9 calls)

  • _group_query_attention.py: 4 calls
  • _skip_layer_norm.py: 2 calls
  • _skip_norm.py: 1 call
  • _unpack_qkv.py: 1 call
  • _separate_rope.py: 1 call

Testing

  • ✅ All 2718 tests pass
  • ✅ Verified GQA (24 ops), SkipNorm (48 ops), RoPE separation all produce correct output on Qwen2.5-0.5B with CUDA EP

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown

Performance Comparison

Comparing 8f7c007dc921b3

Model Metric Baseline Current Delta
bert (feature-extraction) model_size_bytes 359 KB 359 KB +0.0%
bert (feature-extraction) num_nodes 60 60 +0.0%
falcon model_size_bytes 364 KB 364 KB +0.0%
falcon num_nodes 66 66 +0.0%
gemma2 model_size_bytes 428 KB 428 KB +0.0%
gemma2 num_nodes 107 107 +0.0%
gpt2 model_size_bytes 388 KB 388 KB +0.0%
gpt2 num_nodes 53 53 +0.0%
llama model_size_bytes 425 KB 425 KB +0.0%
llama num_nodes 61 61 +0.0%
llama (static-cache) model_size_bytes 425 KB 425 KB +0.0%
llama (static-cache) num_nodes 58 58 +0.0%
mamba (ssm-text-generation) model_size_bytes 296 KB 296 KB +0.0%
mamba (ssm-text-generation) num_nodes 98 98 +0.0%
phi3 model_size_bytes 421 KB 421 KB +0.0%
phi3 num_nodes 59 59 +0.0%
phi3 (static-cache) model_size_bytes 421 KB 421 KB +0.0%
phi3 (static-cache) num_nodes 56 56 +0.0%
qwen2 model_size_bytes 425 KB 425 KB +0.0%
qwen2 num_nodes 61 61 +0.0%
qwen2 (static-cache) model_size_bytes 425 KB 425 KB +0.0%
qwen2 (static-cache) num_nodes 58 58 +0.0%
qwen3_5_moe (hybrid-text-generation) model_size_bytes 506 KB 506 KB +0.0%
qwen3_5_moe (hybrid-text-generation) num_nodes 275 275 +0.0%
qwen3_5_text (hybrid-text-generation) model_size_bytes 458 KB 458 KB +0.0%
qwen3_5_text (hybrid-text-generation) num_nodes 129 129 +0.0%
qwen3_5_vl (hybrid-qwen-vl) model_size_bytes 977 KB 977 KB +0.0%
qwen3_5_vl (hybrid-qwen-vl) num_nodes 408 408 +0.0%
t5 (seq2seq) model_size_bytes 836 KB 836 KB +0.0%
t5 (seq2seq) num_nodes 166 166 +0.0%
whisper (speech-to-text) model_size_bytes 1008 KB 1008 KB +0.0%
whisper (speech-to-text) num_nodes 128 128 +0.0%

No performance regressions.

@codecov

codecov Bot commented May 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown

🏗️ Architecture Diff

Comparing 8f7c007dc921b3

Model Sub-model Changes Status

No architecture changes detected.


Legend: ⚪ No change · 🔵 Minor (attrs/inits) · 🟡 Moderate (nodes added/removed) · 🔴 Major (interface changed)

Copilot AI 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.

Pull request overview

This pull request updates Mobius’ ONNX rewrite rules to stop using the soon-to-be-removed op.op_multi_out() helper by switching to standard OpBuilder.__getattr__ dispatch with _outputs=N. In addition, it introduces and registers a new hy_v3 (Hy3-preview) MoE causal LM and extends HuggingFace config-field alias handling to support that model family.

Changes:

  • Migrated custom-op multi-output calls in rewrite rules from op.op_multi_out(...) to op.<OpName>(..., _domain=..., _outputs=N, **attrs).
  • Added Hy3CausalLMModel implementation and registered hy_v3 in the model registry and test config set.
  • Extended ArchitectureConfig.from_transformers() to recognize additional HF config aliases (e.g., qk_norm, router_scaling_factor, moe_router_use_sigmoid, num_shared_experts).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/_test_configs.py Adds a tiny hy_v3 config entry for coverage in graph-building tests.
src/mobius/rewrite_rules/_unpack_qkv.py Replaces op_multi_out usage with _domain/_outputs dispatch for GroupQueryAttention.
src/mobius/rewrite_rules/_skip_norm.py Replaces op_multi_out usage for SkipSimplifiedLayerNormalization with _outputs.
src/mobius/rewrite_rules/_skip_layer_norm.py Replaces op_multi_out usage for SkipLayerNormalization with _outputs.
src/mobius/rewrite_rules/_separate_rope.py Replaces op_multi_out usage for GroupQueryAttention with _outputs.
src/mobius/rewrite_rules/_group_query_attention.py Replaces multiple GroupQueryAttention op_multi_out calls with _domain/_outputs dispatch.
src/mobius/models/hy_v3.py Adds the Hy3-preview model implementation and weight remapping/splitting logic.
src/mobius/models/init.py Exports/imports Hy3CausalLMModel from the models package.
src/mobius/_registry.py Registers hy_v3 for fallback detection and default model ID mapping.
src/mobius/_configs.py Adds HF config alias support for qk-norm and MoE routing/shared-expert fields.

Comment thread src/mobius/models/hy_v3.py Outdated
Comment thread src/mobius/_registry.py
Comment thread src/mobius/models/hy_v3.py Outdated
@titaiwangms

Copy link
Copy Markdown
Contributor

Thanks for the PR! The op_multi_out migration itself is approve-ready — zero graph changes, 2718 tests passing, and it directly unblocks onnxscript 0.7.1 compatibility. The hy_v3 model is structurally solid but has a few items needed before merge:

Required before merge:

  1. create_attention_bias() missing dtype=self._dtype in Hy3TextModel.forward — without this, fp16/bf16 runs use a float32 mask value which can produce incorrect masking behavior (potential NaN in attention softmax on some hardware). Easy fix, high impact.

  2. Test for moe_router_use_sigmoid=True code path — this logic in _configs.py silently overrides an explicit scoring_func value. It's untested, and exactly the kind of silent-priority rule that most needs a test.

  3. Test for text-only Hy3 (num_local_experts=None) fallback — this code path is not covered. Both this and item 2 are flagged by codecov.

  4. Comment on config fallback priority ordering — the scoring_func/moe_router_use_sigmoid priority rule in _configs.py needs a comment documenting intent (e.g. # moe_router_use_sigmoid takes priority over scoring_func field). Same for the router_scaling_factor and num_shared_experts fallback chains — these are silent model-specific couplings with no explanation.

Recommended (this PR or follow-up):

  1. Move _DeepSeekMoEFFN + DeepSeekMoEGate to mobius/components/_moe.pyhy_v3.py imports _DeepSeekMoEFFN from mobius.models.deepseek, which is a cross-model private symbol import (models → models dependency, not components → models). Since mobius/components/_moe.py already houses MLP and is the established home for MoE primitives, moving these there (and renaming _DeepSeekMoEFFNDeepSeekMoEFFN) makes both deepseek.py and hy_v3.py consumers of shared components rather than one importing from the other.

  2. Hy3TextModel.forward signature divergenceinputs_embeds and dtype params present in DeepSeekV3TextModel.forward are absent here. Please confirm this is an intentional simplification for Hy3 (if so, a short comment would prevent future devs from 'fixing' it); if it's an oversight, it may be a correctness gap.

Cosmetic:

  1. Update PR description — the current description only mentions the op_multi_out migration ("5 files, 9 calls"). The hy_v3 model addition (217 lines) and the _configs.py fixes are not reflected. Worth updating so reviewers understand the full scope.

Positive note: The hy_v3.py module docstring and the preprocess_weights weight-key mapping table are genuinely excellent — clear, complete, and exactly the documentation standard we want for all new model files.

Comment thread src/mobius/models/hy_v3.py Outdated
Replace all 9 op.op_multi_out() calls across 5 rewrite rule files
with the standard __getattr__ dispatch pattern:

  op.op_multi_out('OpName', inputs=[a, b], domain='com.microsoft',
                  attributes=attrs, num_outputs=N)
  →
  op.OpName(a, b, _domain='com.microsoft', _outputs=N, **attrs)

This prepares for the upcoming removal of op_multi_out from
onnxscript/onnx_ir. The __getattr__ dispatch on Builder handles
_outputs internally and will be updated to not depend on
op_multi_out.

Files changed:
- _group_query_attention.py (4 calls)
- _skip_layer_norm.py (2 calls)
- _skip_norm.py (1 call)
- _unpack_qkv.py (1 call)
- _separate_rope.py (1 call)

All 2718 tests pass. Verified GQA/SkipNorm/RoPE rewrite rules
produce correct output on Qwen2.5-0.5B with CUDA EP.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
justinchuby force-pushed the migrate-op-multi-out branch from 4e9bdf4 to a6178f2 Compare May 8, 2026 20:02
@justinchuby
justinchuby requested a review from gramalingam May 8, 2026 20:03
@justinchuby

Copy link
Copy Markdown
Member Author

@titaiwangms hy_v3 removed and isolated to another PR. Thanks

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby
justinchuby merged commit ba2b83e into main May 8, 2026
23 checks passed
@justinchuby
justinchuby deleted the migrate-op-multi-out branch May 8, 2026 20:35
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.

3 participants