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

Rework network interfaces args in horovod.run and horovodrun #3506

Merged
merged 3 commits into from Apr 27, 2022

Conversation

EnricoMi
Copy link
Collaborator

@EnricoMi EnricoMi commented Apr 6, 2022

Reworks network interfaces arguments in two places: the horovod.run API and the horovodrun CLI:

The horovod.run API deprecates network_interface argument in favour of network_interfaces, which becomes a list of strings, rather than a comma separated string that is to be split.

The horovodrun CLI replaces --network-interface with semantically more accurate --network-interfaces, which still takes a comma separated string of network interfaces, but also provides the new --network-interface argument, that semantically correctly expects a single network interface. That argument can be used multiple times to set multiple NICs.

Arguably, --network-interfaces could be removed completely, as --network-interface allows to specify multiple NICs. Unless, there is any use of comma separated strings.

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

Unit Test Results

     821 files  +     86       821 suites  +86   9h 16m 38s ⏱️ + 54m 39s
     768 tests ±       0       725 ✔️ +     47       43 💤  -   47  0 ±0 
18 950 runs  +1 990  13 655 ✔️ +1 460  5 295 💤 +530  0 ±0 

Results for commit b56af7d. ± Comparison against base commit 98db066.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 6, 2022

Unit Test Results (with flaky tests)

     911 files  +     90       911 suites  +90   10h 3m 28s ⏱️ + 1h 9m 37s
     768 tests ±       0       724 ✔️ +     46       43 💤  -   47  1 +1 
21 286 runs  +2 040  15 104 ✔️ +1 497  6 181 💤 +542  1 +1 

For more details on these failures, see this check.

Results for commit b56af7d. ± Comparison against base commit 98db066.

♻️ This comment has been updated with latest results.

Signed-off-by: Enrico Minack <github@enrico.minack.dev>
@EnricoMi EnricoMi marked this pull request as ready for review April 19, 2022 14:57
Copy link
Collaborator

@maxhgerlach maxhgerlach left a comment

Choose a reason for hiding this comment

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

LGTM; the new interface is clearer!

Left a minor comment regarding the argument help strings.

horovod/runner/launch.py Show resolved Hide resolved
Signed-off-by: Enrico Minack <github@enrico.minack.dev>
Signed-off-by: Enrico Minack <github@enrico.minack.dev>
@EnricoMi EnricoMi added this to the v0.25.0 milestone Apr 22, 2022
@EnricoMi EnricoMi merged commit 6285f3f into master Apr 27, 2022
@EnricoMi EnricoMi deleted the branch-nics-args branch April 27, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants