Skip to content

Conversation

@ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 16, 2025

What does this PR do?

torch.set_default_device has some unexpected impacts on other tests even if we try to restore the origin value by calling it at the end of the test. See #37551 for one example that is fixed there.

This PR uses torch._GLOBAL_DEVICE_CONTEXT to do something more close to the internal mechanism in torch.set_default_device to perform the cleanup, see

https://github.com/pytorch/pytorch/blob/e229ce34c4ab8cd4e2800227615be32fb362b1e6/torch/__init__.py#L1205-L1218

Running

RUN_SLOW=1 python3 -m pytest -v tests/models/mask2former/test_modeling_mask2former.py

will pass on this PR while it fails on the base commit (b33edf1) of this PR.

FAILED tests/models/mask2former/test_modeling_mask2former.py::Mask2FormerModelTest::test_torch_export - AttributeError: module 'torch._tensor' has no attribute 'split'

FAILED tests/models/mask2former/test_modeling_mask2former.py::Mask2FormerModelIntegrationTest::test_export - AttributeError: module 'torch._tensor' has no attribute 'split'

@github-actions
Copy link
Contributor

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the Ready for review button (at the bottom of the PR page). This will assign reviewers and trigger CI.

@github-actions github-actions bot marked this pull request as draft April 16, 2025 14:50
@ydshieh ydshieh marked this pull request as ready for review April 16, 2025 15:02
@ydshieh ydshieh requested a review from Cyrilvallez April 16, 2025 15:09
@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.

Copy link
Member

@Cyrilvallez Cyrilvallez left a comment

Choose a reason for hiding this comment

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

This is kind of convoluted (not your fault haha)... Don't you think that simply running in a subprocess would be better?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 16, 2025

yeah, i didn't see it is decorated with require_torch_accelerator and I tried to avoid overhead added to CircleCI. But now I just use subprocess.

@ydshieh ydshieh requested a review from Cyrilvallez April 16, 2025 15:37
Copy link
Member

@Cyrilvallez Cyrilvallez left a comment

Choose a reason for hiding this comment

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

Perfect, LGTM! Much simpler and foolproof! Let's just add a small comment about why we're doing it!

@ydshieh ydshieh merged commit 5a6de70 into main Apr 16, 2025
21 checks passed
@ydshieh ydshieh deleted the fix_after_37216 branch April 16, 2025 17:48
cyr0930 pushed a commit to cyr0930/transformers that referenced this pull request Apr 18, 2025
…gface#37553)

* fix

* fix

* fix

* Update tests/test_modeling_common.py

Co-authored-by: Cyril Vallez <[email protected]>

* fix

---------

Co-authored-by: ydshieh <[email protected]>
Co-authored-by: Cyril Vallez <[email protected]>
ydshieh added a commit that referenced this pull request Apr 18, 2025
* fix

* trigger CI

---------

Co-authored-by: ydshieh <[email protected]>
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request May 14, 2025
…gface#37553)

* fix

* fix

* fix

* Update tests/test_modeling_common.py

Co-authored-by: Cyril Vallez <[email protected]>

* fix

---------

Co-authored-by: ydshieh <[email protected]>
Co-authored-by: Cyril Vallez <[email protected]>
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request May 14, 2025
* fix

* trigger CI

---------

Co-authored-by: ydshieh <[email protected]>
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