-
Notifications
You must be signed in to change notification settings - Fork 75
Fix minor things #1458
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
Fix minor things #1458
Conversation
| #if PRTE_ENABLE_FT | ||
| PMIX_OPTION_DEFINE(PRTE_CLI_ENABLE_RECOVERY, PMIX_ARG_NONE), | ||
| PMIX_OPTION_DEFINE(PRTE_CLI_MAX_RESTARTS, PMIX_ARG_REQD), | ||
| PMIX_OPTION_DEFINE(PRTE_CLI_DISABLE_RECOVERY, PMIX_ARG_NONE), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...what if the default for PRRTE has been set by MCA parameter to enable recovery, but the user doesn't want to do that for this job? Are you sure you want to remove this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was undefined here:
8d735f5#diff-95468cad0a5594de72258b4d462586e17493568348689d12b0b612141a9312daL113
alternatively we could just add this back - oversight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was an error on my part. I'm working right now on how to handle all these runtime controls - many of them are currently set by MCA param (e.g., the abort-on-non-zero-exit behavior), which really isn't right for PRRTE as it is primarily a DVM and not every job it runs will want such behaviors. I'm creating a new --runtime-options CLI for this purpose.
So we have a couple of options here:
- you could restore the definition for now, which is fine with me
- you could decide to add the
--runtime-optionsdirective to your cmd line and include "disablerecover" (or whatever you would like to call it) there. You'd have to wait for me to commit that directive and its supporting code - should be later today or tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I just put it back as it's a compile error.
Signed-off-by: Austen Lauria <[email protected]>
Restore PRTE_CLI_DISABLE_RECOVERY definition for now. Signed-off-by: Austen Lauria <[email protected]>
ac4ea68 to
8e4af4a
Compare
|
bot:ibm:retest |
No description provided.