Skip to content

add on_main_process decorators - #488

Merged
sgugger merged 8 commits into
huggingface:mainfrom
ZhiyuanChen:patch-2
Jul 26, 2022
Merged

add on_main_process decorators#488
sgugger merged 8 commits into
huggingface:mainfrom
ZhiyuanChen:patch-2

Conversation

@ZhiyuanChen

Copy link
Copy Markdown
Contributor

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Jul 5, 2022

Copy link
Copy Markdown

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

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

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

ZhiyuanChen commented Jul 21, 2022

Copy link
Copy Markdown
Contributor Author

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

@muellerzr

muellerzr commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

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

Thanks!

@ZhiyuanChen

Copy link
Copy Markdown
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...

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

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!

Comment thread src/accelerate/accelerator.py Outdated
Comment thread src/accelerate/accelerator.py Outdated
Comment thread src/accelerate/accelerator.py Outdated

return wrapper

def on_process(process_idx):

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

sgugger commented Jul 26, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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