Add PMIX - #349
Conversation
|
cc @manopapad |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 27.15% 27.81% +0.65%
==========================================
Files 190 191 +1
Lines 39174 41142 +1968
Branches 14170 15281 +1111
==========================================
+ Hits 10637 11442 +805
- Misses 27128 28706 +1578
+ Partials 1409 994 -415 ☔ View full report in Codecov by Harness. |
| handle->barrier = bootstrap_pmix_barrier; | ||
| handle->bcast = bootstrap_pmix_bcast; | ||
| handle->gather = bootstrap_pmix_gather; | ||
| handle->allgather = bootstrap_pmix_allgather; |
There was a problem hiding this comment.
We only need allgather for now, all other collectives will be using ucc.
| endif() | ||
|
|
||
| if(UCX_BOOTSTRAP_ENABLE_PMIX) | ||
| find_package(PMIX) |
There was a problem hiding this comment.
there is no FindPMIX.cmake, so I am getting the following error
CMake Warning at src/realm/ucx/bootstrap/CMakeLists.txt:77 (find_package):
By not providing "FindPMIX.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "PMIX", but
CMake did not find one.
Could not find a package configuration file provided by "PMIX" with any of
the following names:
PMIXConfig.cmake
pmix-config.cmake
Add the installation prefix of "PMIX" to CMAKE_PREFIX_PATH or set
"PMIX_DIR" to a directory containing one of the above files. If "PMIX"
provides a separate development package or SDK, be sure it has been
installed.
We also need to make the PMIX as required if PMIX is enabled.
There was a problem hiding this comment.
No, please don't make pmix required if enabled. The way we set up dependencies here is, if enabled, the build system will look for the dependency and use it if it finds it. Otherwise it'll disable the feature and not use it. So please, no required.
If you need FindPMIX.cmake, I think pkg-config should find it, so copy what we do with gasnet and hwloc, etc.
There was a problem hiding this comment.
We should update this to follow the style in #434. That is:
find_package(PMIX QUIET)to discover if we have PMIx- use
PMIX_FOUNDas the default foroption(UCX_BOOTSTRAP_ENABLE_PMIX ...) - under
if(UCX_BOOTSTRAP_ENABLE_PMIX)nowfind_package(PMIX REQUIRED)so that if the user explicitly requested, we'll hit an error if it's not found
There was a problem hiding this comment.
Also, looks like the option() call is currently missing so we won't get a cache variable right now.
| #include "realm/ucx/bootstrap/bootstrap_internal.h" | ||
| #include "realm/ucx/bootstrap/bootstrap_loader.h" | ||
|
|
||
| #include <iostream> |
There was a problem hiding this comment.
Why are we including iostream? Doesn't appear to be used.
|
@artempriakhin What's the status on this? |
5 participants
No description provided.