Skip to content

Bugfix device map detr model#26849

Merged
SunMarc merged 8 commits intohuggingface:mainfrom
pedrogengo:bugfix-device-map-detr-model
Oct 23, 2023
Merged

Bugfix device map detr model#26849
SunMarc merged 8 commits intohuggingface:mainfrom
pedrogengo:bugfix-device-map-detr-model

Conversation

@pedrogengo
Copy link
Contributor

@pedrogengo pedrogengo commented Oct 16, 2023 •
edited
Loading

What does this PR do?

Fixes #26700 #23145

Who can review?

@LysandreJik @SunMarc

@pedrogengo pedrogengo force-pushed the bugfix-device-map-detr-model branch from bb926e0 to f8f70c7 Compare October 16, 2023 19:37
@SunMarc SunMarc self-requested a review October 16, 2023 21:35
SunMarc
SunMarc reviewed Oct 16, 2023
View reviewed changes
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

pedrogengo commented Oct 16, 2023

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 pedrogengo force-pushed the bugfix-device-map-detr-model branch from f8f70c7 to 43a5770 Compare October 17, 2023 21:46
@pedrogengo
Copy link
Contributor Author

pedrogengo commented Oct 17, 2023 •
edited
Loading

@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.

@pedrogengo pedrogengo requested a review from SunMarc October 17, 2023 23:39
SunMarc
SunMarc approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@SunMarc SunMarc left a comment •
edited
Loading

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.

@SunMarc SunMarc requested a review from amyeroberts October 18, 2023 18:39
amyeroberts
amyeroberts approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@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
Copy link

HuggingFaceDocBuilderDev commented Oct 23, 2023

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
@SunMarc SunMarc mentioned this pull request Oct 26, 2023
Merged
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
@pedrogengo @piEsposito @EduardoPach
Bugfix device map detr model (huggingface#26849)
6f96ce4 
* 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>
@NielsRogge NielsRogge mentioned this pull request Feb 12, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SunMarc SunMarc SunMarc approved these changes

+1 more reviewer

@amyeroberts amyeroberts amyeroberts approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

@pedrogengo @HuggingFaceDocBuilderDev @amyeroberts @SunMarc