Fix UnboundLocalError in get_parameter_dtype from a shadowed tuple builtin - #14579
Open
Om-singhaI wants to merge 1 commit into
Open
Fix UnboundLocalError in get_parameter_dtype from a shadowed tuple builtin#14579Om-singhaI wants to merge 1 commit into
Om-singhaI wants to merge 1 commit into
Conversation
`get_parameter_dtype` loops with `for tuple in gen:`, which makes `tuple` a function local for the whole scope. The nested `find_tensor_attributes` declares `list[tuple[str, Tensor]]` as its return annotation, and since this module has no `from __future__ import annotations`, that annotation is evaluated when the `def` statement runs. `tuple` is then read as an unbound fast local, so the call raises `UnboundLocalError` before the loop ever binds it. The failing path is the fallback for modules whose tensors live in `__dict__` rather than in `named_parameters()` or `buffers()`, which is exactly what `torch.nn.parallel.replicate` builds for each device. `ModelMixin.dtype` routes here, so any model that reads `self.dtype` in `forward` raises under `torch.nn.DataParallel`. Renaming the loop variable removes the shadow, so the annotation resolves `tuple` as the builtin again. `get_parameter_device` is unaffected because it already binds `first_tuple`. Fixes huggingface#13789
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?
Fixes #13789
get_parameter_dtypeinsrc/diffusers/models/modeling_utils.pyraisesUnboundLocalErroron everycall that reaches its
nn.DataParallelfallback.ModelMixin.dtypeis a thin wrapper around thatfunction, so a model that reads
self.dtypeinsideforwarddies undertorch.nn.DataParallel.UNet2DModeldoes exactly that atsrc/diffusers/models/unets/unet_2d.py:292, which is the path inthe issue report:
The failure has nothing to do with multiple GPUs. It only needs a module whose tensors sit in
__dict__instead of in_parametersor_buffers, which is precisely whattorch.nn.parallel.replicateproduces: it empties_parameterson each replica, then re assigns thebroadcast copies with
setattr. Because those copies are plain tensors rather thannn.Parameter,they land in
replica.__dict__,named_parameters()andbuffers()both come back empty, andget_parameter_dtypefalls through to its last branch.Reproduction
Single process, CPU only, no accelerator needed. On
mainat58eb52c08:Note where the traceback points: line 205, the
defstatement, not the loop below it. With this PRthe same script prints
replica dtype : torch.float16.Root cause
for tuple in gen:bindstuple, so the compiler treatstupleas a function local for the entirescope of
get_parameter_dtype, including the lines above the loop.modeling_utils.pyhas nofrom __future__ import annotations, so the return annotation on the nesteddefis a normalexpression evaluated when the
defstatement executes. Evaluatinglist[tuple[str, Tensor]]readstuple, which at that moment is an unbound fast local, and Python raises before the loop can everbind it.
The compiled code object shows it directly:
list,strandTensorare global lookups inco_names, as expected.tupleis not: it sits inco_varnameswith the locals.get_parameter_devicehas the same nesteddefwith the same annotation but bindsfirst_tuplerather than
tuple, so it never shadows the builtin and is not affected. Itsco_varnamesare('parameter', '_get_group_onload_device', 'parameters_and_buffers', 'find_tensor_attributes', 'gen', 'first_tuple').When it regressed
git blameon the two lines splits them across two commits:The
for tuple in gen:loop arrived in7c2f0afb1c("updateget_parameter_dtype", #10342) and washarmless for over a year, because the annotation then read
List[Tuple[str, Tensor]]andTuplewasa module global that nothing shadowed.
2843b3d37a("Sunset Python 3.8 & get rid of explicittypingexports where possible", #12524)lowercased it:
That is the moment the annotation started colliding with the loop variable.
get_parameter_devicegot the identical edit in the same commit and stayed correct only because it never binds the name the
annotation reads: it takes
first_tuple = next(gen)rather than looping overtuple.Affected Python versions
Measured on the standalone pattern, so the interpreter is the only variable:
UnboundLocalError.tupleis inco_varnames,co_cellvarsis empty.UnboundLocalError. Same code object shape.tupleis inco_cellvarsand the function has__annotate__.3.14 hides the bug. Under PEP 649 the annotation is compiled into a separate
__annotate__codeobject that is not evaluated when the
defstatement runs,tuplebecomes a cell variable ratherthan a fast local, and the call completes. Anyone checking this on 3.14 will not reproduce it.
That matters because the affected range is the supported range:
setup.pydeclarespython_requires=">=3.10.0"..github/workflows/pr_tests.ymlpinspython-version: "3.10"for the PR test jobs.So every supported interpreter from 3.10 through 3.13 hits this, CI itself runs on one of them, and
the reporter in #13789 is on 3.11.13.
The fix
Rename the loop variable. Nothing else changes, and the annotation resolves
tupleas the builtinagain.
gen = parameter._named_members(get_members_fn=find_tensor_attributes) last_tuple = None - for tuple in gen: - last_tuple = tuple - if tuple[1].is_floating_point(): - return tuple[1].dtype + for t in gen: + last_tuple = t + if t[1].is_floating_point(): + return t[1].dtypelast_tupleis left alone because it shadows nothing, andget_parameter_deviceis left alonebecause it was never broken.
After the patch,
co_varnamesno longer containstuple, andtuplejoinslist,strandTensorinco_namesas a global lookup.Test
Two tests in
TestModelUtilsintests/models/test_modeling_common.py, both CPU only and offline.The first puts a module into the same state
torch.nn.parallel.replicateleaves a replica in andasserts the dtype comes back, parametrized over
float16,bfloat16andfloat32; the secondcovers the other exit from the same branch, where no tensor is floating point and the last one found
is returned.
That replica state is rebuilt by hand rather than obtained from
torch.nn.parallel.replicateitself, because
replicatebroadcasts through CUDA and the CPU test job has no device to broadcastto. See non blocking item 3 in the self review notes below, and say the word if you would rather
have a real
torch.nn.DataParalleltest gated on@require_torch_multi_gpualongside this one.With the source change reverted and the tests kept:
With the source change in place, the whole module is green:
ruff checkandruff format --check(0.9.10, the version pinned insetup.py) pass on bothchanged files and across
examples scripts src tests utils setup.py.Self review notes
Checked the diff against
.ai/references/review-rules.md, pluscode_style.md,models.md,testing.mdandpitfalls.md. Small thing for maintainers:SKILL.mdpoints atreferences/...relative to itself, but the skill directory has noreferences/subdirectory, so those files actually live at.ai/references/.Blocking issues
None.
Non blocking issues
Typo in a new test comment.
The comment reads "re assigns" where the word is
reassigns.tests/models/test_modeling_common.py:284Impact: cosmetic only. The comment is otherwise accurate and stands alone for a future reader, which is what
references/review-rules.mdasks of comments under "Ephemeral context".No rule citation; this is a plain typo.
Single letter loop variable.
tis the smallest edit that removes the shadow and it is correct. The repo does state a preference for spelled out names, and the sibling function bindsfirst_tupleatmodeling_utils.py:161, socurrent_tupleorname_and_tensorwould read more consistently with the file. Every one of these choices fixes the bug equally.src/diffusers/models/modeling_utils.py:211Impact: readability only, no behavior difference.
Per
references/models.md: "Prefer descriptive variable names over short ones. For example, prefer spelling outqueryoverq."The replica in the test is hand built rather than produced by
torch.nn.parallel.replicate.The helper calls the real private
Module._replicate_for_data_parallel()and then performs thesetattrhalf oftorch.nn.parallel.replicateitself, readingmodule._parametersdirectly. Both are private torch APIs. If torch ever changes howreplicateassigns broadcast copies, this test stays green while realnn.DataParallelbreaks. The reason for hand building it is legitimate:torch.nn.parallel.replicatebroadcasts through CUDA, so it cannot run in the CPU test job.tests/models/test_modeling_common.py:283Impact: the test pins the shape of the bug rather than the real
nn.DataParallelpath.Per
references/testing.md: "don't monkeypatch a component method (e.g. the scheduler'sset_timesteps) just to capture what the code under test passed to it — that only verifies the caller against itself, not against the real method's contract. Call the real component and assert on its resulting state."Documentation impact: none.
get_parameter_dtypeis not exported fromsrc/diffusers/__init__.pyorsrc/diffusers/models/__init__.py, no public signature, argument or default changed, and behavior for every module that has parameters or buffers is unchanged. No page underdocs/describes this fallback. Flagged explicitly becausereferences/review-rules.mdasks that docs be scanned rather than assumed.Suggestion for a maintainer: write this gotcha down.
The ruff selectors in
pyproject.tomlare["C", "E", "F", "I", "W"]withF402in the ignore list, and flake8-builtins (A001) is not enabled, so no configured linter can catch a builtin shadowed by a loop variable. The failure only surfaces here because a nested annotation is evaluated at runtime in the same scope, which is easy to miss on review. A line in.ai/references/code_style.mdwould give the next contributor this for free. Raised here rather than changed in the diff, because the contributor guide says.ai/is maintained by core maintainers and contributors should flag rather than edit.Per
references/review-rules.md: "If the review turns up a rule, pattern, or common gotcha that isn't written down yet — especially one the author got wrong or that you had to reason out — propose adding it to the relevant agent guide".Dead code (advisory)
src/diffusers/models/modeling_utils.py:211(the renamed loop)_parameters, no buffers, and tensors in__dict__.ModelMixin.dtype(modeling_utils.py:1963) routes here, and shipped models reach it from insideforwardon plain submodules:transformer_minimax_h3.py:129,152,628,629,630,642,658,transformer_skyreels_v2.py:361,transformer_motif_video.py:437,autoencoder_kl_minimax_h3.py:862,887,autoencoder_kl_minimax_h3_audio.py:611,644,autoencoder_cosmos3_audio.py:594. Undernn.DataParalleleach of those submodules is a replica, so this is a live forward path, not just a.dtypeproperty read.src/diffusers/models/modeling_utils.py:216to:218(thelast_tuplefallback)__dict__holds only non floating point tensors. Covered by the new test attests/models/test_modeling_common.py:302.tests/models/test_modeling_common.py:283_as_data_parallel_replicatests/models/test_modeling_common.py:296.tests/models/test_modeling_common.py:293and:302TestModelUtils, which is a plain class and not aunittest.TestCase, so the@pytest.mark.parametrizeat line 292 applies as intended.src/diffusers/models/modeling_utils.py:216, the path wherelast_tuple is NoneNone. Outside this diff; see "Leave for the actual review" below.Summary
Verdict: READY.
The diff does one thing and does it correctly. The stated root cause holds up under isolation: the same shape raises
UnboundLocalErrorwithfor tuple in gen:and returns the expected dtype after the rename, and the two new tests exercise both exits of the repaired branch. Nothing added is defensive, unused or out of scope, no# Copied fromblock is touched somake fix-copieshas nothing to propagate, and all added lines sit inside the configured ruff line length.Fix before submitting
pytest tests/models/test_modeling_common.py -k get_parameter_dtypein a diffusers environment before opening the PR. Both branches were checked by reproducing them standalone against torch 2.13, but the repo suite itself was not executed during this review.tests/models/test_modeling_common.py:284, since it costs nothing.Leave for the actual review
tis correct and minimal, and a maintainer may simply prefer a spelled out name. Not worth guessing at.nn.DataParalleltest gated on@require_torch_multi_gpushould accompany the CPU one, given that the CPU test necessarily reconstructs the replica state by hand..ai/references/code_style.mdaddition, which only a maintainer can make.get_parameter_dtypefalls off the end of the function and returnsNone(src/diffusers/models/modeling_utils.py:216), soModelMixin.dtypewould hand backNonerather than thetorch.dtypeits annotation promises. That predates this branch, and fixing it would turn a four line bug fix into a behavior change. Happy to open a separate issue if you would like it addressed.Before submitting
self-reviewskill on the diff?documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@sayakpaul @DN6