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

Adding all_to_all #3931

Merged
merged 12 commits into from Jun 28, 2019
Merged

Adding all_to_all #3931

merged 12 commits into from Jun 28, 2019

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 23, 2019

Adding all_to_all facility

  • flyby: create new module for collectives, all_to_all is the first (and only)
    facility in this module
  • flyby: adding promise::get_shared_future to allow extracting a shared future
    from a promise without access to any (shared-)future that was extracted
    from this promise before
  • flyby: minor fixes to gather_here/gather_there

This relies on #3902 being merged first

@hkaiser hkaiser added this to the 1.4.0 milestone Jun 23, 2019
@hkaiser hkaiser added this to TODO in HPX modularization via automation Jun 23, 2019
@hkaiser
Copy link
Member Author

hkaiser commented Jun 23, 2019

@msimberg I'd need some help integrating the doxygen documentation for the all_to_all to the new documentation system (see: https://github.com/STEllAR-GROUP/hpx/pull/3931/files#diff-43aea3f47587805148adec9676d6b19dR14). Any pointers/guidelines would be much appreciated.

@hkaiser hkaiser force-pushed the collectives branch 4 times, most recently from b4cca5a to 7c9be63 Compare June 24, 2019 00:53
@biddisco
Copy link
Contributor

What is the underlying implementation based on? I didn't look at the code yet but will review it later in the week if you wish. Does it use any tree based algorithms or anything fancy to minimize latency/bandwidth or total messages sent etc?

@hkaiser
Copy link
Member Author

hkaiser commented Jun 24, 2019

@biddisco the implementation is straightforward, nothing special. It creates a central point of collecting the information where all participants extract the data from. My goal was not to have something very high performant, I needed the abstraction. We can always get fancy implementation-wise later.

@msimberg
Copy link
Contributor

@msimberg I'd need some help integrating the doxygen documentation for the all_to_all to the new documentation system (see: https://github.com/STEllAR-GROUP/hpx/pull/3931/files#diff-43aea3f47587805148adec9676d6b19dR14). Any pointers/guidelines would be much appreciated.

It should "just work", i.e. anything not in detail is added as doxygen docs now. I'll check that it does the right thing though.

Another thing this does is create a cyclic dependency between this and the core library. I don't think we can avoid this at the moment without having all the prerequisites in place as modules, but I just want to point out that we should be very careful about not forgetting cyclic dependencies like these. I'm really happy to see this as a module though!

@hkaiser
Copy link
Member Author

hkaiser commented Jun 26, 2019

Another thing this does is create a cyclic dependency between this and the core library.

I thought about this. In this case it doesn't create an issue as all_to_all is header only...

- flyby: create new module for collectives, all_to_all is the first (and only)
         facility in this module
- flyby: adding promise::get_shared_future to allow extracting a shared future
         from a promise without access to any (shared-)future that was extracted
         from this promise before
- flyby: minor fixes to gather_here/gather_there

This relies on #3902 being merged
- flyby: remove superfluous file, properly install generated files
- flyby: fix unregistration for all_to_all server component
@hkaiser
Copy link
Member Author

hkaiser commented Jun 26, 2019

@msimberg this is good to go from my end, please review.

HPX modularization automation moved this from TODO to In Progress Jun 27, 2019
libs/CMakeLists.txt Outdated Show resolved Hide resolved
libs/_example/tests/CMakeLists.txt Show resolved Hide resolved
libs/collectives/docs/index.rst Show resolved Hide resolved
libs/collectives/examples/CMakeLists.txt Outdated Show resolved Hide resolved
@msimberg msimberg requested a review from biddisco June 27, 2019 09:39
@msimberg
Copy link
Contributor

@msimberg I'd need some help integrating the doxygen documentation for the all_to_all to the new documentation system (see: https://github.com/STEllAR-GROUP/hpx/pull/3931/files#diff-43aea3f47587805148adec9676d6b19dR14). Any pointers/guidelines would be much appreciated.

It should "just work", i.e. anything not in detail is added as doxygen docs now. I'll check that it does the right thing though.

@hkaiser you don't need to do anything additional for this to work.

- adding minimal docs
- fixing pseudo target names
- updating create_library_skeleton.py
@hkaiser
Copy link
Member Author

hkaiser commented Jun 27, 2019

@msimberg I think I have addressed all of your comments, thanks!

msimberg
msimberg previously approved these changes Jun 28, 2019
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.

Thanks!

@msimberg
Copy link
Contributor

@hkaiser the force_linking.hpp header was missing. I extended add_hpx_module to generate it instead, but could you verify that it still works correctly for you on Windows?

@hkaiser
Copy link
Member Author

hkaiser commented Jun 28, 2019

@msimberg thanks for looking into this. I applied a couple of minor changes to streamline the force-linking even further. Should be good to go now.

- Generating force_linking source files
- remove unneeded source files (those are being generated now)
- add missing source groups
- export modules as targets only if those are installed
@hkaiser hkaiser merged commit eb3d35f into master Jun 28, 2019
HPX modularization automation moved this from In Progress to Done Jun 28, 2019
@hkaiser hkaiser deleted the collectives branch June 28, 2019 18:44
@sithhell sithhell mentioned this pull request Aug 8, 2019
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants