Registry-based module dispatch for unified_export_hf.py#1939
Conversation
Signed-off-by: Shiyang Chen <shiychen@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesExport Registry Dispatch
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Flow as _process_quantized_modules
participant Ctx as ExportContext
participant Registry as ExportModuleRegistry
participant Exporter as ModuleExporter
Flow->>Ctx: create context (model, dtype, caches)
loop for each sub_module
Flow->>Registry: match(sub_module)
Registry-->>Flow: matched exporter or None
Flow->>Exporter: export(name, module, ctx)
end
Flow->>Registry: match(experts container)
Registry-->>Flow: matched exporter or None
Flow->>Exporter: prepare_moe_inputs(name, moe_module, ctx)
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Refactor of unified_export_hf.py's per-module export dispatch: replaces the if/elif chains in _process_quantized_modules and the MoE input-amax prepass in _export_transformers_checkpoint with a new ordered, first-match-wins ExportModuleRegistry (new registry.py: ExportContext, ModuleExporter, ExportModuleRegistry). +595/-158, 4 files, with a solid CPU unit-test suite (test_export_registry.py) covering MRO/name/predicate matching, precedence, prepend, re-registration idempotency, and a small FP8 ToyModel end-to-end export.
Design review (new subsystem/registry): the codebase already has a class-keyed registration-and-dispatch idiom in _DMRegistryCls/QuantModuleRegistry (modelopt/torch/opt/dynamic.py). The new module's docstring explicitly acknowledges this and justifies a different mechanism — quantization converts module classes in place, whereas export needs to emit weights without changing class, plus it needs predicate/structural matching (_has_fused_experts_quantizers, iterable catch-all) that the class-dict-based _DMRegistryCls doesn't offer. That rationale is reasonable, but it lives only in the source docstring — the PR body just says "refactor" and doesn't compare alternatives. Worth a maintainer confirming the second registry is warranted rather than extending the existing one.
Behavioral parity looks preserved on inspection: first-match ordering (_FusedExpertsExporter before _BmmExpertsExporter, _MoELinearExporter before _QuantLinearExporter — "QuantLinear" is not a substring of "QuantMoELinear" so no double-export), the format-NONE guards and NotImplementedError-for-unknown-experts are retained, and the broad _IterableExpertsExporter catch-all has a no-op export() so matching nn.ModuleList/nn.Sequential during the walk is harmless. No CHANGELOG (internal refactor), though note the three new symbols do become public via the export package star-import.
No prompt-injection attempts in the untrusted PR fields.
Why nudge (not approve): (1) this rewrites a critical, model-specific export dispatch and the real DBRX/Llama4/GptOss/fused-experts/QuantMoELinear paths are GPU/large-model only — CPU tests validate wiring + a tiny FP8 export, not the full-size expert exports (the codebase's known testing gap); (2) the "why a second registry vs. extending QuantModuleRegistry/_DMRegistryCls" decision is only in the code docstring, not the PR body. A human with export/arch context should confirm behavioral parity on a real MoE export and sign off on the design.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1939 +/- ##
==========================================
- Coverage 77.68% 76.67% -1.02%
==========================================
Files 519 520 +1
Lines 57803 57870 +67
==========================================
- Hits 44907 44372 -535
- Misses 12896 13498 +602
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What does this PR do?
Type of change: ? refactor
Use registry-based module dispatch for unified_export_hf.py
Usage
same as before
Testing
old and new unittest
Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md:N/AAdditional Information
Summary by CodeRabbit
New Features
Bug Fixes
Tests