Fix callback tensor inputs that are never bound in the denoising loop - #14416
Conversation
LEditsPPPipelineStableDiffusion allows prompt_embeds, LEditsPPPipelineStableDiffusionXL allows negative_add_time_ids and StableDiffusionXLControlNetUnionInpaintPipeline allows masked_image_latents, but none of those names exist as locals in the respective __call__, so callback_kwargs[k] = locals()[k] raises KeyError as soon as a user requests them. Add a test_callback_inputs fast test to both LEdits++ test classes, which are plain unittest.TestCase and therefore never ran the PipelineTesterMixin callback tests.
|
Hi @LuShadowX, 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. |
|
Hi @LuShadowX, a follow-up on the reminder above: this PR still does not link an issue it fixes. Please note that PRs without a linked issue are likely to be automatically closed 10 days after this notice. Adding a closing keyword (e.g. |
| model_cpu_offload_seq = "text_encoder->unet->vae" | ||
| _exclude_from_cpu_offload = ["safety_checker"] | ||
| _callback_tensor_inputs = ["latents", "prompt_embeds", "negative_prompt_embeds"] | ||
| _callback_tensor_inputs = ["latents", "negative_prompt_embeds"] |
There was a problem hiding this comment.
nit: I think it might make sense to replace "prompt_embeds" here with "text_embeddings" in a follow-up PR, since my understanding is that text_embeddings plays an analogous role in this pipeline with a non-standard name.
|
Merging as the CI failures are unrelated. |
What does this PR do?
Three pipelines list names in
_callback_tensor_inputsthat are never bound in their__call__, socallback_kwargs[k] = locals()[k]raisesKeyErroras soon as a user requests one:LEditsPPPipelineStableDiffusionprompt_embedsLEditsPPPipelineStableDiffusionXLnegative_add_time_idsStableDiffusionXLControlNetUnionInpaintPipelinemasked_image_latentsmask, _ = self.prepare_mask_latents(...)discards itOn
main:Each allowlist now matches the actual denoising-loop locals. Both LEdits++ test classes are plain
unittest.TestCase, so thePipelineTesterMixincallback tests never ran against them — this addstest_callback_inputsto each.StableDiffusionXLControlNetUnionInpaintPipelinehas no test file, so it is covered by the fix alone; I found it with an AST sweep oversrc/diffusers/pipelinesfor the same defect, which now reports zero remaining instances.Addresses Issue 4 of #13635 (coordination comment: #13635 (comment)). The commented-out
check_inputs()call that issue also mentions is left alone.Tests
utils/check_repo.pyandutils/check_inits.pyfail the same way on a pristine checkout ofmainon my machine (missingdiffusers.models.auto, and a lookup forsrc/transformers), so they are unrelated to this diff.Self-review notes
.ai/AGENTS.md, don't guess intent and silently correct behavior.callback_outputs.pop(...)lines instead of leaving them next to the change, per the no-dead-code rule.callback_inputs_allcheck fromPipelineTesterMixin.test_callback_inputsrather than inventing a pattern, per.ai/testing.md.negative_prompt_embedsresolves to the__call__argument, which isNoneunless the caller passes it — the real uncond tensor isuncond_embeddingsin the SD pipeline. It doesn't crash, so I'd rather have your call on it than change it here.check_inputs()call. Re-enabling it changes validation behavior well beyond this bug.Before submitting
self-reviewskill on the diff?Who can review?
@yiyixuxu @asomoza