Hydra: Add parsing of MPIEXEC_PREFIX_DEFAULT #5135
Conversation
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The internal function should be declared as static and you don't need a namespace for static functions.
tkoehring
force-pushed
the
issue_4388
branch
4 times, most recently
from
c17b5f9 to
66672c2
Compare
src/pm/hydra/ui/mpich/utils.c
Outdated
| { | ||
| HYD_status status = HYD_SUCCESS; | ||
| int val; | ||
| char *arg = "MPIEXEC_PREFIX_DEFAULT"; |
src/pm/hydra/ui/mpich/utils.c
Outdated
| if (MPL_env2int("MPIEXEC_PREFIX_DEFAULT", &val)) { | ||
| if (val > 0) { | ||
| //prepend_rank_fn("prepend-rank", NULL); | ||
| status = HYDU_set_str(arg, &HYD_ui_info.prepend_pattern, "[%r] "); |
There was a problem hiding this comment.
Do HYDU_ERR_POP(status, ...), and add fn_fail, fn_exit labels. This way in the future when we add addition environment variable, one wouldn't easily overwritten the status return.
There was a problem hiding this comment.
When later during command-line parsing, and if additional -l is given and we set the string twice, does it result in memory leak? Please double check.
tkoehring
force-pushed
the
issue_4388
branch
3 times, most recently
from
502671a to
8370aeb
Compare
|
@travisk1154 Looks good. Could you swap the order of the commits? That way we keep each commits to be able to compile clean. Also, did you manually test and confirm the user issue is fixed? |
|
test: mpich/ch4/ofi |
|
Yea I have manually tested things. Setting |
This is not ideal |
HYDU_set_str was updated to take a const char* instead of a char*. No where in the function is the char* value modified and this now allows string literals to be passed. char* will be cast in to const char* and there should be no issue with this change.
There was no handling of the environment variable MPIEXEC_PREFIX_DEFAULT in the hydra code base. The variable adds the rank as a prefix to stdout in the form [%r] by settings its value to anything greated than zero. A new function, HYD_check_envs, was created to handle this and can be used for the handling of other environment variables in the future. The already existing function, prepend_rank_fn, checks for duplicate settings so using both MPIEXEC_PREFIX_DEFAULT and -prepend-rank will correctly result in an error. These new settings were tested manually and confirm that the changes work as intended.
| if (val >= 1) { | ||
| goto fn_exit; | ||
| } | ||
| } |
There was a problem hiding this comment.
The point of processing environment in check_envs is so we don't check environment again in another place.
|
We decided to remove these environment variables. See #5848. |
Pull Request Description
There was no handling of the environment variable MPIEXEC_PREFIX_DEFAULT in the hydra code base. The variable adds the rank as a prefix to stdout in the form [%r] by settings its value to anything greated than zero. A new function, HYD_check_envs, was created to handle this and can be used for the handling of other environment variables in the future. The already existing function, prepend_rank_fn, is used to add the pattern to stdout. This function checks for duplicate settings so using both MPIEXEC_PREFIX_DEFAULT and -prepend-rank will correctly result in an error. These new settings were tested manually and can
confirm that it works as intended.
Fixes #4388
TODO
Expected Impact
Users will now be able to user environment variable MPIEXEC_PREFIX_DEFAULT
Author Checklist
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits are self-contained and do not do two things at once.
Commit message is of the form:
module: short descriptionCommit message explains what's in the commit.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.