-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Deprecate Trainer.devices
in favor of Trainer.num_devices
and Trainer.device_ids
#12151
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
327b6a8
fix tests
awaelchli 9013c73
fix tests
DuYicong515 264cf80
update test
DuYicong515 7b88216
address comments
DuYicong515 89a5044
address comments for testing
DuYicong515 3c38609
comments
DuYicong515 24d6c8c
docstring
DuYicong515 94e0016
simplify test in deprecated_api test
DuYicong515 6e65276
improve tests and docstring
DuYicong515 91dcc69
change num_devices to use device_ids
DuYicong515 54c5703
add pl_multi_process_test decorator to TPU num_devices test
DuYicong515 8e6dbc4
Comment test
kaushikb11 9f6c899
Remove pl_multi_process_test
kaushikb11 0cc5b05
skip newly added TPU tests
DuYicong515 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred if this was implemented on the accelerator connector. It is IMO not the responsibility of the Trainer to compute the device_ids off the strategy. Note the the strategy is owned by the connector, not the trainer. As the trainer has already many responsibilities, we should delegate as much as possible to the concrete components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing @awaelchli!
My personal thoughts: the accelerator connector should stay internal, and accelerator connector should not expose those device related public properties.
Personally I feel the accelerator connector does a lot of checks/set up/initialization and configuration of the accelerator and strategy. After things are set up already, strategy/accelerator are already accessible on the Trainer, while the connector stays internal.
I think delegating computations using accelerator/strategy to the accelerator_connector will make us add many public properties to the internal-facing accelerator_connector. Deriving from strategy/accelerator directly are more straight forward, and they(strategy, accelerator) are very external facing as well.
I'm very new to this and my feelings might be wrong. Feel free to add more thoughts to issue #12171!