Skip to content

Added args/kwargs to idist barrier #2353

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 38 commits into
base: master
Choose a base branch
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
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 for the updates @fco-dv !
Just a few nits to fix and I think good to go

fco-dv and others added 2 commits December 22, 2021 14:30
vfdev-5
vfdev-5 previously approved these changes Dec 22, 2021
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

@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

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

@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 ?

# 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
module: distributed Distributed module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Add kwargs to idist.barrier
2 participants