Fix callback tensor inputs that are never bound in the denoising loop - #14416
Open
LuShadowX wants to merge 1 commit into
Open
Fix callback tensor inputs that are never bound in the denoising loop#14416LuShadowX wants to merge 1 commit into
LuShadowX wants to merge 1 commit into
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.
Contributor
|
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. |
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?
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