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.

Sign up for GitHub

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

Jump to bottom

Added args/kwargs to idist barrier #2353

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Added args/kwargs to idist barrier #2353

wants to merge 38 commits into from

Conversation

fco-dv
Copy link
Contributor

@fco-dv fco-dv commented Dec 11, 2021

Fixes #2213

Description: Pass args and kwargs to idist.barrier method, route it to backends

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: distributed Distributed module label Dec 11, 2021
@fco-dv fco-dv marked this pull request as ready for review December 12, 2021 10:01
@fco-dv fco-dv requested review from vfdev-5 and sdesrozis and removed request for vfdev-5 December 19, 2021 16:28
@vfdev-5 vfdev-5 changed the title Idist barrier Added args/kwargs to idist barrier Dec 22, 2021
vfdev-5
vfdev-5 reviewed Dec 22, 2021
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the updates @fco-dv !
Just a few nits to fix and I think good to go

ignite/distributed/utils.py Outdated Show resolved Hide resolved
ignite/distributed/utils.py Outdated Show resolved Hide resolved
vfdev-5
vfdev-5 previously approved these changes Dec 22, 2021
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @fco-dv !

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 22, 2021 •
edited
Loading

@fco-dv looks good (approving the content), can we also add few tests of this new feature ?

@fco-dv
Copy link
Contributor Author

fco-dv commented Dec 22, 2021

thanks for approving @vfdev-5 ! yes I will add some

vfdev-5
vfdev-5 approved these changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @fco-dv ! LGTM

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 17, 2022 •
edited
Loading

@fco-dv are we good and can merge it ? If OK for you please merge it by yourself.

@vfdev-5 vfdev-5 enabled auto-merge (squash) January 17, 2022 13:13
@vfdev-5 vfdev-5 disabled auto-merge January 17, 2022 13:22
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 17, 2022

Checking this PR on GPUs: https://app.circleci.com/pipelines/github/pytorch/ignite/2413/workflows/0d61de1f-9ed6-4567-be02-7313a7b0cf45

@fco-dv
Copy link
Contributor Author

fco-dv commented Jan 17, 2022

seems like test_idist_barrier_hvd is failing on two_gpus_hvd_tests ...

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 25, 2022

@fco-dv what remains for this PR ?

fco-dv
fco-dv commented Jan 25, 2022
View reviewed changes
ignite/distributed/comp_models/horovod.py
# hvd.allreduce(torch.tensor(0, device=self.device()), name="barrier")
hvd.allreduce(torch.tensor(0, device="cpu"), name="barrier")
else:
hvd.barrier(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vfdev-5 not a lot on this PR remaining I think, just wanted to check the hvd 2 gpus tests for this because it seems to fail here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

@sdesrozis sdesrozis Awaiting requested review from sdesrozis

Assignees
No one assigned
Labels
module: distributed Distributed module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] Add kwargs to idist.barrier
2 participants
@fco-dv @vfdev-5