Conversation
|
PR-Agent gate blocked at exact head |
seanphan
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED at exact head bc2bb6e8aeb20e92dfe8a8602485fa262b606355.
The attempt is correctly placed and I found no sanitization issue. The following decision-critical claims need correction before approval:
-
[blocking: inferred/untested size] Recompute the artifact size from the complete pinned checkpoint inventory. The official checkpoint at
04c4e9e95c5da8862dced7e5056455116f83a7e0contains 321,323,031,390 parameters, while this note counts 313,327,015,936, leaving 7,996,015,454 parameters unaccounted for. The omitted set includes MTP/vision and architecture-specific tensors, so 171.2 GB is not a complete artifact estimate. The4 × 4096 × 8192 × 45attention shortcut also does not represent the Glm5Next hybrid KDA/NoPE sparse-MLA projections in the pinned config. Recompute from the pinned safetensors index/config and state which tensors remain FP8/BF16 or are excluded. -
[blocking: inferred size] Fully specify the compressed-tensors schemes before claiming exact packing. “Group-wise” does not define group size, symmetry, scale dtype, zero-point dtype, or target precedence. Applying one 1.0625 multiplier to both W4 and W8 is not generally valid. Pin a concrete config, show byte arithmetic per scheme including scales/zero-points/padding, and label the result inferred/untested until an emitted checkpoint is measured.
-
[blocking: unsupported runtime/topology] PR #53906 alone is not a serving path for the stated SM80 node. At the cited upstream head it implements the official GLM path for SM90/SM100/SM120, not SM80; PP is explicitly gated off, so PP4 is unsupported there. TP4 is only plausible/untested. On four PCIe GPUs without P2P, vLLM custom all-reduce is not supported for this topology and must be disabled; host-routed TP collectives are a material performance risk. Replace “no additional serving-side patch is required beyond #53906” with the actual blocker, separate any community backport from upstream vLLM, and label TP4 untested / PP4 unsupported. Also change “~85 GB headroom” to aggregate unallocated VRAM before runtime state, rank imbalance, quant buffers/repacking, graphs, NCCL, model-state omissions, and KV cache.
-
[blocking: inferred storage/evidence] The ~340 GB scratch requirement is not supported. The pinned official FP8 safetensors alone are 328,337,455,672 bytes; source plus a ~171–190 GB output exceeds 500 GB before temporary/offload data. Define whether “scratch” excludes separately stored source/output and give a toolchain-specific peak-disk/host-RAM estimate. Pin all model/runtime/quant source links to revisions; link the vLLM issues directly; and use the repository evidence labels consistently for every external, inferred, and untested claim.
Primary references: official pinned model, official pinned config, vLLM GLM support PR, compressed-tensors MoE integration, runtime issue #54317, and runtime issue #54458.
GitHub does not permit the PR author to submit a formal REQUEST_CHANGES review, so this COMMENT review carries the explicit CHANGES_REQUESTED verdict.
fix: add measured UD-IQ4_XS baseline on CMP 170HX
|
Corrected per exact-head review 5063926089 (all 4 blockers):
Fix head: f331479 (supersedes bc2bb6e via merge commit, no force-push). Requesting fresh independent exact-head review at this head. |
seanphan
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED at exact head f331479572c27487f498a39ba3cc49f30d23d641.
The branch now contains merged PR #7 main as an ancestor; the exact-head diff against current main is only the new attempt note. git diff --check, commit metadata, large-file/symlink checks, and targeted publication-boundary scans pass. Four decision-critical blockers remain:
-
[blocking: arithmetic/inventory] The corrected artifact estimate still quantizes source scale tensors and the displayed formula does not equal its result. The pinned Hugging Face metadata reports 321,323,031,390 model parameters; the note's 321,342,220,638 “parameters” adds 19,189,248 auxiliary quantization-scale elements. At lines 55-56,
311,653,564,416 × 0.5 + (311,653,564,416 / 128) × 2is 160,696,369,152 B, not 160,706,177,280 B. The stated result is exactly what results from first adding the existing 19,021,824weight_scale_invelements and then W4-packing them, even though those source FP8 scales must be replaced—not treated as model weights in the emitted scheme. Likewise, the pinned indexmetadata.total_sizeis 328,326,771,576 tensor-payload bytes; if 328,337,455,672 is the 62 LFS blob sum including safetensors headers, label that distinction. Recompute output bytes from the emitted tensor classes, separately reporting model parameters, source auxiliary scales, new scales, padding, and shard/header overhead. Primary receipt: pinned safetensors metadata and pinned index. -
[blocking: the scheme/tensor map is still not frozen and conflicts with the owner contract] Lines 12-21 define W8 for “all other Linear” while preserving norms/KDA/non-linear state, but lines 57-60 apply W8 + scale overhead to every remaining tensor element. That bucket includes embeddings, norms, KDA/state tensors and source auxiliary scales, so it cannot implement the stated target set. The tracking contract also preserves embeddings/output ties, biases, routers and unsupported state, and requires shared-expert treatment to be decided from the frozen tensor map. The note commits neither a concrete
config_groupsmanifest nor an exhaustive included/excluded tensor inventory. Also, the source FP8[16,32]scale layout is 128×128 block scaling, while the estimate assumes one output FP16 scale per 128 weights; it does not “verify” the planned WNA16 grouping. Commit the exact config and machine-checkable per-tensor classification/counts before claiming gate 1 complete. -
[blocking: runtime/quant claims remain mutable rather than pinned] PR #53906, PR #48918, llm-compressor #2940, “vLLM main compressed_tensors.py,” and the community backport are all linked or named without immutable revisions. #53906 is already at
f221389f16e02712b0e8bd82a485017a24a18453, different from the head used in the original research. Pin commit URLs for every implementation claim, link/license the chosen community backport, and record status as-of the review date. Reconcile gate 3's TP4 plan with the current #66 decision record, which now prefers a separately pinned community PP4 lane after import validation. -
[blocking: PR body and tracking link are stale] The public PR body still says 171.2 GB / 159.4 GiB and “~85 GB headroom for KV/context,” contradicting the note's 170.5 GB / 158.8 GiB / 97.2 GiB not-serving-headroom wording; it also contains literal
\nescapes. Its unqualified “issue #66” points at a nonexistent issue in this repository rather than the actual owner ticket. Update the body after the artifact math/tensor contract is corrected.
This review is a COMMENT because GitHub does not allow the PR author/account to submit a formal request-changes review; it carries the explicit CHANGES_REQUESTED verdict for this SHA.
|
VERDICT: CHANGES_REQUESTED Independent exact-head review of
Re-review at the next head should confirm: W4/W8 formulas equal their displayed results, scales reported separately from model parameters, a frozen config_groups manifest with per-tensor counts, commit-pinned runtime/quant references, and a body matching the note. |
Static/toolchain feasibility gate for mixed W4A16-experts + W8A16-dense quantization, per issue #66. Repo-only; no GPU, download, or quantization run.\n\n- New attempt note: attempts/w4a16-experts-w8a16-dense-vllm/README.md (status: static-fit-only)\n- Confirms per-target mixed-scheme support is native to the vLLM compressed-tensors loader (config_groups dispatch), W4/W8 both route through WNA16, MoE path is wired via the merged PR 48918.\n- Names the true blockers: Glm5Next only exists via open/blocked vLLM PR 53906 (plus open bugs #54317/#54458), and no mixed checkpoint exists publicly; producing one needs an explicit resource lease.\n- Estimated weight footprint ~171.2 GB (~159.4 GiB), ~9% over the measured UD-IQ4_XS baseline, fits a 4x64 GiB node with ~85 GB headroom for KV/context.\n\nGates 2-5 (recipe, TP4 boot smoke, quality spot-check, full ladder) are written as pending-resource and stay closed until issue #66 records a lease.\n\nTest plan:\n- rg sanitization scan clean (no private hosts/IPs/paths)\n- git diff vs origin/main touches only the new attempt note\n- All size arithmetic shown inline and labeled inferred\n- No executable code changed; bench/ untouched