Skip to content

Make '-x' a PRTE personality CLI option#813

Merged
rhc54 merged 2 commits intoopenpmix:masterfrom
jjhursey:prte-x-cli
Mar 9, 2021
Merged

Make '-x' a PRTE personality CLI option#813
rhc54 merged 2 commits intoopenpmix:masterfrom
jjhursey:prte-x-cli

Conversation

@jjhursey
Copy link
Member

@jjhursey jjhursey commented Mar 9, 2021

@jjhursey jjhursey added this to the v2.0.0 milestone Mar 9, 2021
@jjhursey jjhursey requested a review from rhc54 March 9, 2021 02:42
@jjhursey
Copy link
Member Author

jjhursey commented Mar 9, 2021

shell$ env FOO=bar prterun -x FOO -n 1  env | grep FOO
FOO=bar
shell$ env FOO=bar prterun -x FOO=baz  -n 1  env | grep FOO
FOO=baz
shell$ prte --daemonize
shell$ env FOO=bar prun -x FOO --mca schizo_base_personalities prte -n 1  env | grep FOO
FOO=bar
shell$ env FOO=bar prun -x FOO=baz --mca schizo_base_personalities prte -n 1  env | grep FOO
FOO=baz
shell$ pterm

I did notice the with prun I needed to explicitly set the prte personality for the schizo/prte to be active for that job. Is that expected? Nevermind - that was a bug in my testing

shell$ prte --daemonize
shell$ env FOO=bar prun -x FOO  -n 1  env | grep FOO
shell$ env FOO=bar prun -x FOO --mca schizo_base_personalities prte -n 1  env | grep FOO
FOO=bar
shell$ pterm

@jjhursey
Copy link
Member Author

jjhursey commented Mar 9, 2021

@acolinisi This should fix the problem you were working on in PR #795

@jjhursey jjhursey changed the title Mkae '-x' a PRTE personality CLI option Make '-x' a PRTE personality CLI option Mar 9, 2021
@jjhursey
Copy link
Member Author

jjhursey commented Mar 9, 2021

Some documentation on expected behavior. I'll add something to the man page with this PR, but want to preview it here in case there are any objections.

I was working towards behavior parity with Open MPI v4.1.x. The PRRTE output below represents the current behavior in this branch without having to specify the personality. I'm working on the prun side now.

Open MPI v4.1.x

shell$ mpirun --version
mpirun (Open MPI) 4.1.1rc1

Report bugs to http://www.open-mpi.org/community/help/

Default: Do not forward envars (except for MCA)

shell$ env FOO=bar mpirun -n 1 env | grep FOO

Forward version from environment (warn if not present)

shell$ env FOO=bar mpirun -n 1 -x FOO  env | grep FOO
FOO=bar
shell$ env FOO=bar mpirun -n 1 -x FOO1  env | grep FOO
[c712f6n01:117477] Warning: could not find environment variable "FOO1"

Allow command line to override the environment

shell$ env FOO=bar mpirun -n 1 -x FOO=baz  env | grep FOO
FOO=baz

If multiple '-x' options match the same parameter then the last takes precedence

shell$ env FOO=bar mpirun -n 1 -x FOO=baz  env | grep FOO
FOO=baz
shell$ env FOO=bar mpirun -n 1 -x FOO=baz -x FOO env | grep FOO
FOO=bar
shell$ env FOO=bar mpirun -n 1 -x FOO=baz -x FOO -x FOO=bip env | grep FOO
FOO=bip
shell$ env FOO=bar mpirun -n 1 -x FOO=baz -x FOO -x FOO=bip -x FOO env | grep FOO
FOO=bar

PRRTE master

shell$ prterun --version
prterun (PMIx Reference RunTime Environment) 2.0.0a1

Report bugs to https://github.com/openpmix/prte/

Default: Do not forward envars (except for MCA)

shell$ env FOO=bar prterun -n 1 env | grep FOO

Forward version from environment (warn if not present)

shell$ env FOO=bar prterun -n 1 -x FOO  env | grep FOO
FOO=bar
shell$ env FOO=bar prterun -n 1 -x FOO1  env | grep FOO
--------------------------------------------------------------------------
WARNING: Could not find the environment variable: FOO1
--------------------------------------------------------------------------

Allow command line to override the environment

shell$ env FOO=bar prterun -n 1 -x FOO=baz  env | grep FOO
FOO=baz

