Repository: nv-legate/legate.core

Issue:
cunumeric/legate eats the `-l` command line flag
The following program:
```
import cunumeric
import argparse

parser = argparse.ArgumentParser()
parser.add_argument("-l", type=int, default=None)
args, _ = parser.parse_known_args()
print(args.l)
```

run as `legate repro.py -l 5` will print `None`. However, if the `import cunumeric` is removed, then it will print 5. I don't see anywhere in the legate help messages etc why this might happen.

Pinging @bryevdv since you probably know what's up here.

Discussion:
manopapad: Do you cover the case where we have two libraries with single-dash arguments adding their arguments on the same command line? e.g. `legate prog.py -legate:foo -cunumeric:bar`. If each library is only converting its own arguments to long-form before parsing, and assuming legate.core parses arguments first, then it would convert the arguments to `["--legate:foo", "-cunumeric:bar"]`, i.e. the cunumeric args remain in short-form, which presumably could trigger a mis-parse. Is this something that you think could happen?

bryevdv: > i.e. the cunumeric args remain in short-form, which presumably could trigger a mis-parse. Is this something that you think could happen?

The internal parser only has long `--` args with `allow_abbrev=False` so I think anything that is not one of the internal long-args for the current library will pass through unchanged to `extra`. AFAIK single-dash "short" args can only be a problem when they are actually configured on a parser, which we are explicitly avoiding. 
rohany: > @rohany can you check out this PR branch and confirm it fixes the issue for your use case? Seems to for me:

My legate core is slightly diverged from branch-22.12 and i have some things in flight that are making it hard for me to test this. If it works for you locally on that example I think it should be fine.

Patch:
diff --git a/legate/util/args.py b/legate/util/args.py
index e8fdc0c3..88cd7319 100644
--- a/legate/util/args.py
+++ b/legate/util/args.py
@@ -14,6 +14,7 @@
 #
 from __future__ import annotations
 
+import re
 import sys
 import warnings
 from argparse import Action, ArgumentParser, Namespace
@@ -73,6 +74,7 @@ class ArgSpec:
     choices: NotRequired[Sequence[Any]] = Unset
     help: NotRequired[str] = Unset
     metavar: NotRequired[str] = Unset
+    required: NotRequired[bool] = Unset
 
 
 @dataclass(frozen=True)
@@ -80,6 +82,10 @@ class Argument:
     name: str
     spec: ArgSpec
 
+    @property
+    def kwargs(self) -> dict[str, Any]:
+        return dict(entries(self.spec))
+
 
 def entries(obj: Any) -> Iterable[tuple[str, Any]]:
     for f in fields(obj):
@@ -153,25 +159,47 @@ def parse_library_command_args(
         prog=f"<{libname} program>", add_help=False, allow_abbrev=False
     )
 
-    lib_prefix = f"-{libname}:"
+    # Some explanation is in order. Argparse treats arguments with a single
+    # dash differently, e.g. "-xyz" is interpreted as "-x -y -z". This can
+    # cause confusion and clashes when there are multiple single-dash args
+    # with identical prefixes. TLDR; we want "-legate:foo" to behave just
+    # as if it was "--legate:foo". In order to do this, we configure a parser
+    # for "long argumens" and then munge the values in sys.argv to update
+    # any "short prefix" arguments to be "long prefix" arguments first, before
+    # parsing. We also take care to update any output. The alternative here
+    # would be to abandon argparse entirely, and parse sys.argv manually.
+    #
+    # ref: https://github.com/nv-legate/legate.core/issues/415
+
+    short_prefix = f"-{libname}:"
+    long_prefix = f"-{short_prefix}"
 
     argnames = [arg.name for arg in args]
 
     for arg in args:
-        argname = f"{lib_prefix}{arg.name}"
-        kwargs = dict(entries(arg.spec))
-        parser.add_argument(argname, **kwargs)
+        argname = f"{long_prefix}{arg.name}"
+        parser.add_argument(argname, **arg.kwargs)
 
     has_custom_help = "help" in argnames
 
