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

[Core] be more specific when doing accelerate imports. #8772

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sayakpaul
Copy link
Member

What does this PR do?

By being more specific on the imports, we can cut down the overall import time in our library.

Follow this thread: https://huggingface.slack.com/archives/C021H1P1HKR/p1719924173632779.

We should try to identify more such opportunities.

@sayakpaul sayakpaul requested review from DN6 and yiyixuxu July 2, 2024 13:22
Copy link
Contributor

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Nice!

Just want to point out this is more style, doesn't change import time I found when using main/the new methods

@@ -83,9 +83,9 @@ def run(self) -> dict:

accelerate_version = "not installed"
if is_accelerate_available():
import accelerate
from accelerate import __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually speed things up, see my trace here from accelerate:
Before:
image

After:
image

@@ -72,7 +72,7 @@


if is_accelerate_available():
import accelerate
from accelerate import init_empty_weights, load_checkpoint_and_dispatch
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here
image

@sayakpaul
Copy link
Member Author

@muellerzr just making sure that all of your findings are from accelerate main right?

@muellerzr
Copy link
Contributor

@sayakpaul yep!

@muellerzr
Copy link
Contributor

So if you want to support older versions/worry about that, definitely feel free to do so. :)

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

@sayakpaul
Copy link
Member Author

IMO it's better to rely on a stable version of accelerate rather than relying on the main branch.

@muellerzr
Copy link
Contributor

Agreed, main won't be main for long (literally this week) so up to you!

@yiyixuxu
Copy link
Collaborator

yiyixuxu commented Jul 4, 2024

@muellerzr
so sounds like we won't need this once the new version is released?

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.

None yet

4 participants