PipelineTesterMixin parameter configuration refactor #2502

williamberman · 2023-02-27T04:35:04Z

Currently used test config

`allowed_required_args`

testing that all required parameters on all pipelines are from a subset of variable names -> ends up not really testing much because when we add a new pipeline with a new required parameter, we just add it to the list. this isn't really checking an invariant or anything.
also being used to configure what parameters to batch in another location -> this is really separate configuration as there's no real requirement that required parameters are batched
This also gets a bit confusing as we don't have consistent treatment of required parameters and in many cases don't have any required parameters. I.e. prompt which you would think is the canonical required parameter is really technically optional in most pipelines as we allow overriding it with prompt_embeddings.

`required_optional_params`

This is a good config but we could add a few more of the commonly included optional parameters

`num_inference_steps_args`

This was a bad hack that I added for testing batching on the unclip pipelines with many different inference stages.

Refactor

num_inference_steps_args -> remove and pass as argument to test helper function

required_optional_parameters is good but we want to add a few more of the default optional parameters i.e. callback/callback_steps.

Really we want to separate allowed_required_args into two parts:

A config for a set of parameters that are included in all (optional and required) paramers -> params
A config for batched parameters -> batch_params

Both params and batch_params are generally determined by the type of pipeline -- i.e. text to image, image variation. They are configured on the specific pipeline subclass with a good error message explaining why and how to configure them when they are not set. Because we don't have strict interfaces (i.e. not all image variation pipelines take height and width) on pipelines nor families of pipelines, params are easily tweaked from provided defaults with operations on frozenset.

HuggingFaceDocBuilderDev · 2023-02-27T04:39:32Z

The documentation is not available anymore as the PR was closed or merged.

williamberman · 2023-02-27T06:56:50Z

tests/pipeline_params.py

+# These are canonical sets of parameters for different types of pipelines.
+# They are set on subclasses of `PipelineTesterMixin` as `params` and
+# `batch_params`.
+#
+# If a pipeline's set of arguments has minor changes from one of the common sets
+# of arguments, do not make modifications to the existing common sets of arguments.
+# I.e. a text to image pipeline with non-configurable height and width arguments
+# should set its attribute as `params = TEXT_TO_IMAGE_PARAMS - {'height', 'width'}`.
+
+TEXT_TO_IMAGE_PARAMS = frozenset(
+    [
+        "prompt",
+        "height",
+        "width",
+        "guidance_scale",
+        "negative_prompt",
+        "prompt_embeds",
+        "negative_prompt_embeds",
+        "cross_attention_kwargs",
+    ]
+)
+
+TEXT_TO_IMAGE_BATCH_PARAMS = frozenset(["prompt", "negative_prompt"])


Default param values for families of pipelines

williamberman · 2023-02-27T06:57:37Z

tests/pipelines/stable_diffusion/test_stable_diffusion_img2img.py

+    params = TEXT_GUIDED_IMAGE_VARIATION_PARAMS - {"height", "width"}
+    required_optional_params = PipelineTesterMixin.required_optional_params - {"latents"}
+    batch_params = TEXT_GUIDED_IMAGE_VARIATION_BATCH_PARAMS


Example modifying default params

williamberman · 2023-02-27T06:58:04Z

tests/test_pipelines_common.py

+    # Canonical parameters that are passed to `__call__` regardless
+    # of the type of pipeline. They are always optional and have common
+    # sense default values.
+    required_optional_params = frozenset(
+        [
+            "num_inference_steps",
+            "num_images_per_prompt",
+            "eta",
+            "generator",
+            "latents",
+            "output_type",
+            "return_dict",
+            "callback",
+            "callback_steps",
+        ]
+    )


new set of required_optional_params

tests/test_pipelines_common.py

williamberman · 2023-02-27T06:58:33Z

tests/test_pipelines_common.py

+    @property
+    def batch_params(self) -> frozenset:
+        raise NotImplementedError(
+            "You need to set the attribute `batch_params` in the child test class. "
+            "`batch_params` are the parameters required to be batched when passed to the pipeline's "
+            "`__call__` method. `pipeline_params.py` provides some common sets of parameters such as "
+            "`TEXT_TO_IMAGE_BATCH_PARAMS`, `IMAGE_VARIATION_BATCH_PARAMS`, etc... If your pipeline's "
+            "set of batch arguments has minor changes from one of the common sets of batch arguments, "
+            "do not make modifications to the existing common sets of batch arguments. I.e. a text to "
+            "image pipeline `negative_prompt` is not batched should set the attribute as "
+            "`batch_params = TEXT_TO_IMAGE_BATCH_PARAMS - {'negative_prompt'}`. "
+            "See existing pipeline tests for reference."
+        )


batch_params with error message/docs

williamberman · 2023-02-27T06:58:54Z

note failing onnx tests seem unrelated

sayakpaul · 2023-02-27T11:18:21Z

tests/pipelines/dance_diffusion/test_dance_diffusion.py

+        "callback",
+        "latents",
+        "callback_steps",
+        "output_type",
+        "eta",
+        "num_images_per_prompt",


I'd group them into variable and then use it instead of defining ad-hoc. Applies here and elsewhere.

Actually ok for me as is - I'd just move "eta" out of the "required" optional base params. ETA is too specific & we shouldn't incentivize adding it in the future

tests/test_pipelines_common.py

patrickvonplaten

Looks very nice! My only suggestion is to move "eta" out of the required/default list of optional parameters. ETA is only relevant for DDIM and is also generally not used.

