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

Bugfix device map detr model #26849

Merged
merged 8 commits into from
Oct 23, 2023

Conversation

pedrogengo
Copy link
Contributor

@pedrogengo pedrogengo commented Oct 16, 2023

What does this PR do?

Fixes #26700 #23145

Who can review?

@LysandreJik @SunMarc

@SunMarc SunMarc self-requested a review October 16, 2023 21:35
Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Hi @pedrogengo thanks for making detr model compatible with device_map ! The PR looks good overall. I don't think that we need to add tests for device_map because we already have them. You just need to make sure that you can pass the accelerate tests for all models. This will trigger the following tests: test_disk_offload, test_cpu_offload and test_model_parallelism. Please use 2 GPUs to tests them. If you don't have them, i can run the tests for you!

@pedrogengo
Copy link
Contributor Author

Hi @SunMarc :) I don't have 2 GPUs unfortunately. Can you run on your side? In the meantime I can remove the tests I added + fix the style issue

@pedrogengo
Copy link
Contributor Author

pedrogengo commented Oct 17, 2023

@SunMarc addressed your comment on my side. Could you test?
I also noticed a bad pattern maybe on check_copies.py. At first, I tried this:

# Copied from transformers.models.deformable_detr.modeling_deformable_detr.DeformableDetrPreTrainedModel with DeformableDetr->Deta,DeformableDetrConvEncoder->DetaBackboneWithPositionalEncodings

And it was breaking the CI, because it first replace DeformableDetr to Deta, and after search for the next pair, which was DeformableDetrConvEncoder->DetaBackboneWithPositionalEncodings. However, as we first replaced, it turns that in the code DeformableDetrConvEncoder was changed to DetaConvEncoder. Maybe we can sort by length and start replacing from the largest string, and also replace with a placeholder in both codes to avoid replacing more than one time. Just something that I saw haha I can propose a PR for that, but I don't think it is that much important.

Copy link
Member

@SunMarc SunMarc 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've tested the accelerate tests on 2GPUs and they passed. Feel free to raise an issue or a PR to fix the pattern on check_copies.

Copy link
Collaborator

@amyeroberts amyeroberts 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 fixing @pedrogengo, and thanks @SunMarc for running the accelerate tests!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@SunMarc SunMarc merged commit f370beb into huggingface:main Oct 23, 2023
18 checks passed
staghado pushed a commit to staghado/transformers that referenced this pull request Oct 24, 2023
* Fixed replace_batch_norm when on meta device

* lint fix

* Adding coauthor

Co-authored-by: Pi Esposito <piero.skywalker@gmail.com>

* Removed tests

* Remove unused deps

* Try to fix copy issue

* try fix copy one more time

* Reverted import changes

---------

Co-authored-by: Pi Esposito <piero.skywalker@gmail.com>
@SunMarc SunMarc mentioned this pull request Oct 26, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
* Fixed replace_batch_norm when on meta device

* lint fix

* Adding coauthor

Co-authored-by: Pi Esposito <piero.skywalker@gmail.com>

* Removed tests

* Remove unused deps

* Try to fix copy issue

* try fix copy one more time

* Reverted import changes

---------

Co-authored-by: Pi Esposito <piero.skywalker@gmail.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.

NotImplementedError: Cannot copy out of meta tensor; no data! when using device = "auto" in pipeline()
4 participants