Coll han: fix allreduce dynamic calling internal han algo on sub_comm #10456
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
FlorentGermain-Bull
force-pushed
the
coll_han_fix_dynamic_allreduce
branch
from
eeb479e to
97d2e34
Compare
|
bot:ibm:retest |
|
bot:ibm:retest |
|
Can this get a review? |
| @@ -116,7 +116,7 @@ int mca_coll_han_comm_create_new(struct ompi_communicator_t *comm, | |||
| /* | |||
| * This sub-communicator contains the ranks that share my node. | |||
| */ | |||
| opal_info_set(&comm_info, "ompi_comm_coll_preference", "^han"); | |||
| opal_info_set(&comm_info, "ompi_comm_coll_preference", "han"); | |||
There was a problem hiding this comment.
Why this change? IIRC, the intention is to disable HAN in the sub communicators, not enable it.
There was a problem hiding this comment.
In the dynamic path, the decision for which submodule to use on each level is taken at collective-op-time, instead of on comm-split-time, like in the classic path. The topo_lvl key attached to the new comm will let han know that this is a sub-comm and decide accordingly when a collective is called on it
There was a problem hiding this comment.
My descriptions in #10458 might help
There was a problem hiding this comment.
So that fixes the dynamic selection. But doesn't that interfere with the static selection? I remember we put the preference key in in order to prevent han from selecting itself in the subcomm.
There was a problem hiding this comment.
So the two paths exist together. Some methods use the dynamic (mainly the simple ones), through mca_coll_han_comm_create_new, while others use the old/non-dynamic one through mca_coll_han_comm_create. If we exclude HAN on the dynamic path sub-comm, the component with the next higher priority gets chosen, but in this way the submodule to use cannot be tuned
There was a problem hiding this comment.
So no, the two paths do not affect each other. Each one creates different sub-comms, in a slightly different way, and the chosen path is determined by the respective collective primitive implementation. Yes this is a bit akward! (#10438)
There was a problem hiding this comment.
Btw if you attempt to test all this I believe you will need a fix for #10335
There was a problem hiding this comment.
I see now that this is in mca_coll_han_comm_create_new (a terrible name, btw). I guess that's fine then
|
@bosilca ? |
|
@bosilca is this good to go? |
|
@bosilca ping do you agree with the change? |
|
@FlorentGermain-Bull Can you rebase this PR so that it picks up the new CI? Thanks! |
Signed-off-by: Florent Germain <[email protected]>
FlorentGermain-Bull
force-pushed
the
coll_han_fix_dynamic_allreduce
branch
from
97d2e34 to
f242689
Compare
Fix for #8248 (a little bit late).
This PR reactivates the dynamic mechanism and corrects the initial bug.
To make it simple, the dynamic had a bug that caused a call to an internal han implementation on a sub_communicator but with a wrong module, leading to a segfault.