Going forward I actually don't think it's a good idea to have a "eta" argument for pipelines

patrickvonplaten · 2023-03-01T08:52:08Z

tests/test_pipelines_common.py

+        [
+            "num_inference_steps",
+            "num_images_per_prompt",
+            "eta",


Suggested change

"eta",

I'd remove "eta" here. "eta" is a very specific case that doesn't apply to all pipelines. We probably shouldn't have added this at all in the first place

patrickvonplaten · 2023-03-01T08:52:19Z

tests/test_pipelines_common.py

+    @property
+    def batch_params(self) -> frozenset:
+        raise NotImplementedError(
+            "You need to set the attribute `batch_params` in the child test class. "
+            "`batch_params` are the parameters required to be batched when passed to the pipeline's "
+            "`__call__` method. `pipeline_params.py` provides some common sets of parameters such as "
+            "`TEXT_TO_IMAGE_BATCH_PARAMS`, `IMAGE_VARIATION_BATCH_PARAMS`, etc... If your pipeline's "
+            "set of batch arguments has minor changes from one of the common sets of batch arguments, "
+            "do not make modifications to the existing common sets of batch arguments. I.e. a text to "
+            "image pipeline `negative_prompt` is not batched should set the attribute as "
+            "`batch_params = TEXT_TO_IMAGE_BATCH_PARAMS - {'negative_prompt'}`. "
+            "See existing pipeline tests for reference."
+        )


patrickvonplaten · 2023-03-01T08:54:37Z

tests/pipelines/dance_diffusion/test_dance_diffusion.py

+        "callback",
+        "latents",
+        "callback_steps",
+        "output_type",
+        "eta",
+        "num_images_per_prompt",


Actually ok for me as is - I'd just move "eta" out of the "required" optional base params. ETA is too specific & we shouldn't incentivize adding it in the future

patrickvonplaten · 2023-03-01T08:55:52Z

tests/pipeline_params.py

+
+UNCONDITIONAL_AUDIO_GENERATION_PARAMS = frozenset(["batch_size"])
+
+UNCONDITIONAL_AUDIO_GENERATION_BATCH_PARAMS = frozenset([])


Super nice! This is really helpful to classify the pipelines into certain tasks and should help us write new tests going forward

@yiyixuxu

* attend and excite batch test causing timeouts * PipelineTesterMixin argument configuration refactor * error message text re: @yiyixuxu * remove eta re: @patrickvonplaten

@yiyixuxu

* attend and excite batch test causing timeouts * PipelineTesterMixin argument configuration refactor * error message text re: @yiyixuxu * remove eta re: @patrickvonplaten

attend and excite batch test causing timeouts

dde7095

williamberman force-pushed the pipeline_argument_tests branch 6 times, most recently from 0c1420a to bf21775 Compare February 27, 2023 06:07

williamberman changed the title ~~wip argument tests~~ PipelineTesterMixin argument configuration refactor Feb 27, 2023

williamberman changed the title ~~PipelineTesterMixin argument configuration refactor~~ PipelineTesterMixin parameter configuration refactor Feb 27, 2023

PipelineTesterMixin argument configuration refactor

e788d12

williamberman force-pushed the pipeline_argument_tests branch from bf21775 to e788d12 Compare February 27, 2023 06:42

williamberman commented Feb 27, 2023

View reviewed changes

tests/test_pipelines_common.py Show resolved Hide resolved

williamberman commented Feb 27, 2023

View reviewed changes

williamberman marked this pull request as ready for review February 27, 2023 06:59

williamberman requested review from patil-suraj, patrickvonplaten, pcuenca, sayakpaul and yiyixuxu February 27, 2023 06:59

williamberman mentioned this pull request Feb 27, 2023

move test num_images_per_prompt to pipeline mixin #2488

Merged

sayakpaul reviewed Feb 27, 2023

View reviewed changes

sayakpaul approved these changes Feb 27, 2023

View reviewed changes

yiyixuxu approved these changes Feb 27, 2023

View reviewed changes

tests/test_pipelines_common.py Show resolved Hide resolved

williamberman and others added 2 commits February 27, 2023 15:33

error message text re: @yiyixuxu

86ec209

Merge branch 'main' into pipeline_argument_tests

c50eb66

patrickvonplaten approved these changes Mar 1, 2023

View reviewed changes

williamberman added 2 commits March 1, 2023 12:26

Merge branch 'main' into pipeline_argument_tests

98df334

remove eta re: @patrickvonplaten

470ec3a

williamberman merged commit 1a6fa69 into huggingface:main Mar 1, 2023


		UNCONDITIONAL_AUDIO_GENERATION_PARAMS = frozenset(["batch_size"])

		UNCONDITIONAL_AUDIO_GENERATION_BATCH_PARAMS = frozenset([])

PipelineTesterMixin parameter configuration refactor #2502

PipelineTesterMixin parameter configuration refactor #2502

Uh oh!

Conversation

williamberman commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Currently used test config

allowed_required_args

required_optional_params

num_inference_steps_args

Refactor

Uh oh!

HuggingFaceDocBuilderDev commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamberman commented Feb 27, 2023

Uh oh!

sayakpaul Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickvonplaten left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

williamberman commented Feb 27, 2023 •

edited

Loading

`allowed_required_args`

`required_optional_params`

`num_inference_steps_args`

HuggingFaceDocBuilderDev commented Feb 27, 2023 •

edited

Loading

sayakpaul Feb 27, 2023 •

edited

Loading

patrickvonplaten left a comment •

edited

Loading