MPI shared memory socket scope - #1072
Conversation
|
Would it be worth considering adding hwloc as a dependency to get the socket info? |
I think it would be worth considering... I think for now we should merge this and see how people like it. If there's a need/desire to add hwloc later, we can. (Maybe as an optional dependency - like use hwloc if detected, otherwise use the linux system calls/numa like the branch does now.) |
|
Update: Hubcast doesn't understand jobs that are allowed to fail. We can either fix the job failure or try to fix within Hubcast somehow.. stay tuned |
You'll need to update branch protections under settings -> branches -> develop and list the CI jobs that are required to pass (instead of ci/gitlab.lc.llnl.gov). |
I don't see where I'd be able to do this in the develop branch's settings... (I'm getting copilot to help me...) |
|
@lc-hubcast approve |
|
To approve syncing this PR, please use the GitHub review comment feature to submit an approval. This ensures approval is tied to a specific commit to avoid the sync of malicious data. |
kab163
left a comment
kab163
left a comment
There was a problem hiding this comment.
@lc-hubcast approve
| #if defined(__linux__) | ||
| namespace { | ||
|
|
||
| bool all_ranks_affinity_maps_to_single_socket(std::string& reason) |
There was a problem hiding this comment.
This function appears to be duplicated a couple of times.
There was a problem hiding this comment.
This still looks duplicated at least twice (once in each test file).
| } | ||
| #endif | ||
|
|
||
| bool affinity_maps_to_single_socket(std::string& reason) |
There was a problem hiding this comment.
Is this wrapper function actually needed?
There was a problem hiding this comment.
Ah, no this can be reworked. I think codex took me a bit too literally here 😵💫
There was a problem hiding this comment.
Without the wrapper, we would call "umpire::resource::affinity_maps_to_single_socket" and with it we call "umpire::affinity_maps_to_single_socket". The point of that wrapper is to make it somewhat easier to call (would be called in the example or test files). But I guess we could also do something else - is there a better way to do this?
There was a problem hiding this comment.
It could also potentially have a shorter name in the wrapper version to make it easier to remember...
There was a problem hiding this comment.
I don't think it really matters all that much. If this is something used directly by clients, then it's probably OK to keep the wrapper. I'm assuming you aren't defining it at the top level because that would introduce circular dependencies?
davidbeckingsale
left a comment
davidbeckingsale
left a comment
There was a problem hiding this comment.
Looks good. See comments. Should the affinity related code be moved to a utility file (e.g. like we have a numa file with utility functions)
| return std::string{buffer, static_cast<std::size_t>(length)}; | ||
| } | ||
|
|
||
| void check_mpi_call(int error_code, const char* call_name) |
There was a problem hiding this comment.
this could be moved to the umpire MPI header and applied to all MPI calls elsewhere.
|
|
||
| constexpr int IGNORE_KEY{0}; | ||
|
|
||
| std::string get_mpi3_resource_error_message(int error_code) |
There was a problem hiding this comment.
Same comment as below.
|
The organization looks a lot better now! |
3 participants
No description provided.