IPAdapterTesterMixin

[a-r-r-o-w]

What does this PR do?

Adds IPAdapterTesterMixin to ensure pipelines supporting it are not broken with newer changes.

Fixes #6829.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline?
• Did you read our philosophy doc (important for complex PRs)?
• Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

@yiyixuxu @sayakpaul

[HuggingFaceDocBuilderDev]

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.

[yiyixuxu]

thanks so much for working on this!!! 😇😇😇
we are discussing about allowing pass image embedding as ip_adapter_image here #6830 I think it will make the testing a lot easier too, no? that way we don't need to make an image_encoder for all the pipelines. Let me know what you think!

[a-r-r-o-w]

Yes that makes sense since ip_adapter_image is annotated as PipelineImageInput, which can be a PIL.Image, np.ndarray or torch.Tensor, and should've maybe been the default behaviour to accept anything with appropriate validation from the start. Once a PR supporting embeddings is merged, I'll update the relevant testing code here. Also looks like all tests passed. Do you want me to add the IPAdapterTesterMixin to all the pipeline tests where relevant (like SD/SDXL I2I, Inpaint)? I think it might be too redundant and makes sense to have only for T2I since most, if not all, other implementations use the # Copied from logic.

Btw, am I right in assuming that IP-Adapter-Plus tests should also be added here?

[yiyixuxu]

hey thanks! looks really good!
let's wait a little bit for #6868 to merge and we can finish it up!

[yiyixuxu]

Do you want me to add the IPAdapterTesterMixin to all the pipeline tests where relevant (like SD/SDXL I2I, Inpaint)? I think it might be too redundant and makes sense to have only for T2I since most, if not all, other implementations use the # Copied from logic.

I think we should add to all pipeline that support ip-adapter; but we should only use IP adapter image embedding directly, so the code addition to the fast tests is actually minimum; I think you just have to append the mixin to the test that's all :)

also if we do it that way, we don't test the part of code that's redundant ( #Copied from)

[yiyixuxu]

love this! thank you!

[yiyixuxu]

look, it already caught a bunch of test failures!!! 😅
I think most of them have the same type of error so probably easy to fix.

Not all of the pipelines need to have ip-adapter support IMO. So if the pipeline isn't very much used, e.g. stable diffusion safe, and the test failure is because the ip-adapter isn't correctly implemented. Let me know! we can probably remove it

[a-r-r-o-w]

Thanks for the Good PR label 😆 Just adding the IPAdapterTesterMixin to the class defintion was quite convenient! Also quite cool to see the error logs 👀 Will fix in a bit

I agree with removing the tests from the not-so-used pipelines that are not regularly discussed about in issues. I will remove them from what I think are lesser used, but since you have access to make changes to my branch, please feel free to change it to your liking.

[a-r-r-o-w]

Pipelines that have the error that looks something like:

FAILED tests/pipelines/controlnet/test_controlnet_img2img.py::ControlNetImg2ImgPipelineFastTests::test_ip_adapter - TypeError: argument of type 'NoneType' is not iterable
FAILED tests/pipelines/stable_diffusion/test_stable_diffusion_img2img.py::StableDiffusionImg2ImgPipelineFastTests::test_ip_adapter - TypeError: argument of type 'NoneType' is not iterable

is because the IP Adapter implementation is incorrect not the latest:

added_cond_kwargs = {"image_embeds": image_embeds} if ip_adapter_image is not None else None

The check should include both ip_adapter_image and ip_adapter_image_embeds as done in some pipelines of #6868 but looks like a few spots were missed. Looks like this mixin will help make things quite consistent!

[yiyixuxu]

thank you!

[a-r-r-o-w]

FAILED tests/pipelines/controlnet/test_controlnet.py::ControlNetPipelineFastTests::test_ip_adapter - AssertionError: 0.007904649 not greater than 0.01 : Output with multi-ip-adapter scale must be different from normal inference
FAILED tests/pipelines/controlnet/test_controlnet_inpaint.py::ControlNetSimpleInpaintPipelineFastTests::test_ip_adapter - AssertionError: 0.0 not greater than 0.01 : Output with ip-adapter must be different from normal inference
FAILED tests/pipelines/controlnet/test_controlnet_img2img.py::ControlNetImg2ImgPipelineFastTests::test_ip_adapter - AssertionError: 8.6426735e-07 not greater than 0.01 : Output with ip-adapter must be different from normal inference
FAILED tests/pipelines/stable_diffusion_panorama/test_stable_diffusion_panorama.py::StableDiffusionPanoramaPipelineFastTests::test_ip_adapter - AssertionError: 0.0014307499 not greater than 0.01 : Output with ip-adapter must be different from normal inference

@yiyixuxu I'm not sure why these tests fail. I think the results are slightly different based on device because they do pass on my system with a difference greater than 0.01. Is lowering the threshold the ideal solution or what do you have in mind? Maybe a threshold with the differences accumulated could also be used. Regarding ControlNetSimpleInpaintPipelineFastTests, I'm not sure why the outputs are the same.

[yiyixuxu]

Maybe increase this value to 100 lol

[yiyixuxu]

@a-r-r-o-w
for the failing test, maybe trying to increase the scale, see https://github.com/huggingface/diffusers/pull/6862/files#r1491365749

[a-r-r-o-w]

Thanks @yiyixuxu. I believe all tests should pass now and that this is ready for a merge.

[yiyixuxu]

Hi @a-r-r-o-w
there is still one test fail for panorama pipeline
can you look into it? I'm ok with removing ip-adapter support from it if it's not easy to get it to work:)

[DN6]

Just a question. Why increase this based on the whether image is provided? Because of strength?

[a-r-r-o-w]

That is correct. The tests fail for img2img tests that have num_inference_steps set to 2 or lower and strength greater than 5. I think if it was:

if "image" in inputs().keys() and "strength" in inputs().keys():
    ...

it would make more sense.

[DN6]

Can we break this up into individually named tests? It is hard to debug when an intermediate assert causes the entire test to fail.

[a-r-r-o-w]

I've split it into two tests for the time being but note that if we have K classes that derive from this mixin, we now have K additional inferences running as compared to before. The time to execute all tests doesn't increase too much though.

[a-r-r-o-w]

Hi @a-r-r-o-w there is still one test fail for panorama pipeline can you look into it? I'm ok with removing ip-adapter support from it if it's not easy to get it to work:)

Yeah, I think I've been having troubles on getting it to work. I'm not sure why the results vary :/

[a-r-r-o-w]

@yiyixuxu Apologies for the delay. With the last commit, all tests pass for me locally and I hope this is completed 🥲

[yiyixuxu]

thank you:)