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

add on_main_process decorators #488

Merged
merged 8 commits into from
Jul 26, 2022
Merged

Conversation

ZhiyuanChen
Copy link
Contributor

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jul 5, 2022

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

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 your PR! I think it would be great to have those as methods of the Accelerator object (your code refers to a `self`` that does not exist otherwise).

@ZhiyuanChen
Copy link
Contributor Author

ZhiyuanChen commented Jul 21, 2022

Hi @sgugger , sorry for the delay, hope this is better now

@muellerzr
Copy link
Collaborator

muellerzr commented Jul 21, 2022

@ZhiyuanChen could you run "make style; make quality" to solve the Quality Check issue? 😃

Thanks!

@ZhiyuanChen
Copy link
Contributor Author

@ZhiyuanChen could you run "make style; make quality" to solve the Quality Check issue? 😃

Thanks!

Sorry, I thought I have fixed it but somehow didn't pushed...

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 a lot for iterating! Almost ready to merge, I just left some comments on the docstrings and a suggestion to group the decorators on_process and on_local_process together. Let me know your thoughts!

src/accelerate/accelerator.py Outdated Show resolved Hide resolved
src/accelerate/accelerator.py Outdated Show resolved Hide resolved
src/accelerate/accelerator.py Outdated Show resolved Hide resolved

return wrapper

def on_process(process_idx):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def on_process(process_idx):
def on_process(process_idx, local=False):

Maybe we could group this one and the text in one decorator since it's one that takes arguments?

ZhiyuanChen and others added 3 commits July 22, 2022 20:49
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@ZhiyuanChen
Copy link
Contributor Author

Thanks a lot for iterating! Almost ready to merge, I just left some comments on the docstrings and a suggestion to group the decorators on_process and on_local_process together. Let me know your thoughts!

Thank you very much tor your comments and suggestion.

For the docstrings, i have accepted rll suggestions.

For the group... I think its rather important to ensure api in a similar organisation, and grouping in this way break the consistency with is_local_main_process and on_local_main_process.
Though we could use alias/partial/impl to make the implementation more compact

@sgugger
Copy link
Collaborator

sgugger commented Jul 26, 2022

Let's roll with your choice and we'll see what users think then. We can always add aliases in the future :-)

Thanks again for your contribution!

@sgugger sgugger merged commit 7d97e9c into huggingface:main Jul 26, 2022
@ZhiyuanChen
Copy link
Contributor Author

Let's roll with your choice and we'll see what users think then. We can always add aliases in the future :-)

Haha, sure thing~

Thanks again for your contribution!

No worries~

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