Repository navigation
Conversation
`StableDiffusionXLInpaintVaeEncoderStep._encode_vae_image` read `self.vae.config.scaling_factor`, but `self` is the block, not the components object. Any inpaint modular pipeline whose VAE config defines `latents_mean` and `latents_std` therefore failed with `AttributeError: 'StableDiffusionXLInpaintVaeEncoderStep' object has no attribute 'vae'`. Use `components.vae`, as the surrounding lines and the equivalent non-inpaint encoder already do. Add a regression test driving the method with a stand-in VAE, since the branch only runs for a config carrying `latents_mean` / `latents_std`. Ref huggingface#13610 (Issue 5) 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 |
adi-IL
left a comment
There was a problem hiding this comment.
Tested locally on Linux x86_64 with python 3.14 and torch 2.13.
Confirmed that StableDiffusionXLInpaintVaeEncoderStep._encode_vae_image raised AttributeError: 'StableDiffusionXLInpaintVaeEncoderStep' object has no attribute 'vae' on main whenever latents_mean and latents_std were present, because vae lives on components, not self.
With this fix to components.vae.config.scaling_factor, the normalization branch runs cleanly and matches the fallback in the else branch. The new unit test passes.
|
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 5 of #13610 (
stable_diffusion_xlmodel/pipeline review).StableDiffusionXLInpaintVaeEncoderStep._encode_vae_imagereadsself.vae.config.scaling_factor, butselfhere is the block, not the components object:Reproduced on
main:So any inpaint modular pipeline whose VAE config defines
latents_meanandlatents_stdfails outright. The equivalent non-inpaint encoder in the same file already usescomponents.vae.Solution
One-word fix —
self.vae→components.vae— matching theelsebranch directly below it and the non-inpaint encoder above it.Testing
TestSDXLInpaintVaeEncoderStep::test_encode_vae_image_with_latents_mean_and_stddrives the method with a stand-in VAE, as the issue's reproduction does. That is deliberate rather than lazy: the branch only executes for a VAE config carryinglatents_mean/latents_std, which the SDXL VAE used elsewhere in that test file does not define, so the existing fixtures cannot reach it.With
latents_mean=0andlatents_std=1the branch reduces to a plainscaling_factormultiply, so the test asserts the exact expected values rather than just the absence of an exception.Verified it catches the bug: with the fix reverted it fails with the
AttributeErrorabove.Before submitting
self-reviewskill on the diff?Self-review notes
Reviewed against
.ai/references/review-rules.md,modular.mdandtesting.md. No blocking issues.For the reviewer:
latents_mean/latents_stdVAE just to reach one line seemed disproportionate. Happy to fold it into the existing tester config instead if you'd prefer consistency.Who can review?
@yiyixuxu @asomoza @DN6