Skip non-quantizable modules in nn.quantize with a custom predicate#3867
Skip non-quantizable modules in nn.quantize with a custom predicate#3867shubhxho wants to merge 1 commit into
Conversation
c896947 to
ed42e2d
Compare
Pablosinyores
left a comment
There was a problem hiding this comment.
Reproduced the reported failure on current main. Because quantize walks leaf_modules(), any leaf that does not define to_quantized reaches the predicate, so a broad predicate hits the raise on ordinary layers:
class Block(nn.Module):
def __init__(self):
super().__init__()
self.proj = nn.Linear(64, 64)
self.norm = nn.LayerNorm(64)
nn.quantize(Block(), group_size=64, bits=4, class_predicate=lambda p, m: True)
# ValueError: Unable to quantize model of type <class 'mlx.nn.layers.normalization.LayerNorm'>leaf_modules() returns ['proj', 'norm'], so this is not an exotic case, any model with a norm or activation layer trips it. Skipping those rather than raising looks like the right direction to me, and doing the hasattr check before calling the predicate reads cleaner than the nested version it replaces. black is clean on both files.
One thing I would want a maintainer to weigh explicitly: the change removes the Unable to quantize model of type ... error entirely, so a predicate that deliberately selects a type that cannot be quantized now silently does nothing. class_predicate=lambda p, m: isinstance(m, nn.LayerNorm) used to say why nothing happened; after this it returns a model that is simply unquantized, with no signal.
Skipping incidental leaves and staying silent when the user asked for something impossible are two different things, and this collapses them into one behavior. If you want to keep a signal without bringing back the old over-eager raise, one option is to raise only when the predicate returned true for at least one module and nothing was quantized at all. Happy either way, but it seems worth deciding rather than dropping the diagnostic as a side effect.
Also worth updating the docstring line about the default: with class_predicate now defaulting to lambda _, m: True, the "all layers that define a to_quantized() method are quantized" sentence is still accurate, but it now describes the hasattr guard rather than the default predicate. The added sentence covers it, just noting the two are doing different jobs now.
|
Thank you for flagging this! Will look at it today |
3436004 to
5d04604
Compare
A class_predicate that returned truthy for a module without a to_quantized() method raised "Unable to quantize model of type ..." and aborted the whole call, so a broad predicate (e.g. lambda p, m: True) tripped on any incidental norm or activation leaf. Check to_quantized() before quantizing and skip such leaves so a predicate can be applied to a whole model. Keep a diagnostic for the case the old raise was useful for: if a predicate selects only modules that cannot be quantized and nothing ends up quantized, raise instead of silently returning an unchanged model. The default predicate still selects only quantizable leaves, so it stays lenient and never trips it. Also copy a params dict returned by the predicate before dropping a falsy quantize_input, so a predicate returning a shared dict across modules is not mutated. Tests cover: broad True/dict predicates skipping incidental leaves, a mixed predicate quantizing what it can, a predicate selecting only unquantizable modules raising and naming the type, an invalid return type, an empty selection being a no-op, the shared-dict not being mutated, quantize_input passed through a dict, and the default predicate staying lenient.
5d04604 to
775f3a4
Compare
|
Thanks for looking into this and for the detailed issue. That said, I think the current behaviour is intended:
In other words, If you want to quantize a custom module, the intended way is to define a
Given that, I don't think we should change this behaviour, so I'll close the PR. |
Fixes #3865.
With the default predicate,
nn.quantizeskips modules that don't defineto_quantized(). Passing a customclass_predicateloses that behavior: if it returns truthy for such a module, the call raisesUnable to quantize model of type ...and the whole model load fails.This is why call sites like
mlx_lmstart every predicate withif not hasattr(m, "to_quantized"): return False.The fix checks for
to_quantized()before calling the predicate, so a module without it is skipped whether the predicate is custom or the default. Standard layers all defineto_quantized(), so existing calls are unaffected. Updated the docstring and added test coverage for a custom predicate returningTrueand a params dict over a non-quantizable module.