Allow launcher_extra to split quoted values #444

bryevdv · 2022-10-18T17:39:37Z

This PR makes the driver automatically split --launcher-extra values on whitespace internally, Doing so allows for simpler invocations with quoted strings like this:

legate --launcher-extra="-H =g0002,g0003"

rather than having to manually split across multiple --launcher-extra arguments as is currently required:

legate --launcher-extra="-H" --launcher-extra="g0002,g0003"

cc @lightsighter

manopapad · 2022-10-18T20:03:05Z

What if I want to pass to the launcher an argument that contains spaces? Say I want the final result to be something like:

mpirun -f "some dirname with spaces/a.txt" legate-prog.py

bryevdv · 2022-10-18T20:55:20Z

What if I want to pass to the launcher an argument that contains spaces? Say I want the final result to be something like:

Ah, I misunderstood your comment in chat... Yah that's a fly in the ointment. I am sure there are ways to accommodate this possibility but I'm not sure we'd want to inflict them on users. Do any of these sound appealing?

require quoted quotes legate --launcher-extra="-f 'some dir/foo.txt'"
another cmdline opt: legate --launcher-extra="-f" --launcher-extra-nosplit="some dir/foo.txt"
env var WAR? FOO="some dir/foo.txt" then --launcher-extra="-f $FOO"

? that said are we sure that it even works now/before without extra quoting? I would expect

legate --launcher-extra="-f" --launcher-extra="some dir/foo.txt"

to render as

mpirun -f some dir/foo.txt

which would not work?

If extra quoting is already necessary to make things work then we could try to make this PR more careful to not split quoted values

manopapad · 2022-10-18T21:15:54Z

require quoted quotes

Of the alternatives, this is the most palatable. I would be a little weary of writing a "quoted expression" parser from scratch, as there's a bunch of silly edge cases that we would need to handle, e.g.:

--launcher-extra "a b \"c \\" d\" e"

which should produce these final arguments: a, b, c " d , and e.

So it would be great if there's an existing "quoted expression" parser we can use, e.g. something in the standard library, or invoking a sub-shell.

that said are we sure that it even works now/before without extra quoting?

Yes, it works today (in verbose mode you even quote the arguments!)

~/cunumeric> ~/quickstart/run.sh 1 prog.py --dry-run --launcher-extra="-f" --launcher-extra="some dir/f.txt"
Redirecting stdout, stderr and logs to /Users/mpapadakis/Sites/local/2022/10/18/140237
Did not detect a supported cluster, assuming local-node run.
Command: legate --launcher mpirun --numamem 8192 --omps 1 --ompthreads 4 --cpus 1 --sysmem 256 --verbose --log-to-file --nodes 1 --ranks-per-node 1 prog.py --dry-run --launcher-extra=-f --launcher-extra=some dir/f.txt --logdir /Users/mpapadakis/Sites/local/2022/10/18/140237

--- Legion Python Configuration ------------------------------------------------

Legate paths:
  legate_dir       : /Users/mpapadakis/legate.core
  legate_build_dir : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build
  bind_sh_path     : /Users/mpapadakis/legate.core/bind.sh
  legate_lib_path  : /Users/mpapadakis/legate.core/build/lib

Legion paths:
  legion_bin_path       : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/bin
  legion_lib_path       : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/lib
  realm_defines_h       : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/runtime/realm_defines.h
  legion_defines_h      : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/runtime/legion_defines.h
  legion_spy_py         : /Users/mpapadakis/legion/tools/legion_spy.py
  legion_prof_py        : /Users/mpapadakis/legion/tools/legion_prof.py
  legion_python         : /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/bin/legion_python
  legion_module         : /Users/mpapadakis/legion/bindings/python/build/lib
  legion_jupyter_module : /Users/mpapadakis/legion/jupyter_notebook

Command:
  mpirun -n 1 --npernode 1 --bind-to none --mca mpi_warn_on_fork 0 -x CONDA_SHLVL -x CONDA_PROMPT_MODIFIER -x CONDA_EXE -x CONDA_TOOLCHAIN_BUILD -x CONDA_TOOLCHAIN_HOST -x LEGION_DIR -x CONDA_PREFIX_1 -x PATH -x CONDA_PREFIX -x CONDA_PYTHON_EXE -x CONDA_DEFAULT_ENV -x CMAKE_PREFIX_PATH -x PYTHONDONTWRITEBYTECODE -x PYTHONPATH -x NCCL_LAUNCH_MODE -x GASNET_MPI_THREAD -x DYLD_LIBRARY_PATH -x LEGATE_NEED_OPENMP -x LEGATE_MAX_DIM -x LEGATE_MAX_FIELDS -x REALM_BACKTRACE -f 'some dir/f.txt' /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/bin/legion_python -ll:py 1 -lg:local 0 -ll:ocpu 1 -ll:othr 4 -ll:onuma 1 -ll:util 2 -ll:csize 256 -ll:nsize 8192 -level openmp=5 -logfile /Users/mpapadakis/Sites/local/2022/10/18/140237/legate_%.log -lg:eager_alloc_percentage 50 prog.py

bryevdv · 2022-10-18T21:30:43Z

I would be a little weary of writing a "quoted expression" parser from scratch

I would have to be back in undergrad CS to even dream of it :)

I think stdlib shlex.split might do what we want out of the box, though.

bryevdv · 2022-10-18T22:36:56Z

@manopapad see the test in f201fdc I believe that is what we want.

