Skip to content

Coll han: fix allreduce dynamic calling internal han algo on sub_comm #10456

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Coll han: fix allreduce dynamic calling internal han algo on sub_comm #10456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ompi/mca/coll/han/coll_han_dynamic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ mca_coll_han_allreduce_intra_dynamic(const void *sbuf,
* They points to the collective to use, according to the dynamic rules
* Selector's job is done, call the collective
*/
allreduce = mca_coll_han_allreduce_intra;
allreduce = sub_module->coll_allreduce;
}
return allreduce(sbuf, rbuf, count, dtype,
op, comm, sub_module);
Expand Down
2 changes: 1 addition & 1 deletion ompi/mca/coll/han/coll_han_subcomms.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? IIRC, the intention is to disable HAN in the sub communicators, not enable it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My descriptions in #10458 might help

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@gkatev gkatev Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw if you attempt to test all this I believe you will need a fix for #10335

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that this is in mca_coll_han_comm_create_new (a terrible name, btw). I guess that's fine then

opal_info_set(&comm_info, "ompi_comm_coll_han_topo_level", "INTRA_NODE");
ompi_comm_split_type(comm, MPI_COMM_TYPE_SHARED, 0,
&comm_info, low_comm);
Expand Down