Repository navigation
feat(kimi-k3): model config, schema and HF->MaxText param mapping - #5548
snehalv2002 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Kimi-K3 model, adding its configuration, parameter mapping, and transformation hooks to enable checkpoint conversion and parity testing. Feedback on the changes suggests robustly handling PretrainedConfig objects by converting them to dictionaries using .to_dict() within the parameter mapping and hook functions to avoid potential AttributeErrors. Additionally, it is recommended to remove the @requires_kimi_k3_reference decorator from the coverage test so that it can run partially in CI/CD environments using fallback configurations when the local reference checkout is unavailable.
| def _resolve_kimi_k3_full_attn_layers(config) -> set[int]: | ||
| """Returns the 0-indexed set of full-attention (MLA) layer indices. | ||
|
|
||
| Two sources are accepted, with an explicit index base for each rather than sniffing: | ||
| * `config["linear_attn_config"]["full_attn_layers"]` -- the Hugging Face field. It is | ||
| 1-indexed by definition (`KimiLinearConfig.is_kda_layer` tests `layer_idx + 1`) and | ||
| is authoritative when present. | ||
| * `config["full_attn_layers"]` at the top level -- the MaxText convention used by | ||
| `kimi-k3.yml` and by test configs, 0-indexed. | ||
| Falls back to the production 93-layer layout if neither is present. | ||
| """ | ||
| num_layers = config.get("num_hidden_layers", 93) |
There was a problem hiding this comment.
If config is passed as a Hugging Face PretrainedConfig object (e.g., KimiK3Config) rather than a dictionary, calling .get() directly on it will raise an AttributeError since PretrainedConfig does not implement a .get() method.
To make this robust and support both dictionary and PretrainedConfig inputs, convert config to a dictionary using .to_dict() if the attribute is present. This is especially useful because PretrainedConfig.to_dict() recursively converts nested configurations (like linear_attn_config) into dictionaries as well.
def _resolve_kimi_k3_full_attn_layers(config) -> set[int]:
"""Returns the 0-indexed set of full-attention (MLA) layer indices.
Two sources are accepted, with an explicit index base for each rather than sniffing:
* config["linear_attn_config"]["full_attn_layers"] -- the Hugging Face field. It is
1-indexed by definition (KimiLinearConfig.is_kda_layer tests layer_idx + 1) and
is authoritative when present.
* config["full_attn_layers"] at the top level -- the MaxText convention used by
kimi-k3.yml and by test configs, 0-indexed.
Falls back to the production 93-layer layout if neither is present.
"""
if hasattr(config, "to_dict"):
config = config.to_dict()
num_layers = config.get("num_hidden_layers", 93)| def KIMI_K3_MAXTEXT_TO_HF_PARAM_MAPPING(config, maxtext_config, scan_layers=False): | ||
| """Generates parameter mapping from MaxText to Hugging Face for Kimi-K3.""" | ||
| num_main_layers = config.get("num_hidden_layers", 93) |
There was a problem hiding this comment.
Similar to _resolve_kimi_k3_full_attn_layers, config can be passed as a PretrainedConfig object, which will cause config.get to raise an AttributeError. Converting config to a dictionary at the start of KIMI_K3_MAXTEXT_TO_HF_PARAM_MAPPING ensures safe key access.
| def KIMI_K3_MAXTEXT_TO_HF_PARAM_MAPPING(config, maxtext_config, scan_layers=False): | |
| """Generates parameter mapping from MaxText to Hugging Face for Kimi-K3.""" | |
| num_main_layers = config.get("num_hidden_layers", 93) | |
| def KIMI_K3_MAXTEXT_TO_HF_PARAM_MAPPING(config, maxtext_config, scan_layers=False): | |
| """Generates parameter mapping from MaxText to Hugging Face for Kimi-K3.""" | |
| if hasattr(config, "to_dict"): | |
| config = config.to_dict() | |
| num_main_layers = config.get("num_hidden_layers", 93) |
| def KIMI_K3_MAXTEXT_TO_HF_PARAM_HOOK_FN(config, maxtext_config, scan_layers=False, saving_to_hf=False): | ||
| """Transformation hooks for Kimi-K3 parameters.""" | ||
| hidden_size = config.get("hidden_size", 7168) |
There was a problem hiding this comment.
Similar to the mapping function, config can be passed as a PretrainedConfig object in KIMI_K3_MAXTEXT_TO_HF_PARAM_HOOK_FN. Converting config to a dictionary at the start of the function ensures safe key access.
| def KIMI_K3_MAXTEXT_TO_HF_PARAM_HOOK_FN(config, maxtext_config, scan_layers=False, saving_to_hf=False): | |
| """Transformation hooks for Kimi-K3 parameters.""" | |
| hidden_size = config.get("hidden_size", 7168) | |
| def KIMI_K3_MAXTEXT_TO_HF_PARAM_HOOK_FN(config, maxtext_config, scan_layers=False, saving_to_hf=False): | |
| """Transformation hooks for Kimi-K3 parameters.""" | |
| if hasattr(config, "to_dict"): | |
| config = config.to_dict() | |
| hidden_size = config.get("hidden_size", 7168) |
| @requires_kimi_k3_reference | ||
| def test_kimi_k3_param_mapping_100_percent_coverage(self): |
There was a problem hiding this comment.
The test_kimi_k3_param_mapping_100_percent_coverage test is currently decorated with @requires_kimi_k3_reference, which skips the entire test if the large reference checkout is not present locally.
However, the first part of this test (verifying that the parameter mapping and hooks contain all expected keys) runs perfectly fine using the fallback self.text_config dictionary defined in setUp. The only part requiring the reference checkout is the index_file check, which is already safely guarded by if os.path.exists(self.index_file):.
Removing the @requires_kimi_k3_reference decorator from this test allows it to run in CI/CD environments, providing valuable continuous integration coverage for the Kimi-K3 parameter mapping.
| @requires_kimi_k3_reference | |
| def test_kimi_k3_param_mapping_100_percent_coverage(self): | |
| def test_kimi_k3_param_mapping_100_percent_coverage(self): |
There was a problem hiding this comment.
No longer applies. The test was split, and key coverage now runs without the reference checkout. Only the safetensors-index check is gated.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
b332c56 to
f341ad9
Compare
f341ad9 to
1af3969
Compare
1af3969 to
38df8a0
Compare
|
Review notes on PR #5548 (in addition to the open Blocking
Should-Fix / Minor
|
|
@AusarYao28 thanks for the review!
|
38df8a0 to
d8b471c
Compare
|
Thanks for addressing all the config, One quick import issue to avoid breaking the downstream sync when this merges: maxtext/src/maxtext/checkpoint_conversion/utils/hf_shape.py Lines 1676 to 1681 in d8b471c Every other shape helper in hf_shape.py is pure stdlib with zero maxtext.* imports, and the hf_shape library target in the internal BUILD file has deps = [] (BUILD files are excluded from automated sync). Even a function-scoped from maxtext.checkpoint_conversion.utils.param_mapping import ... inside hf_shape.py triggers a static strict-deps build failure on hf_shape during import. Keeping small local copies of _kimi_k3_text_config, _kimi_k3_require, and _resolve_kimi_k3_full_attn_layers inside hf_shape.py keeps hf_shape.py self-contained and avoids breaking the import presubmit.
|
Add Kimi-K3 architecture configuration, Pydantic configuration schema, and HuggingFace parameter mapping and transformation hooks.
d8b471c to
ef04f65
Compare
|
@AusarYao28 Done. |
feat(kimi-k3): model config, schema and HF->MaxText param mapping
Foundation PR for the Kimi-K3 (
moonshotai/Kimi-K3, 93-layer hybrid KDA/MLA, latent MoE, attention residuals) bring-up.What
kimi-k3model name (types.py,pyconfig_deprecated.py),DecoderBlockType.KIMI_LINEAR, HF id inglobals.py, flattened HFtext_config+KimiK3Configinhf_model_configs.py.kimi-k3.yml: production dims, 0-indexedfull_attn_layers,scan_layers: false(the stack is unscanned-only).KimiK3schema mixin: only fields later PRs read (attn_res_block_size,full_attn_layers,routed_expert_hidden_size,shared_intermediate_size,moe_intermediate_size,num_shared_experts,short_conv_kernel_size). A validator rejectsmoe_intermediate_size/num_shared_expertsvalues that differ frombase_moe_mlp_dim/shared_experts.PARAM_MAPPING/HOOK_FNS["kimi-k3"]: HF->MaxText, unscanned.scan_layers=TrueraisesNotImplementedError. The layer layout comes from HFlinear_attn_config.full_attn_layers(1-indexed) or a top-levelfull_attn_layers(0-indexed), with no default: a missing layout raises.A_logpadding (128 -> 96) is sliced only when the tail is all zeros. No vLLM hook is registered.tests/utils/kimi_k3_conversion_utils.py,kimi_k3_reference.py): only later PRs (PR3+) use them. Key matching respects.boundaries and prefers the longest match. Hooks can receivetarget_shapes. The reference dir can be overridden withKIMI_K3_HF_REFERENCE_DIR..gitignoreignores the defaultkimi-k3-hf-reference/.Size: 802 src / 742 test lines (vs. its stack base).
Testing
kimi_k3_config_test.pyloads the yml throughpyconfigand checks it againstkimi_k3_dict. It also checks the alias validator, scan/layout errors, and per-layer mapping keys for all 93 layers, and runs the hooks on random tensors. All of that runs in CI. The test that compares against the releasedmodel.safetensors.index.jsonneeds the local reference, so CI skips it.Known limitations
kimi_lineardecoder lands in PR8, and PR2 fixesmlp_activations.saving_to_hf=True) is untested, andhf_shape.pyhas no Kimi-K3 entry, soto_huggingfaceexport is unsupported.tokenizer_type: tiktokenhas no path yet (PR7).Stack: merge order PR0, PR1 (this), PR2..PR7, PR8, PR9, PR10 (#6014).
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.