bryevdv · 2022-10-18T22:46:33Z

@manopapad FYI I think the quickstart run.sh must be quoting internally or something?

legate --verbose --dry-run --launcher=mpirun --launcher-extra="-f" --launcher-extra="some dir/f.txt"

yields for me:

... -x REALM_BACKTRACE -f some dir/f.txt /home/bryan/work/legate.core/_skbuild/linu....

bryevdv · 2022-10-18T22:49:07Z

Yikes, despite my test, the actual output is wrong:

legate --verbose --dry-run --launcher=mpirun --launcher-extra="-f 'some dir/f.txt'"

yields

-x REALM_BACKTRACE -f ''"'"'some dir/f.txt'"'"'' /home/bryan/work/legate.co

🙄

bryevdv · 2022-10-18T22:51:58Z

OK, we were already calling shlex.quote near the end, so the posix=False was not necessary and resulted in over-quoting. I think its finally good:

legate --verbose --dry-run --launcher=mpirun --launcher-extra="-f 'some dir/f.txt'" --color

then

... -x REALM_BACKTRACE -f 'some dir/f.txt' /home/bryan/work/legate.core ...

manopapad · 2022-10-19T00:05:58Z

Actually, can we do the same for nsys_extra? It has the exact same constraints as launcher_extra.

Also, can we note on the documentation for these two arguments that we split on whitespace, and the user will need to internally quote if they want to pass an argument that is meant to contain spaces?

manopapad · 2022-10-19T00:09:09Z

Otherwise works great!

~/legate.core> ~/quickstart/run.sh 1 prog.py --dry-run --launcher-extra="a b \"c \\\" d\" e"
Redirecting stdout, stderr and logs to /Users/mpapadakis/Sites/local/2022/10/18/165802
...
Command:
  mpirun -n 1 --npernode 1 --bind-to none --mca mpi_warn_on_fork 0 -x CONDA_SHLVL -x CONDA_PROMPT_MODIFIER -x CONDA_EXE -x CONDA_TOOLCHAIN_BUILD -x CONDA_TOOLCHAIN_HOST -x LEGION_DIR -x CONDA_PREFIX_1 -x PATH -x CONDA_PREFIX -x CONDA_PYTHON_EXE -x CONDA_DEFAULT_ENV -x CMAKE_PREFIX_PATH -x PYTHONDONTWRITEBYTECODE -x PYTHONPATH -x NCCL_LAUNCH_MODE -x GASNET_MPI_THREAD -x DYLD_LIBRARY_PATH -x LEGATE_NEED_OPENMP -x LEGATE_MAX_DIM -x LEGATE_MAX_FIELDS -x REALM_BACKTRACE a b 'c " d' e /Users/mpapadakis/legate.core/_skbuild/macosx--x86_64-3.8/cmake-build/_deps/legion-build/bin/legion_python -ll:py 1 -lg:local 0 -ll:ocpu 1 -ll:othr 4 -ll:onuma 1 -ll:util 2 -ll:csize 256 -ll:nsize 8192 -level openmp=5 -logfile /Users/mpapadakis/Sites/local/2022/10/18/165802/legate_%.log -lg:eager_alloc_percentage 50 prog.py

bryevdv · 2022-10-19T15:27:58Z

@manopapad done in b47b864

Allow launcher_extra to split quoted values

c0a0d17

bryevdv added the category:improvement PR introduces an improvement and will be classified as such in release notes label Oct 18, 2022

bryevdv requested a review from manopapad October 18, 2022 17:39

bryevdv added 2 commits October 18, 2022 15:32

Merge branch 'branch-22.12' into bv/launcher-extra-ux

325b22c

use shlex.split to preserve sub-quotes

f201fdc

avoid over-quoting

25657e5

bryevdv added 2 commits October 18, 2022 19:07

same treatment for nsys_extra

b47b864

docs

765a2ce

manopapad approved these changes Oct 19, 2022

View reviewed changes

bryevdv merged commit 90fb1ee into nv-legate:branch-22.12 Oct 19, 2022

bryevdv deleted the bv/launcher-extra-ux branch October 19, 2022 17:05

manopapad pushed a commit that referenced this pull request Mar 5, 2025

Do not delay label check (#444)

170e114

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow launcher_extra to split quoted values #444

Allow launcher_extra to split quoted values #444

bryevdv commented Oct 18, 2022

manopapad commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

manopapad commented Oct 18, 2022

bryevdv commented Oct 18, 2022

bryevdv commented Oct 18, 2022

bryevdv commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

manopapad commented Oct 19, 2022

manopapad commented Oct 19, 2022

bryevdv commented Oct 19, 2022

Allow launcher_extra to split quoted values #444

Allow launcher_extra to split quoted values #444

Conversation

bryevdv commented Oct 18, 2022

manopapad commented Oct 18, 2022 • edited Loading

bryevdv commented Oct 18, 2022 • edited Loading

manopapad commented Oct 18, 2022

bryevdv commented Oct 18, 2022

bryevdv commented Oct 18, 2022

bryevdv commented Oct 18, 2022 • edited Loading

bryevdv commented Oct 18, 2022 • edited Loading

bryevdv commented Oct 18, 2022 • edited Loading

manopapad commented Oct 19, 2022

manopapad commented Oct 19, 2022

bryevdv commented Oct 19, 2022

manopapad commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading

bryevdv commented Oct 18, 2022 •

edited

Loading