-    if f"{lib_prefix}help" in sys.argv and not has_custom_help:
-        parser.print_help()
+    if f"{short_prefix}help" in sys.argv and not has_custom_help:
+        help_string = parser.format_help()
+
+        # this is a little sloppy but should suffice in practice
+        print(help_string.replace(long_prefix, short_prefix))
+
         sys.exit()
 
+    # convert any short-prefix args to be long-prefix
+    sys.argv = [re.sub(f"^{short_prefix}", long_prefix, x) for x in sys.argv]
+
     args, extra = parser.parse_known_args()
 
+    # put any unconsumed args back they way they were
+    extra = [re.sub(f"^{long_prefix}", short_prefix, x) for x in extra]
+
     for item in extra:
-        if item.startswith(lib_prefix):
+        if item.startswith(short_prefix):
             warnings.warn(
                 f"Unrecognized argument {item!r} for {libname} (passed on as-is)"  # noqa: E501
             )


Test patch:
diff --git a/tests/unit/legate/util/test_args.py b/tests/unit/legate/util/test_args.py
index 02d01a58..83e3e02b 100644
--- a/tests/unit/legate/util/test_args.py
+++ b/tests/unit/legate/util/test_args.py
@@ -91,6 +91,13 @@ class TestArgSpec:
         }
 
 
+class TestArgument:
+    def test_kwargs(self) -> None:
+        arg = m.Argument("arg", m.ArgSpec("dest", default=2, help="help"))
+
+        assert arg.kwargs == dict(m.entries(arg.spec))
+
+
 def test_entries() -> None:
     assert set(m.entries(_TestObj())) == {("a", 10), ("c", "foo")}
 
@@ -115,13 +122,26 @@ class Test_parse_library_command_args:
         self, monkeypatch: pytest.MonkeyPatch, capsys: Capsys
     ) -> None:
         monkeypatch.setattr("sys.argv", ["app", "-foo:help", "-foo:bar"])
-        args = [m.Argument("bar", m.ArgSpec(dest="help"))]
+        args = [m.Argument("bar", m.ArgSpec(dest="bar"))]
         with pytest.raises(SystemExit) as e:
             m.parse_library_command_args("foo", args)
         assert e.value.code is None
         out, err = capsys.readouterr()  # type: ignore[unreachable]
         assert out.startswith("usage: <foo program>")
 
+    def test_default_help_patches_short_args(
+        self, monkeypatch: pytest.MonkeyPatch, capsys: Capsys
+    ) -> None:
+        monkeypatch.setattr("sys.argv", ["app", "-foo:help", "-foo:bar"])
+        args = [m.Argument("bar", m.ArgSpec(dest="bar"))]
+        with pytest.raises(SystemExit) as e:
+            m.parse_library_command_args("foo", args)
+        assert e.value.code is None
+        out, err = capsys.readouterr()  # type: ignore[unreachable]
+        assert out.startswith("usage: <foo program>")
+        assert "-foo:bar" in out
+        assert "--foo:bar" not in out
+
     def test_help_override(
         self, monkeypatch: pytest.MonkeyPatch, capsys: Capsys
     ) -> None:
@@ -182,6 +202,19 @@ class Test_parse_library_command_args:
         assert vars(ns) == {}
         assert sys.argv == ["app", "-foo:bar", "-foo:baz"]
 
+    def test_no_prefix_conflict(
+        self, monkeypatch: pytest.MonkeyPatch, capsys: Capsys
+    ) -> None:
+        monkeypatch.setattr(
+            "sys.argv", ["app", "-foo:bar", "--foo", "-f", "1", "-ff"]
+        )
+        args = [m.Argument("bar", m.ArgSpec(dest="bar"))]
+        ns = m.parse_library_command_args("foo", args)
+        out, err = capsys.readouterr()
+        assert out == ""
+        assert vars(ns) == {"bar": True}
+        assert sys.argv == ["app", "--foo", "-f", "1", "-ff"]
+
 
 if __name__ == "__main__":
     sys.exit(pytest.main(sys.argv))