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

Refactor process executors to be in AcceleratorState #1039

Merged
merged 4 commits into from
Feb 6, 2023

Conversation

muellerzr
Copy link
Collaborator

Based on conversation in slack, refactors the logic for process executors to be handled in the AcceleratorState and delegated to the Accelerator instead of in the Accelerator itself so that the State can be used by individuals that want to do process delegation without needing multiple Accelerator objects in their codebase.

Still TODO is to move the decorators over, but those need some more improvements due to confusions in them and tests so that is a much later to-get-to

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 3, 2023

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.

Never sure between yield and yield from but otherwise LGTM!

@@ -573,7 +558,7 @@ def main_process_first(self):
... print(f"This will be printed by process {accelerator.process_index}")
```
"""
yield from self._goes_first(self.is_main_process)
yield self.state.main_process_first()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shoudn't this be yield from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using yield from actually will raise an error and using yield with the inner calling yield from is enough. I'll add a test with this first but the multi-GPU tests passed :)

@muellerzr muellerzr added the enhancement New feature or request label Feb 6, 2023
@muellerzr muellerzr merged commit a60640d into main Feb 6, 2023
@muellerzr muellerzr deleted the state-passthrough branch February 6, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants