Add DeepSeek-V4 and GLM-5 support - #29
Conversation
|
/broly scan |
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
9939caa to
ae9cefe
Compare
qywu
left a comment
There was a problem hiding this comment.
Reviewed: description is clear, CI passing, no suspicious file changes. LGTM.
qywu
left a comment
There was a problem hiding this comment.
Deep review summary
I read through the full ~14.7k-line diff (checkpoint converters/handlers, sparse-attention kernels/indexers for both DSv4 and GLM-5, parallelize/registry wiring, and the new test suites), hand-verified the tensor-shape algebra in the checkpoint fusion/dequantization paths, and cross-checked the shared/common-code edits (deepseek_v3, glm4_moe modeling files) for regression risk.
What holds up: the FP8 block-dequant, MXFP4 unpack, and per-expert gate/up/down fusion transforms in deepseek_v4/checkpoint_handler.py are internally consistent (I traced the shapes by hand — HF [out,in] Linear weights are correctly transposed before stacking into xorl's [E,H,2I]/[E,I,H] layout). The shared-code edits to deepseek_v3/modeling_deepseek_v3.py and glm4_moe/modeling_glm4_moe.py are additive getattr(..., default) reads, so existing V3/GLM4-MoE behavior is preserved. The new end-to-end tests (test_dsv4_hf_to_dcp.py, test_dsv4_loader.py) do real value/shape assertions rather than "just don't crash" smoke checks, and EP divisibility is asserted with clear errors in both the DSv4 and GLM-5 checkpoint handlers and the converter script.
Non-blocking concerns worth a second pair of eyes before this touches production checkpoints:
-
src/xorl/models/transformers/deepseek_v4/checkpoint_handler.py(_undo_ape_hotfix, ~L727-754) and its test intests/models/test_dsv4_loader.py(test_ape_hotfix_round_trip, ~L11072-11088). The round-trip test validates_undo_ape_hotfixagainst_miles_apply_ape_hotfix, a from-scratch reimplementation written in this same PR to mimic what the external "miles" export tool allegedly does. It's internally consistent (I checked the permutation algebra by hand and it does invert), but it never validates against a real HF/SGLang-exported checkpoint or the actual upstream miles source — so if the test author's guess at the forward permutation is wrong, this would silently produce a wrong-but-correctly-shapedcompressor.apefor every C4 (DSA) layer, and nothing in the suite would catch it. Given the PR's own validation notes only claim "imports passed" / "72 passed", worth explicitly confirming this against a real Flash/Pro checkpoint (or the miles source) before it's used for real conversions. -
src/xorl/ops/dsv4/kernel/tilelang_indexer_fwd.py:7457-7465(_make_causal_cu_seqlens). The causal boundary for compressed KV groups iscu_seqlen_ke = (positions + 1) // compress_ratio(floor division). This is documented as intentional ("valid compressed groups are[0, (p+1)//compress_ratio)"), and is self-consistent with the rest of the indexer, but I could not find a test that checks this boundary against a known-correct reference for a partially-filled trailing compression group (e.g. verifying groupgonly becomes visible once its last member position has been produced, not before). An off-by-one here would leak future-token information into the sparse index scores without raising any error — worth a targeted unit test asserting the exact boundary token index if one doesn't already exist upstream in the stack. -
Minor:
scripts/convert_dsv4_hf_to_dcp.py_maybe_truncate_config(~L84-91) mutatescfg.num_hidden_layers/cfg.compress_ratiosdirectly on an already-constructedDeepseekV4Config, bypassing the constructor's owncompress_ratioslength/value validation (configuration_deepseek_v4.py~L1849-1862). Low risk since it's only used for converter smoke/truncation runs, but a truncation that produces an inconsistent length would not be caught the way full-config construction would.
None of the above are proven-wrong logic bugs — they're places where correctness rests on an unverified external contract (the real "miles"/SGLang export format) rather than something checkable from this diff alone. I did not find resource leaks, unsafe deserialization, path traversal, or dead/unwired code — the DSv4/GLM-5 registrations in auto.py and transformers/__init__.py are wired correctly, and the shared-code touches are safe defaults.
Posting as a comment (non-blocking) rather than requesting changes, since nothing here is a demonstrated defect — just spots that deserve explicit verification against real checkpoints given how much downstream correctness depends on them.
ae9cefe to
e556408
Compare
e556408 to
1530ce9
Compare
1530ce9 to
0b499ee
Compare
Summary
Add DeepSeek-V4 and GLM-5 support: sparse-attention kernels and indexers, conversion support, checkpoint handlers, parallelization, registry wiring, and focused model coverage. The shared registry and checkpoint-key hunks are layered on the preceding common-MoE cut.
Validation
Stack
This is 5/9, stacked on the common-MoE/model-family cut.