-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[Ray] Use num_workers API for Horovod Ray #2870
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
Signed-off-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
Signed-off-by: Richard Liaw <rliaw@berkeley.edu> Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
e75b08c
to
4d3c014
Compare
@richardliaw this is ready for review |
This comment has been minimized.
This comment has been minimized.
13d5230
to
33cf6e8
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Amog Kamsetty <amogkamsetty@yahoo.com>
This comment has been minimized.
This comment has been minimized.
Awesome work @amogkam! Is this working with the |
horovod/ray/runner.py
Outdated
raise DeprecationWarning("`num_slots` is now deprecated. Please " | ||
"use the `num_workers` API, " | ||
"or to enforce an equal number of " | ||
"workers on each node, set " | ||
"`num_hosts` and `num_workers_per_host`") | ||
if cpus_per_slot or gpus_per_slot: | ||
raise DeprecationWarning("`cpus_per_slot` and `gpus_per_slot` " | ||
"have been deprecated. Use " | ||
"`cpus_per_worker` and " | ||
"`gpus_per_worker` instead.") |
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.
for 1 release, can we make this a soft warning (warnings.warn) and then in the later release make this a hard warning?
This will make it much easier to enable migration,
Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
@tgaddair yep! just added a test for that too. |
This comment has been minimized.
This comment has been minimized.
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.
Looks great!
Unit Test Results 792 files ±0 792 suites ±0 5h 37m 0s ⏱️ ±0s For more details on these failures, see this check. Results for commit 39aa85b. ± Comparison against base commit 39aa85b. |
Update to use latest Horovod-Ray API (horovod/horovod#2870)
Checklist before submitting
Description
Closes #2702
Review process to land