-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Official callback tests #7930
base: main
Are you sure you want to change the base?
Official callback tests #7930
Conversation
I used the normal callback test as a guide, are this tests enough or should I add more? Also for the 3 official callbacks we have right now, should I add test cases for the specific pipelines? What I mean is if we're going to test all the official callbacks implementations? |
@@ -1808,6 +1809,77 @@ def callback_increase_guidance(pipe, i, t, callback_kwargs): | |||
# accounts for models that modify the number of inference steps based on strength | |||
assert pipe.guidance_scale == (inputs["guidance_scale"] + pipe.num_timesteps) | |||
|
|||
def test_official_callback(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just test the officially supported callback here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can I know here if the pipeline being tested is SD 1.5 or SDXL? Maybe I can check the pipeline class, but still, IMO is a bit inefficient since the official callbacks right now works for a limited number of pipelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay. So, IIUC, you're proposing not to add tests for the official callbacks then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, it's more like asking for guidance on how to do it following the rest of the code base, so maybe I'll split the questions:
- Are we're going to make tests for all the official callbacks (I think there's going to be at least 10), some for IP Adapters, SD, SDXL and probably Pixart and SD3. So it's not that I was proposing not to add tests, just adding more context to make the decision.
- For example, if I want to test the SDXL one, I didn't see any other test that checked the pipeline class. So what would be the best option for diffusers here? create only one test in
StableDiffusionXLPipelineFastTests
?, on all of them, create a mixin or just write it in the common one? This will be relevant also for the SD 1.5 but not for IP Adapters since they already have a mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing the additional context. I think the following could work but please let me know your thoughts too:
Create something like
class IPAdapterTesterMixin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I would have done so I like it, I'll do the other tests then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree here; we can make a mixin and add to relevant pipeline tests. I think we should at least test SD and SDXL-based pipelines, it will be nice to have more comprehensive coverage to include Kandinsky, pixart, cascade
we also need to figure out a way to find out which callback works with which pipelines
Here in my from_pipe tester mixin, I use this simple logic to figure out which ones are SD-based and which ones are SDXL based, https://github.com/huggingface/diffusers/blob/e0e8c58f64c024e89d125ddb563d0de4cf252dc1/tests/pipelines/test_pipelines_common.py#L626C9-L626C32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Maybe we just test the officially supported callbacks?
I'll do the kandinsky special test at the end which is the one failing. |
While doing the tests I'm getting an error if I do the test with The error happens in this line: diffusers/src/diffusers/pipelines/stable_diffusion/pipeline_stable_diffusion_inpaint.py Line 1380 in 1221b28
Since the I think we have this options:
@sayakpaul @yiyixuxu WDYT? Is there an alternative that I'm missing? |
I think both the pipelines are important. So, I won't prefer skipping them. Having separate callbacks (Inpainting, Pix2Pix, etc.) seems like the best and cleanest way here. @yiyixuxu WDYT? |
@sayakpaul @asomoza |
ohhhh another way is just handle this in the callbacks. I think you can actually combine the SD and SDXL callbacks with |
yeah I think try to make the callback work for all pipeline is better, this way it's easier for tests too |
yeah, I'll try to do them all in this PR. |
Now that I analyze this, for the For this to work I would need to bypass that check and allow the use of any name in |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one comment - looks good to me overall!
@DN6 can you do a final review? |
Sorry I missed this. Left a comment on a spelling mistake and I believe @yiyixuxu's comment still needs to be addressed. Good to merge once conflicts are resolved and tests pass. |
I changed the Don't merge yet. |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
What does this PR do?
Tests for the official callbacks introduced in #7761
Who can review?
@sayakpaul @yiyixuxu