If multiple '-x' options match the same parameter then the last takes precedence

shell$ env FOO=bar prterun -n 1 -x FOO=baz  env | grep FOO
FOO=baz
shell$ env FOO=bar prterun -n 1 -x FOO=baz -x FOO env | grep FOO
FOO=bar
shell$ env FOO=bar prterun -n 1 -x FOO=baz -x FOO -x FOO=bip env | grep FOO
FOO=bip
shell$ env FOO=bar prterun -n 1 -x FOO=baz -x FOO -x FOO=bip -x FOO env | grep FOO
FOO=bar

Different rule for MCA parameters

shell$ env PRTE_MCA_FOO=bar prterun -n 1 -x PRTE_MCA_FOO=123 env | grep FOO
--------------------------------------------------------------------------
Error: an MCA parameter was listed more than once on the command line, or
multiple times in one or more files, but with conflicting values:

  Param:  PRTE_MCA_FOO
  Value:  123
  Value:  bar

We cannot determine which value was intended, and therefore are unable to proceed.
Please correct your command line.
--------------------------------------------------------------------------
shell$ prterun -n 1 -x PRTE_MCA_FOO=bip -x PRTE_MCA_FOO=123 env | grep FOO 
--------------------------------------------------------------------------
Error: an MCA parameter was listed more than once on the command line, or
multiple times in one or more files, but with conflicting values:

  Param:  PRTE_MCA_FOO
  Value:  123
  Value:  bip

We cannot determine which value was intended, and therefore are unable to proceed.
Please correct your command line.
--------------------------------------------------------------------------

Note that the below is redundant since we automatically forward MCA parameters

shell$ env PRTE_MCA_FOO=bar prterun -n 1 env | grep FOO
PRTE_MCA_FOO=bar
shell$ env PRTE_MCA_FOO=bar prterun -n 1 -x PRTE_MCA_FOO env | grep FOO 
PRTE_MCA_FOO=bar

 * Fixes openpmix#801

Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>
@jjhursey
Copy link
Member Author

jjhursey commented Mar 9, 2021

This also works correctly with prun

shell$ prte --daemonize
shell$ env FOO=bar prun -n 1 env | grep FOO 
shell$ env FOO=bar prun -n 1 -x FOO env | grep FOO 
FOO=bar
shell$ env FOO=bar prun -n 1 -x FOO=baz env | grep FOO 
FOO=baz
shell$ pterm

Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

This looks fine - thanks for doing it!

prun should not require the prte personality - even though it doesn't call prte_init, it is still a PRRTE tool.

We will probably have to do something about -x in the OMPI personality as there is some cross-talk between it and the --tune option. Can look at that as a separate problem.

This will conflict with my schizo refactor branch (where I fully separate out the command lines), but I'll take care of it once this is merged.

Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>
@jjhursey
Copy link
Member Author

jjhursey commented Mar 9, 2021

I think this is ready now. I just pushed a clarification to the man page - no code changes since Ralph's review.

The prun personality thing was a red herring. I had a bug in the code that I fixed this morning.

@rhc54 rhc54 merged commit 8e9bef7 into openpmix:master Mar 9, 2021
@jjhursey jjhursey deleted the prte-x-cli branch March 9, 2021 22:42
@rhc54
Copy link
Contributor

rhc54 commented Mar 10, 2021

I was afraid of this - the OMPI cmd line parsing wrt envars is a horrid mess, and this change broke the expected behavior for Mellanox:

image

I'll restore the original -x processing in OMPI in my schizo cleanup branch where we completely separate the personalities instead of layering them like we do here.

@jjhursey
Copy link
Member Author

Wow. OMPI v5 is a good time for them to fix that, but that's for them to discuss.

As long as we preserve the -x option for PRRTE's core personality (so we can use it with prun and prterun - and maybe prte) I'm ok with that.

@rhc54
Copy link
Contributor

rhc54 commented Mar 11, 2021

Yeah, I can preserve it. What I'm doing is ensuring that only one schizo component gets to define and parse the cmd line. That way OMPI can do whatever the heck it wants without worries about cross-talk with PRRTE, and vice-versa. It's the only solution I could come up with. I've had the argument about all this envar/tune crud - not worth revisiting. There is too much history already out there that would break if we force the required changes.

@jjhursey
Copy link
Member Author

Ref: #413

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.

Make -x a core prte CLI option

2 participants