Fix convert_model_to_fp8_ao converting the first and last linear layers - #4147
Open
vineethsaivs wants to merge 1 commit into
Open
Fix convert_model_to_fp8_ao converting the first and last linear layers#4147vineethsaivs wants to merge 1 commit into
vineethsaivs wants to merge 1 commit into
Conversation
convert_model_to_fp8_ao documents that it converts every nn.Linear
"except the first and last", and find_first_last_linear_layers exists
because quantizing those two destabilises training. Its default
module_filter_func was filter_first_and_last_linear_layers, which calls
find_first_last_linear_layers on the module it is handed. torchao's
swap_linear_layers hands the filter one candidate layer at a time, so
that lookup runs on a single nn.Linear, returns ("", ""), and matches no
real FQN, so nothing is ever filtered.
On a three-linear model, with real torchao:
default {'embed_proj': True, 'block.0': True, 'lm_head': True}
module_filter_func=None {'embed_proj': False, 'block.0': True, 'lm_head': False}
The second line is the branch that is already correct: when
module_filter_func is None, the function binds the model's real first and
last layer names into filter_linear_layers. Default to None so that branch
runs, which also makes the signature agree with the docstring, which has
said "Defaults to filter_linear_layers" since this was added.
Accelerator is unaffected: AORecipeKwargs.module_filter_func defaults to
None and is passed straight through, so the correct path was already
taken there. This fixes the direct caller, which is what the function's
own example shows.
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.
What does this PR do?
convert_model_to_fp8_aodocuments that it converts everynn.Linear"except the first and last", andfind_first_last_linear_layersexists precisely because quantizing those two destabilises training:It does not skip them. On a three-linear model, with real
torchao0.17.0 on CPU:Truemeans the layer was swapped forFloat8Linear. The first line is what a direct caller ofconvert_model_to_fp8_ao(model)gets today, which is the call its own docstring example shows.Root cause
The default
module_filter_funcisfilter_first_and_last_linear_layers, which does:modulethere is not the model. torchao'sswap_linear_layerscallsmodule_filter_fn(module, cur_fqn)once per candidate layer, sofind_first_last_linear_layersruns on a singlenn.Linear, whosenamed_modules()yields only("", itself). It therefore returns("", ""), andfqn in ["", ""]is false for every real FQN, so the filter approves everything:The fix
The correct implementation is already in the function, one line below, but unreachable by default:
So the change is to default
module_filter_functoNone. That also makes the signature agree with its own docstring, which has saiddefaults to filter_linear_layerssince #3348, and it restores the branch #3450 was fixing ("I didn't actually update the call in the case of the default being used"), which has been dead for a direct caller since the signature default was set.Blast radius, stated rather than implied
Acceleratoris not affected.AORecipeKwargs.module_filter_funcdefaults toNoneand is passed straight through toconvert_model_to_fp8_ao, and an explicitNoneargument beats the signature default, so the FP8 path throughAcceleratorwas already taking the correct branch. What this fixes is the direct call, which is exported fromaccelerate.utilsand is what the docstring example demonstrates.One thing I did not change, and would like your call on
After this change nothing references
filter_first_and_last_linear_layers, and I do not think it can be made to work with its current signature: torchao's contract is(module, fqn) -> booland the first and last linear cannot be derived from one candidate layer. The options I see are to turn it into a factory that takes the model and returns a bound filter, or to drop it. Both change or remove a symbol exported fromaccelerate.utils, so I left it alone rather than decide that in a bug-fix PR. Happy to do either here or in a follow-up, whichever you prefer.Testing
Added
test_convert_model_to_fp8_ao_skips_the_first_and_last_linear_layerstotests/test_utils.py, guarded with@require_torchao. It builds a three-linear toy model, callsconvert_model_to_fp8_ao(model)with no filter argument, and asserts the first and last staynn.Linearwhile the middle one becomesFloat8Linear. It is CPU-only; the layer swap needs no GPU, which is why it can live intests/test_utils.pyrather than the launcher-basedtests/test_fp8.py.Three runs against
accelerate1.14.0, whosesrc/accelerate/utils/ao.pyis byte-identical tomainat16cb6eb, withtorch2.13.0 andtorchao0.17.0:ruff checkandruff format --checkreport the same result on the changed files as on the unmodified ones (two pre-existing preview-rule findings inao.py,collapsible-ifandneedless-bool, both present before this change and untouched by it), so the lint result is a real comparison rather than a config that checks nothing.Before submitting
On the documentation box: no docs change was needed, because the docstring already describes the fixed behaviour; it was the signature that disagreed with it.
Who can review?
@SunMarc @BenjaminBossan