Repository navigation
Restore the MXFP4 dequantize transpose on the transformers 5.x load path - #1114
Merged
Merged
Conversation
Zoo's convert_moe_packed_tensors replacement returns the un-transposed [E, D, G*B*2] layout on purpose and leaves transpose(1, 2) to whichever loader hook is live. That hook is mxfp4.dequantize only on 4.x. From transformers 5.0 the live path is Mxfp4Dequantize (a ConversionOps) into dequantize_convertops, so the transpose was never applied and GPT-OSS loaded with dims 1 and 2 swapped. 5.16.0 then deleted dequantize and reduced shard_and_distribute_module to a raising tombstone, which is what turned the drift detector red. Patch dequantize_convertops on 5.x, gate the legacy dequantize patch to < 5.0.0, and gate both shard_and_distribute_module call sites to < 5.16.0. Also drop set_param=False: it only exists on 5.x, and the branch that passed it is now 4.x only, where the signature has no **kwargs to absorb it and every tensor parallel load would TypeError. Add tests/test_mxfp4_load_path_layout.py, which asserts the loaded layout against pristine transformers. The existing mxfp4 tests only cover the save path, which stayed self-consistent and missed this entirely.
Existence and signature pinning alone is what let this bug run from transformers 5.0.0 to 5.16.0. Zoo patched mxfp4.dequantize while every 5.x release loaded through Mxfp4Dequantize into dequantize_convertops. The symbol existed with an unchanged signature the whole time, so every detector stayed green while the patch reached nothing. Add _referenced_global_names and _assert_on_live_path, which walk a caller's code objects (nested code included, so a call inside a comprehension or nested def is still seen) and fail when the symbol zoo patches has no live caller. Wire it into the ConversionOps detector, and add the 4.x equivalent asserting quantizer_mxfp4 still calls the legacy hook. The 4.x check skips rather than fails when transformers.quantizers cannot import, since a broken optional backend says nothing about drift.
…usly Two gates that did not gate. The layout regression test was in no workflow. Zoo CI runs explicitly named files, so it was only ever collected by `pytest tests/ --collect-only`, which proves it imports and nothing more. It now runs in the Core drift job, whose three lanes resolve to 4.57.6, 5.5.0 and 5.16.x: two affected versions gate it and one exercises the skip. That job sets UNSLOTH_IS_PRESENT in env, which the probe subprocess needs and conftest cannot reach, and it has no continue-on-error and propagates its lane status. The reachability check read co_names, which mixes attribute operands in with globals, so every self.quantization_config.dequantize config read counted as a call site. On transformers 5.16.1, where upstream genuinely removed the call, it still reported three callers against zero real ones, so it would have stayed green through the entire window it exists to prevent. Filtering on the opcode narrows 4.x detection to create_quantized_param alone.
Compress the comments and docstrings added by this PR, and fix three statements about upstream transformers: - set_param was removed in 4.57.0 and is absent from 5.x, not a 5.x only kwarg - shard_and_distribute_module was not deleted in 5.16.0; it still imports and is a tombstone that raises only when called, so the gates are defensive - the dropped transpose affects 5.1.0 and newer, since 5.0.0 dequantize_convertops is still 3-arg
The core-upstream lane installs unsloth with `|| true`, so the checkout is
allowed to fail. The probe subprocess then imports unsloth_zoo, which raises at
__init__.py:250 when find_spec("unsloth") is None, and the failure names
unsloth rather than the layout this test exists to police.
Neither variable already in play covers it. UNSLOTH_ALLOW_CPU only bypasses
accelerator detection, and UNSLOTH_IS_PRESENT guards a separate later check
that the import never reaches, so the comment above the invocation named the
wrong mechanism as well. The probe now sets UNSLOTH_ZOO_DISABLE_GPU_INIT.
Verified with transformers 5.16.1 and unsloth genuinely absent: before, the
test fails with the unsloth ImportError; after, it passes. Reverting the source
still fails with the layout diagnostic, so the gate is intact.
danielhanchen
added a commit
to shimmyshimmer/unsloth-zoo-staging-2
that referenced
this pull request
Aug 27, 2026
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danielhanchen
added a commit
to shimmyshimmer/unsloth-zoo-staging-2
that referenced
this pull request
Aug 27, 2026
danielhanchen
added a commit
to shimmyshimmer/unsloth-zoo-staging-2
that referenced
this pull request
Aug 27, 2026
This was referenced Aug 27, 2026
danielhanchen
added a commit
that referenced
this pull request
Aug 27, 2026
The guard asserted the lane invokes pytest on exactly 20 files. #1114 added tests/test_mxfp4_load_path_layout.py to that lane, making it 21, so the guard went red on a legitimate addition and blocked the fix for a real bug. The risk it exists for is a continuation that silently drops files. A required set catches that and names which file went missing, where a count only says that one did. Additions no longer fail; a removal means dropping the name here in the same commit, which is the deliberate act it should be. Also reject a file named twice, which would parse as two entries and mask a drop from the set check. Co-authored-by: danielhanchen <unslothai@gmail.com>
4 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GPT-OSS MXFP4 has been loading with dims 1 and 2 swapped on every transformers 5.x
The red CI is the alarm, not the fire.
unsloth_zoo/temporary_patches/mxfp4.pyreplacesconvert_moe_packed_tensorswith a variant that deliberately returns the un-transposed[E, D, G*B*2]layout, and leaves it to whichever loader hook is live to restore GPT-OSS's[E, G*B*2, D]. Which hook that is depends on the transformers version:mxfp4.dequantize, called fromquantizer_mxfp4. Zoo patches it, and it applies the transpose. Correct.Mxfp4Dequantize(aConversionOps) intodequantize_convertops. Nothing callsdequantizeany more.Zoo only ever patched
dequantize. So from transformers 5.0.0 onward the compensating transpose was silently dropped and the model loaded with dims 1 and 2 swapped.dequantizestill existed on 5.0 through 5.15, so the patch appeared to apply and nothing complained.Measured with the real
patch_convert_moe_packed_tensors():(2, 4, 96)(2, 96, 4)(2, 4, 96)(2, 96, 4)What actually changed upstream
Both CI failures come from a single upstream PR, huggingface/transformers#47579 (the DTensor tensor parallel rewrite), first released in v5.16.0:
mxfp4.dequantizeshard_and_distribute_module(*args, **kwargs)tombstone that raisesRuntimeErrordequantizewas not renamed or moved, it was deleted as dead code. Thetlatest5-trl1latestdrift lane installs unpinnedtransformers>=5,<6, so it picked 5.16.0 up the day it shipped, with no zoo commit to blame.Worth stating for the next person who reads the drift output:
shard_and_distribute_modulereporting(*args, **kwargs)is not a decorator wrapper hiding a real signature. It has no__wrapped__,__closure__isNone,co_argcountis 0, andco_filenamepoints attensor_parallel.py:54, the tombstone's own code object.inspect.signature(..., follow_wrapped=True)returns the identical thing. The detector was reading the truth; the source was wrong.Attribution: no zoo commit is to blame
Worth stating outright so nobody has to re-derive it. The regression was introduced by an upstream release, not by any commit in this repo. The
tlatest5-trl1latestdrift lane installs unpinnedtransformers>=5,<6, so it adopts each new transformers the moment it is published, with no zoo change involved.The run history dates it to the second:
Core driftjob on183a1ee49, greenCore driftjob onaf37769fb, redThe last green drift job finished two seconds before the breaking release existed on PyPI. The next one started three and a half hours later, after the queue, and installed 5.16.1.
af37769fb(#1108) is simply the commit that happened to be atmainwhen the next run started. It touchescompiler.py,empty_model.py,peft_utils.py,saving_utils.py,tokenizer_utils.py,vllm_utils.py, lint config and security tests, and no mxfp4, tensor_parallel or gpt_oss code at all. Blaming it would be wrong. The 5.4 hour queue delay between run creation and job start is what made an unpinned lane flip mid-day.Was upstream right to do this?
Taking the other reading seriously rather than assuming the newer change is correct:
On
dequantize, upstream is right and the removal is clean. It was genuinely dead code from 5.0.0 onward:get_weight_conversions()has returnedMxfp4Dequantizesince v5.0.0, and nothing has called the module leveldequantizesince. Upstream also removed the matching re-export fromtransformers/integrations/__init__.py, so there is no dangling public name left behind.from transformers.integrations import dequantizeworked on 5.4.0 and now raisesImportErrorcleanly rather than resolving to something broken.One inconsistency worth noting, not worth an issue.
dequantizewas publicly re-exported, and it was hard deleted in a minor release with no deprecation cycle, while its sibling in the very same PR,shard_and_distribute_module, got a properFutureWarningtombstone. Two public symbols removed by one PR, two different removal policies. The tombstone is the better treatment and is why that half of the breakage produced a legible error instead of anAttributeError. This does not change the fix on our side and I am not filing anything upstream, but if someone else hits thedequantizeremoval cold, that asymmetry is the reason it is harder to diagnose than it should be.The fix
Patch
dequantize_convertopson 5.x. It is a clean choke point:Mxfp4Dequantize.convertis its only caller, verified on both 5.4.0 and 5.16.1. Critically this leaves zoo's un-transposed convention intact, sosaving_utils._mxfp4_base_returns_transposedand the export convention tests need no changes at all.Rejected alternatives:
convert_moe_packed_tensorsself-transpose. Double-transposes on stock 4.55.x, and forces coherent edits tosaving_utils, the CPU export branch, and both convention tests.convert_moe_packed_tensorspatch on 5.16.0+. Only fixes 5.16+, leaves 5.0 through 5.15 corrupt, and desyncs the_cpu-presence signalsaving_utilskeys off.The legacy
dequantizepatch is gated to< 5.0.0and bothshard_and_distribute_modulecall sites to< 5.16.0.Two more defects CI never reported
temporary_patches/gpt_oss.py:476also callsshard_and_distribute_modulewith 8 positionals. Its patch targetload_and_swizzle_mxfp4was dropped in the same 5.16.0 release, so the patch is inert there, but the import bailed the wholepatch_gpt_ossearly and tookreplace_with_mxfp4_lineardown with it. Now resolved leniently.set_param=False, which only exists on 5.x. Now that the branch carrying it is 4.x only, that kwarg would hit a 4.57.6 signature with no**kwargsto absorb it andTypeErroron every 4.x tensor parallel load. Dropped, matching upstream 4.57.6's own 8 positional call.Test evidence
-k mxfp4, before-k mxfp4, afterRemaining failures on both versions are a pre-existing local bitsandbytes artifact (
libbitsandbytes_cuda128.somissing in my workspace), identical on pristinemainand unrelated. CI installs a working bitsandbytes.The drift tests do not catch the corruption. Reverting only the source fix from a byte snapshot, with the test changes left in place:
That is the single most important result here. Version gating the two red tests would have turned CI green over a broken loader.
New regression test
tests/test_mxfp4_load_path_layout.pyasserts the loaded layout against what pristine transformers produces for the same input. This gap is why the bug survived since 5.0.0:test_mxfp4_transpose_conventionandtest_mxfp4_convert_no_double_transposeboth exercise the save path, which stayed self-consistent throughout.Subprocess isolated because applying the patch mutates
transformers.integrations.mxfp4process-wide, CPU only,UNSLOTH_ALLOW_CPU=1since conftest's GPU-free harness does not reach a subprocess. It captures its golden value before patching rather than hardcoding a shape, so it needs no version gating of its own.Mutation tested in both directions. With the source reverted it fails on 5.16.1 and on 5.4.0, catching the pre-5.16 corruption that no existing test saw:
It skips cleanly on 4.57.6, which has no
dequantize_convertops.Deliberate scope boundary
Tensor parallel MXFP4 loading on transformers 5.16.0+ now raises a clear
RuntimeErrorpointing atfrom_pretrained(..., tp_plan=...)rather than silently corrupting weights. That matches what upstream itself now does. Genuinely restoring it means porting toapply_tensor_parallelism, which is a feature port and does not belong in a CI fix. Happy to be overruled if you would rather that land here.Also fixed the drift message wording: a
(*args, **kwargs)signature now reports that it accepts any call and forwards nothing, rather than "accepts only 0 positionals", which sent the reader looking for a signature change that never happened.Robustifying the detector, not just the loader
The detector was right here and still let a silent corruption run from 5.0.0 to 5.16.0, because it only ever asked "does this symbol exist with this signature" and never "is the symbol we patch the one actually on the live path".
mxfp4.dequantizeexisted with an unchanged signature the entire time. Every pin was green. The patch reached nothing.This PR adds
_referenced_global_namesand_assert_on_live_path, which statically walk a caller's code objects and fail when the symbol zoo patches has no live caller. Nested code is included viaco_consts, so a call made inside a comprehension, a nested def or a lambda is still seen, which a bareco_namescheck would miss. It is wired into the ConversionOps detector, and a 4.x equivalent assertsquantizer_mxfp4still calls the legacy hook.Verified non-vacuous rather than assumed:
The second line is the point: this mechanism can see the exact condition that made the old patch a no-op.
Known limitation, stated rather than implied. This is a static reference check, so it catches a symbol losing all its callers, which is the failure mode that actually bit us. It will not catch a caller that is present but unreachable at runtime (guarded by a config branch that is never taken, say), and it covers the mxfp4 detectors here rather than the whole suite. The 4.x check also skips rather than fails when
transformers.quantizerscannot import, since it pulls optional heavy backends like torchao and a broken one of those says nothing about drift.Unrelated
#1112 fixes the other
mainred (theDEVICE_TYPEcollection error in the security lane). It is independent of this one.