Skip to content

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Mar 21, 2021

Description:

  • Added safe_mode for Idist broadcast

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 Mar 21, 2021
@vfdev-5 vfdev-5 force-pushed the idist-broadcast-use-none branch from 1ef0d40 to d6b8be4 Compare March 21, 2021 23:41
@sdesrozis
Copy link
Contributor

@vfdev-5 I’m not sure about the use case.

As far I have understood, only one process holds a valid tensor and others have none. So the type is reduced to build a valid empty tensor (same style).

Build such empty tensor should be pretty easy for the user, shouldn’t it ?

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Mar 22, 2021

@sdesrozis here is the use-case where we are blocked a bit: #1758

Otherwise, the idea is to write the code like that:

t = None
if rank == src:
    t = ...  # can be tensor, number or str
t = idist.broadcast(t, src=src)

@vfdev-5 vfdev-5 marked this pull request as ready for review March 23, 2021 00:23
@vfdev-5 vfdev-5 requested a review from sdesrozis March 23, 2021 00:23
Copy link
Contributor

@sdesrozis sdesrozis left a comment

Choose a reason for hiding this comment

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

@vfdev-5 Perfect ! Maybe a matter of taste about the argument name use_none which is not very expressive but examples provided are.

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Mar 23, 2021

@sdesrozis any other better ideas on how to name it :) ?

I'll add a test to cover one of missing raise ValueError and good to go.

@sdesrozis
Copy link
Contributor

@sdesrozis any other better ideas on how to name it :) ?

use_none_target_tensors ?

Moreover, we could have a specific safe broadcast mode replacing every target tensors to avoid errors.

@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Mar 23, 2021

@sdesrozis any other better ideas on how to name it :) ?

use_none_target_tensors ?

we can broadcast not only tensors but also numbers and strings => use_none_target ?

Moreover, we could have a specific safe broadcast mode replacing every target tensors to avoid errors.

what do you mean by "replacing every target tensors to avoid errors." ?

@sdesrozis
Copy link
Contributor

what do you mean by "replacing every target tensors to avoid errors." ?

Stupid scenario

if rank == 0:
    t = tensor.empty((1,))
if rank == 1:
    t = 0
if rank == src: # src != 0 & src != 1
    t = 3
t = idist.broadcast(t, src=src, use_none=True) # here, max is tensor.empty (I suppose)

@vfdev-5 vfdev-5 changed the title Added use_none for Idist broadcast Added safe_mode for Idist broadcast Apr 19, 2021
@vfdev-5
Copy link
Collaborator Author

vfdev-5 commented Apr 19, 2021

@sdesrozis i renamed use_none to safe_mode and now handle also the case you talked about: #1839 (comment)

Could you please review again, thanks !

07a8dba

Copy link
Contributor

@sdesrozis sdesrozis left a comment

Choose a reason for hiding this comment

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

@vfdev-5 LGTM ! Safe mode with discard is nice !

@vfdev-5 vfdev-5 merged commit a44b327 into master Apr 20, 2021
@vfdev-5 vfdev-5 deleted the idist-broadcast-use-none branch April 20, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: distributed Distributed module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants