Conversation
…m_transformer `from_transformer` bound `config = transformer.config`, aliasing the transformer's live config, then wrote `num_layers` and `extra_conditioning_channels` into it. Building a ControlNet therefore silently changed the source transformer, which can corrupt later serialization, logging, or pipeline construction that reuses it. Copy the config first, as FluxControlNetModel and QwenImageControlNetModel already do. Add regression tests covering the config staying untouched, the ControlNet receiving the overrides, and `num_layers=None` falling back to the transformer's value. Ref huggingface#13611 (Issue 4) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XBYeq5vB4DNZDEaqUsroKR
|
Hi @Devam0311, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Once the PR links an issue (or gets the |
|
This PR has been automatically closed because it does not link an issue and the reminder above was not addressed within 10 days. If this PR is still relevant, please link the issue it fixes (e.g. We are experimenting with this process to keep the review queue manageable, and it will sometimes get it wrong. If you think this PR should stay open, please just say so here and we will reopen it — no need to justify it at length. Thanks again for contributing, and sorry for the noise if we closed this by mistake! |
What does this PR do?
Fixes Issue 4 of #13611 (
stable_diffusion_3model/pipeline review).SD3ControlNetModel.from_transformerboundconfig = transformer.config, which aliases the transformer's live config, and then wrote ControlNet-specific values into it. Building a ControlNet therefore mutated the transformer it was built from.Reproduced on
main:The source transformer's
num_layersis overwritten and an unrelatedextra_conditioning_channelskey is injected, which can corrupt later serialization, logging, or any pipeline construction that reuses that transformer.Solution
Copy the config before mutating it — which is what the sibling ControlNets already do:
controlnet_flux.py:135config = dict(transformer.config)controlnet_qwenimage.py:113config = dict(transformer.config)controlnet_sd3.py:257config = transformer.config← fixed hereThe
num_layersfallback now reads fromtransformer.configrather than the copied dict, since a plain dict has no attribute access.I also checked
controlnet_hunyuan.py:177, which binds the config the same way but only ever reads attributes from it and never writes, so it is not affected and is left alone.Testing
New
tests/models/controlnets/test_models_controlnet_sd3.py:test_from_transformer_does_not_mutate_source_config— the regression itselftest_from_transformer_applies_controlnet_config— the overrides still reach the ControlNettest_from_transformer_num_layers_falls_back_to_transformer— covers thenum_layers=Nonepath, which the rewritten fallback touchesVerified the regression test actually catches the bug: with the fix reverted it fails, with it applied it passes.
Note:
tests/models/controlnets/test_models_controlnet_cosmos.pyhas 7 failures, but they reproduce identically on a clean checkout ofmainhere, so they are unrelated to this change.Before submitting
self-reviewskill on the diff?Self-review notes
Reviewed against
.ai/references/review-rules.md,models.mdandtesting.md. No blocking issues.For the reviewer:
tests/models/controlnets/previously held only the Cosmos file, and there were nofrom_transformertests anywhere in the suite. I added a small focused file rather than wiring up the fullBaseModelTesterConfigmixin suite, which felt like scope creep for a one-line fix — happy to expand it if you'd rather it be a full model tester.Who can review?
@yiyixuxu @DN6 @hlky