-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add rank table for locality-aware streaming #1505
Conversation
f1c65ff
to
14d5bf9
Compare
d728f35
to
4918ff0
Compare
test/SerialIOTest.cpp
Outdated
Access::READ_WRITE, | ||
R"({"rank_table": "hostname"})"); |
Check notice
Code scanning / CodeQL
Equality test on floating-point values Note test
4918ff0
to
c474815
Compare
c474815
to
c95e067
Compare
CMakeLists.txt
Outdated
# called surrounding the gethostname() function on Windows | ||
# and it needs to be done at client site since the winsocks API is | ||
# initialized statically per process.... | ||
target_link_libraries(openPMD PUBLIC ws2_32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more portable (MSYS/MinGW, MSVC, LLVM on Windows) if this was searched for via find_library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes, will add this tomorrow when I get my hands on a Windows computer again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
Regarding the ws2_32
lib for gethostname
- there is a more portable way to achieve this with standard MPI calls: can MPI_Get_processor_name
in chapter 9 of the MPI standard:
https://www.mpi-forum.org/docs/mpi-4.0/mpi40-report.pdf
Note that this sometimes appends and sometimes does not append the CPU id as well.
@@ -19,6 +19,7 @@ | |||
* If not, see <http://www.gnu.org/licenses/>. | |||
*/ | |||
#include "openPMD/ChunkInfo.hpp" | |||
#include "openPMD/binding/python/Mpi.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need an openPMD_HAVE_MPI
guard?
Please include after Common.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.../python/Mpi.hpp
has that guard itself, so it's safe to include unguarded.
test/SerialIOTest.cpp
Outdated
#ifdef _WIN32 | ||
WSADATA wsaData; | ||
WSAStartup(MAKEWORD(2, 0), &wsaData); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very scary and would need extensive user documentation (and inline documentation), no? It essentially changes the user API contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably be solved (a bit) by above suggestion of splitting enum class Method{HOSTNAME}
into enum class Method{POSIX_HOSTNAME, WINSOCKS_HOSTNAME, MPI_PROCESSOR_NAME}
, this way making the use of Winsocks explicit. Documentation still needed obviously, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed support for Winsocks-Hostnames entirely, and further renamed the identifiers to posix_hostname
and mpi_processor_name
, so that people know to initialize MPI before using the latter.
Using the MPI call is not ideal either, since MPI is not always available and the call suffers from exactly the same trouble:
This is in practise a lesser trouble since most applications that use MPI also have it initialized, but I think the better solution is to make the used implementation explicit: // include/openPMD/ChunkInfo.hpp
namespace host_info
{
enum class Method
{
HOSTNAME
};
std::string byMethod(Method);
#if openPMD_HAVE_MPI
chunk_assignment::RankMeta byMethodCollective(MPI_Comm, Method);
#endif
std::string hostname();
} // namespace host_info // src/Series.cpp
if (viaJson.value == "hostname")
{
return host_info::hostname();
}
else
{
throw error::WrongAPIUsage(
"[Series] Wrong value for JSON option 'rank_table': '" +
viaJson.value + "'.");
}; The thought behind this is: In some setups, you want writer and reader to match by hostname, in other cases by NUMA node, in other cases by CPU id, you might even want to group multiple hosts into one group. Since the chunk distribution algorithms in #824 use this rank table as a basis for distributing chunks, this keeps that choice flexible for the user. My suggestion hence: Split this into This way, users would explicitly need to state that they want to use the Winsocks implementation, along with all implications that that has. |
5011846
to
ff8c373
Compare
ff8c373
to
8324e8d
Compare
d1dc589
to
edea828
Compare
edea828
to
9b7adab
Compare
Initialize Winsock API
2fd538f
to
9508c50
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
This is the first logical part of #824 which I am now splitting in two separate PRs:
The idea is that a writer code can either explicitly use:
.. or alternatively initialize the Series with JSON/TOML parameter
rank_table
:(The second option is useful as it requires no rewriting of existing code.)
A 2D char dataset is then created, encoding the per-rank information line by line, e.g.:
A reader code can then access this information via:
And compare that information against
unsigned int WrittenChunkInfo::sourceID
as is returned byavailableChunks()
: