introduce compute arch specific expectations and fix test_sd3_img2img_inference failure #11227
Conversation
…merical characteristics Signed-off-by: YAO Matrix <matrix.yao@intel.com>
|
@faaany , if this approved and merged finally, we can follow same design to handle other numerical expectation related issues later. |
|
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.
Hi @yao-matrix. Thanks for making this PR, nice improvement. Copying from transformers is ok in this case, diffusers is still supporting Python 3.8, we will hopefully drop it by 0.34, the suggested changes here are all for that purpose. After those changes are applied and a make style && make quality, which I can apply via our bot, this should be good to go.
cc @yiyixuxu for awareness
|
@bot /style |
|
Style fixes have been applied. View the workflow run here. |
|
|
||
|
|
||
| @functools.lru_cache | ||
| def get_device_properties() -> DeviceProperties: |
There was a problem hiding this comment.
Should this return a dict instead? We could have sensible key namings like:
{"device_type": "cuda", "compute_info": 8.0}
There was a problem hiding this comment.
Let's raise it internally with transformers team as it's best to keep this in sync with their code (except Python 3.8 changes)
There was a problem hiding this comment.
Hey!
Honestly any type that can be used as a key is fine by me.
Originally it was a dataclass with fields, but we wanted to keep it as simple as possible while still solving the problem it's addressing.
Feel free to upstream any improvements you can think of
|
Failing tests are unrelated. Merging to not be a blocker, with a note to revisit |
@hlky , as you know, different compute arch can lead to different expected numeric for same case.
One example is:
pytest -rA tests -k "StableDiffusion3Img2ImgPipelineSlowTests and test_sd3_img2img_inference"will fail on A100 and XPU since currentexpected_sliceis designed for T4 (CUDA 7).By borrowing the design from
transformersfor the same case(as @ivarflakstad introduced in huggingface/transformers#36569), I am proposing to use the same mechanism in diffusers tests.I copied the implementations from transformers to avoid unnecessary versioning dependency, and let me know if you think directly import from transformers is more preferred from you.
W. this PR, test_sd3_img2img_inference case passed both pass on A100 and XPU 1550; before both failed.
Thx.