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

Use device as context manager for init_on_device #1826

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

shingjan
Copy link
Contributor

@shingjan shingjan commented Aug 8, 2023

fixes #1814

cc: @sgugger

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 8, 2023

The documentation is not available anymore as the PR was closed or merged.

@shingjan shingjan force-pushed the shingjan/use_device branch 3 times, most recently from b488347 to ece578c Compare August 9, 2023 00:27
Copy link
Collaborator

@sgugger sgugger 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 drafting this! I left a couple of comments, mainly we can't change the default of include_buffers which would break backward compatibility.

src/accelerate/big_modeling.py Outdated Show resolved Hide resolved
src/accelerate/big_modeling.py Outdated Show resolved Hide resolved
src/accelerate/big_modeling.py Outdated Show resolved Hide resolved
tests/test_big_modeling.py Outdated Show resolved Hide resolved
Copy link
Member

@BenjaminBossan BenjaminBossan 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 the PR. Is it possible to have a test for this or would it be too complicated to set up because of the version dependency?

src/accelerate/big_modeling.py Outdated Show resolved Hide resolved
src/accelerate/big_modeling.py Outdated Show resolved Hide resolved
@shingjan shingjan force-pushed the shingjan/use_device branch 4 times, most recently from 4afad01 to 7237f9a Compare August 9, 2023 21:42
@shingjan
Copy link
Contributor Author

shingjan commented Aug 9, 2023

Thanks @sgugger and @BenjaminBossan for the reviews!

I added one test with include_buffers=True to make sure if testing with torch >= 2.0, we go with the device context manager. This PR however won't be fixing the issue I had before:

with tensor_mode():
  model = transformers.from_pretrained(...)

as init_empty_weights(...) uses include_buffers=False as default. I wonder if you have a better context on why we use include_buffers=False as default for init_empty_weights. If the ultimate goal is to move a nn.Module to meta device in order to save cpu/cuda memory, wouldn't buffer also be included in the state dict of a nn.Module and therefore better to move those to meta as well? Thanks again!

@sgugger
Copy link
Collaborator

sgugger commented Aug 10, 2023

We use include_buffers=False because buffers are usually not very heavy and it ends up taking more time to create them on the meta device and move them around than just creating them on the GPU.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Nice new test!

@sgugger sgugger merged commit 058a354 into huggingface:main Aug 10, 2023
24 checks passed
@shingjan
Copy link
Contributor Author

thanks @sgugger and @BenjaminBossan for getting this PR in!

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.

big_modeling.init_on_device not working with WrapperTensor class like FakeTensor
4 participants