Skip to content
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

[tests] enable test_pipeline_accelerate_top_p on XPU #29309

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

faaany
Copy link
Contributor

@faaany faaany commented Feb 27, 2024

What does this PR do?

Is there any particular reason why we use device_map="auto" in this test test_pipeline_accelerate_top_p? If not, I would suggest using the device-agnostic variable torch_device instead of device_map="auto" just like in all other tests e.g. test_small_model_fp16.

Another reason is that the device_map="auto" mechanism is still not mature on XPU, causing the model to be loaded on the CPU, rather than on XPU. With this fix, test_pipeline_accelerate_top_p will definitively work on all devices. Below is an evidence for XPU:

BEFORE

================================================== short test summary info ===================================================
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:221: test requires TensorFlow
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:378: test requires CUDA
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:180: test requires TensorFlow
FAILED tests/pipelines/test_pipelines_text_generation.py::TextGenerationPipelineTests::test_small_model_fp16 - RuntimeError: could not execute a primitive
===================================== 1 failed, 5 passed, 3 skipped, 8 warnings in 7.18s =====================================

AFTER

================================================== short test summary info ===================================================
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:221: test requires TensorFlow
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:378: test requires CUDA
SKIPPED [1] tests/pipelines/test_pipelines_text_generation.py:180: test requires TensorFlow
========================================== 6 passed, 3 skipped, 8 warnings in 7.52s ==========================================

Pls have a review, thx! @Narsil @ArthurZucker

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

I think in this one we want to test accelerate support with sampling so it's probably on purpose!

@faaany
Copy link
Contributor Author

faaany commented Feb 29, 2024

I think in this one we want to test accelerate support with sampling so it's probably on purpose!

Could you elaborate more on the relationship between top_p sampling and device? I don't see the need to use "device_map='auto'" in this case, e.g. if I change "device_map='auto'" to "device=torch_device", will the test on top_p sampling fail?

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

The name of the test is test_pipeline_accelerate_top_p to make sure we test accelerate dispatching automatically.
I thought testing "auto" made sense but you're right here, we can test any device, as long as we make sure we test the accelerate support here!

tests/pipelines/test_pipelines_text_generation.py Outdated Show resolved Hide resolved
faaany and others added 2 commits March 4, 2024 16:02
Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
@faaany
Copy link
Contributor Author

faaany commented Mar 4, 2024

The name of the test is test_pipeline_accelerate_top_p to make sure we test accelerate dispatching automatically. I thought testing "auto" made sense but you're right here, we can test any device, as long as we make sure we test the accelerate support here!

done, thanks for the review!

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks for iterating!

@ArthurZucker ArthurZucker merged commit fa7f3cf into huggingface:main Mar 5, 2024
17 checks passed
damithsenanayake pushed a commit to damithsenanayake/transformers that referenced this pull request Mar 7, 2024
* use torch_device

* Update tests/pipelines/test_pipelines_text_generation.py

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

* fix style

---------

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
itazap pushed a commit that referenced this pull request May 14, 2024
* use torch_device

* Update tests/pipelines/test_pipelines_text_generation.py

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>

* fix style

---------

Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
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.

2 participants