Skip to content

Coll han update file reading 5.0.x #11043

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

Conversation

FlorentGermain-Bull
Copy link
Contributor

@FlorentGermain-Bull FlorentGermain-Bull commented Nov 8, 2022

Rebase of #10828 + #10456 + #10458 + #10983 + #10992 on v5.0.x branch

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Hello! The Git Commit Checker CI bot found a few problems with this PR:

5ae5907: Coll/han Improvements:

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

1a9a3b3: coll/HAN: Don't DQ HAN dynamic @ intra-node subcom...

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

8805074: Coll han: fix allreduce dynamic calling internal h...

  • check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@FlorentGermain-Bull FlorentGermain-Bull force-pushed the coll_han_update_file_reading_5.0.x branch from 5ae5907 to c24541c Compare November 8, 2022 09:38
@awlauria
Copy link
Contributor

awlauria commented Nov 8, 2022

ok to test

@awlauria awlauria requested review from bosilca and devreal November 8, 2022 15:58
Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Can you please include cherry-picks of #10983 and #10992 here since they are fixing issues in these changes?

/*
* Stringifier for topological level
*/
const char* mca_coll_han_topo_lvl_to_str(TOPO_LVL_T topo_lvl)
const char* mca_coll_han_topo_lvl_to_str(TOPO_LVL_T topo_lvl_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize t hat this breaks the common naming scheme. It should be ompi_coll_han_topo_lvl_to_str. Given that this has been merged into main already this (and other occurrences) should be fixed in a separate PR.

FlorentGermain-Bull and others added 5 commits November 15, 2022 08:32
Signed-off-by: Florent Germain <[email protected]>
(cherry picked from commit f242689)
Signed-off-by: George Katevenis <[email protected]>
(cherry picked from commit 705f8ec)
        Allow topological level to be named in configuration file
        Improve algorithm management and choice
        Allow algorithm selection (optional) in configuration file
        Algorithm choice through MCA parameters simplification

Signed-off-by: Florent GERMAIN <[email protected]>
(cherry picked from commit 9245e27)
Fixes CIDs 1516453, 1516452, and 1516451.

Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 9e558f8)
Signed-off-by: Joseph Schuchart <[email protected]>
(cherry picked from commit 3623699)
@FlorentGermain-Bull FlorentGermain-Bull force-pushed the coll_han_update_file_reading_5.0.x branch from c24541c to 425b3eb Compare November 15, 2022 07:53
@FlorentGermain-Bull
Copy link
Contributor Author

FlorentGermain-Bull commented Nov 15, 2022

Can you please include cherry-picks of #10983 and #10992 here since they are fixing issues in these changes?

done + rebase on v5.0.x

@jsquyres
Copy link
Member

bot:ibm:retest

@awlauria awlauria merged commit a17a7d4 into open-mpi:v5.0.x Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants