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

Typeless communicator #4678

Merged
merged 1 commit into from Jun 8, 2020
Merged

Typeless communicator #4678

merged 1 commit into from Jun 8, 2020

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented May 27, 2020

  • This allows to get rid of the macros for all_reduce, all_to_all, broadcast, gather, and scatter

Relies on #4676 (only the last commit is significant)

msimberg
msimberg previously approved these changes May 28, 2020
Copy link
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Can't comment on the implementation, but less boilerplate is definitely good. Thanks!

@hkaiser
Copy link
Member Author

hkaiser commented May 28, 2020

Can't comment on the implementation, but less boilerplate is definitely good. Thanks!

@msimberg the idea is to replace the std::vector<T> in the communicator with a util::any allowing to defer specifying the actual type of the data to collect to the point where the first participant of a particular collective operation contacts the communicator.

@biddisco
Copy link
Contributor

The use of scatter_to and scatter_from / gather_here + gather_there is a truly awful API. It forces the user to place if locality== checks around calls and makes the code look terrible. Much better would be a gather/scatter call with the check for locality done inside the function itself so that user code was symmetrical wrt locality.

@hkaiser
Copy link
Member Author

hkaiser commented May 29, 2020

The use of scatter_to and scatter_from / gather_here + gather_there is a truly awful API. It forces the user to place if locality== checks around calls and makes the code look terrible. Much better would be a gather/scatter call with the check for locality done inside the function itself so that user code was symmetrical wrt locality.

@biddisco For scatter this is done on purpose. I considered a single API, however that would require all participating sites to supply a (possibly default constructed value) that would be discarded in the end as just one site is producing the data for all of them. Please share your ideas of a possible concrete API here.

For gather, please note that #4676 already adds a common API that is doing exactly what you are suggesting.

@hkaiser
Copy link
Member Author

hkaiser commented May 30, 2020

For gather, please note that #4676 already adds a common API that is doing exactly what you are suggesting.

I'm not so sure anymore if introducing a common API is really a good idea. The API introduced by #4676 looks like:

template <typename T> 
hpx::future<std::vector<std::decay_t<T>>> gather(char const* basename, 
    T&& local_result, std::size_t num_sites = std::size_t(-1), 
    std::size_t generation = std::size_t(-1), 
    std::size_t this_site = std::size_t(-1), std::size_t root_site = 0); 

which is essentially equivalent to an all_gather as all participating sites receive the overall result. This is not what we want, I think - but let's discuss this.

If no further opinions are voiced here, I'll go back to #4676 and remove the common API for gather before it's merged.

@hkaiser hkaiser force-pushed the typeless_communicator branch 2 times, most recently from 10d175c to 6808ef7 Compare May 30, 2020 15:18
@biddisco
Copy link
Contributor

OK. I understand, if the interface allow passing by value, then having a default construction of something that isn't used is pointless. I would hope that most of the time, things are being passed via pointers/references rather than copies of std::vectors and suchlike. If there is also a symmetric gather/scatter function that allows the (dare I say it) MPI like API, then that's fine.

@hkaiser
Copy link
Member Author

hkaiser commented May 30, 2020

OK. I understand, if the interface allow passing by value, then having a default construction of something that isn't used is pointless. I would hope that most of the time, things are being passed via pointers/references rather than copies of std::vectors and suchlike.

The problem would go away only if we allowed for pointers in the API, which I would like to avoid.

@hkaiser
Copy link
Member Author

hkaiser commented Jun 2, 2020

This is now good to go as well.

@hkaiser hkaiser force-pushed the typeless_communicator branch 2 times, most recently from dd34beb to eeeb4d0 Compare June 5, 2020 13:12
- This allows to get rid of the macros for all_reduce, all_to_all, broadcast, gather, and scatter
@msimberg msimberg merged commit eabcc18 into master Jun 8, 2020
@msimberg msimberg deleted the typeless_communicator branch June 8, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants