build(base): rebase the Baseten patches onto NVIDIA-NeMo main (e1967a9c) - #34
Conversation
There was a problem hiding this comment.
Pull request overview
Vendors NVIDIA’s upstream Kimi-K3 language-backbone implementation into megatron.bridge, including the Megatron↔HF conversion bridge, K3-specific layer/spec wiring, and MXFP4 quantization utilities needed by K3 checkpoints.
Changes:
- Add the Kimi-K3 model provider, layer spec, custom layers/ops, and pipeline stage-boundary packing helpers.
- Add a Kimi-K3
MegatronModelBridgewith parameter mappings and MXFP4 expert dequant/requant handling. - Extend MXFP4 E2M1 packed (de)quantization to support
uint8E8M0 scales, plus a unit test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/models/test_quantization_utils.py | Adds coverage for MXFP4 E2M1 packed roundtrip when scales are uint8 E8M0. |
| src/megatron/bridge/models/kimi/kimi_k3_spec.py | Defines a K3-specific MCore layer spec (heterogeneous KDA/MLA + SiTU activation wiring). |
| src/megatron/bridge/models/kimi/kimi_k3_provider.py | Introduces KimiK3ModelProvider with K3-specific config knobs. |
| src/megatron/bridge/models/kimi/kimi_k3_pipeline.py | Implements AttnRes stage-boundary pack/unpack helpers for PP. |
| src/megatron/bridge/models/kimi/kimi_k3_ops.py | Adds K3 numerical ops (KDA wrapper, SiTU, Kimi RMSNorm, AttnRes aggregation). |
| src/megatron/bridge/models/kimi/kimi_k3_layers.py | Adds K3 attention/MoE/transformer-layer implementations. |
| src/megatron/bridge/models/kimi/kimi_k3_bridge.py | Adds HF↔Megatron conversion bridge and mapping registry for K3, including MXFP4 expert handling. |
| src/megatron/bridge/models/kimi/init.py | Exports K3 bridge/provider from the Kimi package. |
| src/megatron/bridge/models/conversion/quantization_utils.py | Adds uint8 E8M0 scale decode/encode support for MXFP4 packed weights. |
| src/megatron/bridge/models/init.py | Exposes K3 bridge/provider at the top-level models package. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/code-review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/megatron/bridge/models/kimi/init.py:17
- The new Kimi K3 re-export imports are missing
# noqa: F401, which is used consistently in other model__init__.pyfiles to avoidF401(unused import) lint errors for symbols that are re-exported via__all__(e.g.,src/megatron/bridge/models/llama/__init__.py:15).
from megatron.bridge.models.kimi.kimi_bridge import KimiK2Bridge # noqa: F401
from megatron.bridge.models.kimi.kimi_k3_bridge import KimiK3Bridge
from megatron.bridge.models.kimi.kimi_k3_provider import KimiK3ModelProvider
src/megatron/bridge/models/kimi/init.py:24
- The PR description says the upstream squash SHA (
2f0f8c80) is recorded inmodels/kimi/__init__.py, but it is not present in this module. If provenance is meant to survive outside the PR description, consider recording it (e.g., as an exported constant) here.
__all__ = [
"KimiK2Bridge",
"KimiK3Bridge",
"KimiK3ModelProvider",
]
src/megatron/bridge/models/kimi/kimi_k3_provider.py:32
- The PR description mentions the upstream squash SHA (
2f0f8c80) is recorded onKimiK3ModelProvider, but the class currently has no provenance marker. If this is required for vendoring/auditing, add aClassVarso it’s available without becoming a dataclass field.
@dataclass
class KimiK3ModelProvider(MLAModelProvider):
"""Megatron configuration and provider for Kimi K3."""
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/megatron/bridge/models/kimi/init.py:23
- PR description says the upstream squash SHA
2f0f8c80is recorded inmodels/kimi/__init__.pyand onKimiK3ModelProvider, but this diff only adds K3 imports/exports here and the provider has no provenance field/comment. Please either add the provenance marker to the code (as described) or update the PR description so it matches what’s actually in this vendored layer.
from megatron.bridge.models.kimi.kimi_bridge import KimiK2Bridge # noqa: F401
from megatron.bridge.models.kimi.kimi_k3_bridge import KimiK3Bridge
from megatron.bridge.models.kimi.kimi_k3_provider import KimiK3ModelProvider
__all__ = [
"KimiK2Bridge",
"KimiK3Bridge",
"KimiK3ModelProvider",
There was a problem hiding this comment.
Wait, actually, I think since you're taking from the main branch instead of the dev branch, we should just rebase up to that point and then apply the baseten patches on top of that.
because currently trainers-main history looks like this:
[nvidia main, time 0 to t][our own patches]
so like if we want to take stuff from main, we should rebase and have
[nvidia main, time 0 to t][nvidia main, from t to t'][our own patches]
at least, this is my understanding of what it currently looks like - wdyt?
cause this way we get the base upon which yaoyu-33's kimi implementation might have depended on, and also keeps the lineage clean for when we want to take MORE commits from main because of future models like kimi k4, k5, ... etc
4131ba7 to
1b9880a
Compare
|
@JackRao123 done, and you were more right than either of us thought. This is now a base bump: Two things fell out that I did not expect: Upstream had already taken our FP32 patch. NVIDIA main carries The rebase surfaced a real incompatibility the cherry-pick was hiding. Upstream MCore now pins Scope, so you can judge the risk: 496 upstream bridge commits and 280 MCore commits adopted; our MCore was diverged from what the new bridge expects (ahead 10, behind 280), which is why the paired MCore PR exists and has to merge first. Our whole delta above the new base is 13 files, +329/-21 - Four conflicts total across both repos, all unions or supersets, all hand-resolved. Worth flagging: on a first pass I let Nothing is GPU-tested yet. The matrix needs to cover GLM-5.2 THD-CP, DSv4-Flash, MiMo and Qwen as well as K3, since these patches serve all of them - your |
Automated review of the replay: three real defects fixed, the rest triagedI ran a review over
The All three come from the same mistake, and it is the same one the Verified as pre-existing, not from this PR
Same category, lower severity, all pre-existing: Latent, worth knowing before this fork enables its CIUpstream's vendored It is not firing today - Not fixed, with reasons
|
a030aa5 to
1e60218
Compare
Cherry-picked the Baseten patches from
trainers-mainonto NVIDIA-NeMo/Megatron-Bridge maine1967a9c(2026-08-19), pinned rather than tracking the branch. History is[NVIDIA commits][11 Baseten commits], linear, no merge commits. Delta above the base: 13 files, +315/-20.The submodule points at
a7234ad552fa, the newbasetenlabs/Megatron-LM:trainers-main(basetenlabs/Megatron-LM#33, merged).trainers-mainwas NVIDIA main at7ee8b69d(2026-07-09) plus 17 commits; NVIDIA has moved 504 commits since. This PR no longer vendors Kimi-K3 - the new base already contains it, so there is nothing to cherry-pick for K3.How to review 1600 files
Don't. The upstream range is upstream's.
git diff e1967a9cb6d6..HEADis our entire delta - 13 files - and that is the part worth reading.Accounting for all 17 original commits
11 applied - Paras's GLM-5.2/HF compat, GLM DSA fused backend and the
HybridStackrecompute hook; Jack's FP8 context-parallel launches (#19); Shreya's sixcheck_importscold-cache commits; Ian'sasync_ckpt_use_cpu_shm.3 were gitlink-only MCore bumps (
f7be4c14,57d5f291,20fcf2ea), superseded by the new pin.2 were merge commits, whose constituents applied individually.
1 skipped as obsolete -
1270ea0e"keep checkpoint save strategy compatible with upstream MCore". It existed as atry/exceptfallback for MCore versions withoutget_default_save_sharded_strategy. The new MCore defines it (serialization.py:58), and upstream's bridge now calls neither that helper norget_default_load_sharded_strategy- 0 call sites, down from 3 on our old branch. Replaying it would add a dead shim plus an unused import that tripsruff F401.Conflicts
Two, both trivial:
glm_moe_dsa/glm5_bridge.py- upstream addedfrom typing import Anywhere our patch addsimport os. Kept both.3rdparty/Megatron-LM- the gitlink, twice. Resolved to the new MCore commit.One defect caught during the replay
Ian's
async_ckpt_use_cpu_shmhunk callsinspect.signature, and upstream'scheckpointing.pydoes not importinspect- our old branch did, at module scope. Cherry-picking the hunk alone left the file using a name it never imports, so the firstsave_checkpointwith the feature enabled would die withNameError.import inspectis folded into that commit.This is the second time that exact gap appeared in this file, so it is worth stating the rule: after resolving a conflict, check the whole file compiles against its imports, not just that the hunk looks right.
Verifying
Testing
Not run - the suite needs GPUs. Static only: no conflict markers, every changed file parses, imports match usage. The gate is the validation matrix, which has to cover GLM-5.2 THD-CP, DSv4-Flash, MiMo and Qwen as well as Kimi-K3, since these patches serve all of them.