Skip to content

Conversation

@sayakpaul
Copy link
Member

@sayakpaul sayakpaul commented Jan 3, 2025

What does this PR do?

encode_prompt() is often used in isolation to lift off memory requirements and to also compute prompt embeddings in many trainers. It's important that we test encode_prompt() properly.

This PR adds a test suite to do that. Since we cannot reliably map the outputs of encode_prompt() into keyword arguments of a pipeline call, I propose to use the ast module to determine the names of the variables returned from a particular encode_prompt() method. This PR is a PoC of what the changes might look like at the test level.

The closest test we have currently is this:

class SDXLOptionalComponentsTesterMixin:

But, IMO, this should be extended to the other pipelines on a more general level.

TODOs

  • Propagate to the rest of the pipelines once we agree on the structure.
  • Fix encode_prompt() to work in isolation in the pipelines if needed.
  • Remove SDXLOptionalComponentsTesterMixin if needed.

@sayakpaul sayakpaul requested review from DN6 and hlky January 3, 2025 06:31
@sayakpaul
Copy link
Member Author

@hlky now that the PRs have been merged to support this, let's brainstorm if this is how we want to test encode_prompt()?

@sayakpaul
Copy link
Member Author

@DN6 a gentle ping.

@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.

@sayakpaul sayakpaul marked this pull request as ready for review February 17, 2025 06:15
@sayakpaul sayakpaul changed the title [WIP] [tests] test encode_prompt() in isolation [tests] test encode_prompt() in isolation Feb 17, 2025
@sayakpaul
Copy link
Member Author

@DN6 @hlky would now appreciate a set of reviews on the PR.

Couple of questions:

  • Majority of the test_save_load_optional_components() tests only do encode_prompt() in its testing. So, with this PR, we can safely remove them (at least ones that just test for encode_prompt(). Otherwise, there will be unnecessary duplications. LMK what you think.
  • I think SDXLOptionalComponentsTesterMixin can also be safely removed.

Copy link
Contributor

@hlky hlky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I agree SDXLOptionalComponentsTesterMixin and majority of test_save_load_optional_components can be removed, they would mainly be duplicates and with other recent changes we're working towards all components of a pipeline being usable in isolation which changes the context of these tests.

@sayakpaul
Copy link
Member Author

@DN6 WDYT?

@sayakpaul
Copy link
Member Author

Some further updates regarding removing some duplicate tests:

  • I have removed the SDXLOptionalComponentsTesterMixin class completely and any functional testing that was basically the same as what we're doing in this PR.
  • Removed any tests that incorporated encode_prompt() to check for what we're doing in this PR.

Also, I have fixed some tests that simply has pass to skip them by marking them with unittest.skip(). I know this is not related to this PR but they seemed minor enough to be clubbed.

Could you take a look again @hlky? Sorry for the reiteration.

@hlky
Copy link
Contributor

hlky commented Feb 20, 2025

Some tests are failing @sayakpaul

Details

FAILED tests/pipelines/controlnet/test_controlnet_sdxl.py::StableDiffusionXLControlNetPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/controlnet/test_controlnet_sdxl.py::StableDiffusionXLMultiControlNetPipelineFastTests::test_save_load_optional_components - AttributeError: 'StableDiffusionXLMultiControlNetPipelineFastTests' object has no attribute '_test_save_load_optional_components'. Did you mean: 'test_save_load_optional_components'?
FAILED tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl_adapter.py::StableDiffusionXLAdapterPipelineFastTests::test_save_load_optional_components - AttributeError: 'StableDiffusionXLAdapterPipelineFastTests' object has no attribute '_test_save_load_optional_components'. Did you mean: 'test_save_load_optional_components'?
FAILED tests/pipelines/controlnet/test_controlnet_sdxl.py::StableDiffusionXLMultiControlNetOneModelPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl_img2img.py::StableDiffusionXLImg2ImgRefinerOnlyPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl.py::StableDiffusionXLPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl_adapter.py::StableDiffusionXLMultiAdapterPipelineFastTests::test_save_load_optional_components - AttributeError: 'StableDiffusionXLMultiAdapterPipelineFastTests' object has no attribute '_test_save_load_optional_components'. Did you mean: 'test_save_load_optional_components'?
FAILED tests/pipelines/controlnet/test_controlnet_sdxl.py::StableDiffusionSSD1BControlNetPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/pag/test_pag_sdxl.py::StableDiffusionXLPAGPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/pag/test_pag_sdxl_img2img.py::StableDiffusionXLPAGImg2ImgPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/pag/test_pag_sdxl_inpaint.py::StableDiffusionXLPAGInpaintPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/pag/test_pag_controlnet_sdxl.py::StableDiffusionXLControlNetPAGPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/controlnet_xs/test_controlnetxs_sdxl.py::StableDiffusionXLControlNetXSPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/stable_diffusion_xl/test_stable_diffusion_xl_instruction_pix2pix.py::StableDiffusionXLInstructPix2PixPipelineFastTests::test_save_load_optional_components - AttributeError: 'NoneType' object has no attribute 'tokenize'
FAILED tests/pipelines/deepfloyd_if/test_if.py::IFPipelineFastTests::test_save_load_optional_components - TypeError: 'NoneType' object is not callable
FAILED tests/pipelines/deepfloyd_if/test_if_img2img.py::IFImg2ImgPipelineFastTests::test_save_load_optional_components - TypeError: 'NoneType' object is not callable
FAILED tests/pipelines/deepfloyd_if/test_if_inpainting.py::IFInpaintingPipelineFastTests::test_save_load_optional_components - TypeError: 'NoneType' object is not callable

@sayakpaul
Copy link
Member Author

@hlky yeah I am working on them as we speak. We should now skip them now that we have removed the optional components class (SDXLOptionalComponentsTesterMixin).

@sayakpaul
Copy link
Member Author

@hlky could you check now?

Copy link
Contributor

@hlky hlky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, the remaining failing tests are unrelated.

Useful test to have and the differences in encode_prompt interface is useful information for future refactors.

@sayakpaul sayakpaul merged commit b2ca39c into main Feb 20, 2025
14 of 15 checks passed
@sayakpaul sayakpaul deleted the tests-encode-prompt branch February 20, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants