Union type error messages.

max-hcompany commented 4 weeks ago

Hi, thank you for your library it has been very pleasant to work with (tyro==0.8.4, Py3.10).

I have been struggling with getting union types to feel more natural from the command line. I was wondering if there was anything I could be doing better or how we might be able to improve the error messages to help users fix their flags. Consider the following example:

from dataclasses import dataclass
import tyro

@dataclass 
class OptA:
    lr: float = 3e-4

@dataclass 
class OptB:
    lr: float = 3e-5 
    beta: float = 0.999

@dataclass
class Config:
    name: str
    opt: OptA | OptB
    opt2: OptA | OptB

config = tyro.cli(Config, args=[
    "--name", "test",
    "opt:opt-b",
    "--opt.lr", "3e-6",
    "--opt.beta", "0.5",
    "opt2:opt-a",    
    "--opt2.lr", "0.3",    
]) 
print(config)

This will work correctly and produce the following config Config(name='test', opt=OptB(lr=3e-06, beta=0.5), opt2=OptA(lr=0.3)).

Apologies if I miss name or poorly describe the issue from here, but I will try my best. The pain point comes in with the special flags used to infer type opt:opt-b, and opt2:opt-a. They seem to force the scope of the remaining list of flags until another special flag is used to change the scope again. This means that once one of these special flags are invoked the ordering and placement of all subsequent flags is rigid (whereas, it was previously flexible).

Q1: Is it possible to decouple the type inference from the command line ordering? For example,

config = tyro.cli(Config, args=[
    "--name", "test",
    "opt:opt-b",
    "opt2:opt-a",
    "--opt.lr", "3e-6",
    "--opt.beta", "0.5",
    "--opt2.lr", "0.3",    
]) 
print(config)

This would make the UX less brittle due to needing to follow strong conventions.

Q2: How can we get errors that more readily describe that flag-ordering and these typed scopes are the source of issue? For example, if you run the snippet from Q1 you will receive the error message:

╭─ Unrecognized options ───────────────────────────────────────────╮
│ Unrecognized or misplaced options: --opt.lr --opt.beta           │
│ ──────────────────────────────────────────────────────────────── │
│ Arguments similar to --opt.lr:                                   │
│     --opt2.lr FLOAT                                              │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-b opt2:opt-a --help │
│     --opt.lr FLOAT                                               │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-b --help            │
│     --opt2.lr FLOAT                                              │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-a opt2:opt-a --help │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-b opt2:opt-b --help │
│     --opt.lr FLOAT                                               │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-a --help            │
│     --opt2.lr FLOAT                                              │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-a opt2:opt-b --help │
│ ──────────────────────────────────────────────────────────────── │
│ Arguments similar to --opt.beta:                                 │
│     --opt.beta FLOAT                                             │
│         (default: 0.999)                                         │
│             in ipykernel_launcher.py opt:opt-b --help            │
│     --opt2.beta FLOAT                                            │
│         (default: 0.999)                                         │
│             in ipykernel_launcher.py opt:opt-b opt2:opt-b --help │
│             in ipykernel_launcher.py opt:opt-a opt2:opt-b --help │
│ ──────────────────────────────────────────────────────────────── │
│ For full helptext, run ipykernel_launcher.py --help              │
╰──────────────────────────────────────────────────────────────────╯

ArgumentError: argument {opt:opt-a,opt:opt-b}: invalid choice: '3e-6' (choose from 'opt:opt-a', 'opt:opt-b')

During handling of the above exception, another exception occurred:

SystemExit                                Traceback (most recent call last)
    [... skipping hidden 1 frame]

Cell In[30], line 2
      1 # Missing types.
----> 2 config = tyro.cli(Config, args=[
      3     \"--name\", \"test\",
      4     \"--opt.lr\", \"3e-6\",
      5     \"--opt.beta\", \"0.5\",
      6     \"--opt2.lr\", \"0.3\",    
      7 ]) 
      8 print(config)

...

File ~/miniconda3/envs/env/lib/python3.10/site-packages/IPython/core/ultratb.py:1173, in VerboseTB.structured_traceback(self, etype, evalue, etb, tb_offset, number_of_lines_of_context)
   1164 def structured_traceback(
   1165     self,
   1166     etype: type,
   (...)
   1170     number_of_lines_of_context: int = 5,
   1171 ):
   1172     \"\"\"Return a nice text document describing the traceback.\"\"\"
-> 1173     formatted_exception = self.format_exception_as_a_whole(etype, evalue, etb, number_of_lines_of_context,
   1174                                                            tb_offset)
   1176     colors = self.Colors  # just a shorthand + quicker name lookup
   1177     colorsnormal = colors.Normal  # used a lot

File ~/miniconda3/envs/env/lib/python3.10/site-packages/IPython/core/ultratb.py:1063, in VerboseTB.format_exception_as_a_whole(self, etype, evalue, etb, number_of_lines_of_context, tb_offset)
   1060 assert isinstance(tb_offset, int)
   1061 head = self.prepare_header(str(etype), self.long_header)
   1062 records = (
-> 1063     self.get_records(etb, number_of_lines_of_context, tb_offset) if etb else []
   1064 )
   1066 frames = []
   1067 skipped = 0

File ~/miniconda3/envs/env/lib/python3.10/site-packages/IPython/core/ultratb.py:1131, in VerboseTB.get_records(self, etb, number_of_lines_of_context, tb_offset)
   1129 while cf is not None:
   1130     try:
-> 1131         mod = inspect.getmodule(cf.tb_frame)
   1132         if mod is not None:
   1133             mod_name = mod.__name__

AttributeError: 'tuple' object has no attribute 'tb_frame'"
}

First of all, these boxed help windows are beautiful. Huge fan.

Zooming on on the errors specifically we have 'tuple' object has no attribute 'tb_frame'" and Unrecognized or misplaced options: --opt.lr --opt.beta. Here the world misplaced is doing a lot of heavy lifting in terms of abstracting away the problem and the solution. To the user, all of the flags are valid, so what does misplaced mean? Is it possible to provide the user feedback that they're scoped in to opt2:opt-a at this time and that only those flags are valid at this time and to shift scope to opt to specify those flags?

In the case that Q1 can be implemented then this would also be not an issue. :)

Q3: Is it possible to warn the user that they've not specified the type being used from the union? For example,

# Missing types.
config = tyro.cli(Config, args=[
    "--name", "test",
    "--opt.lr", "3e-6",
    "--opt.beta", "0.5",
    "--opt2.lr", "0.3",    
]) 
print(config)

Will raise the following error:

╭─ Unrecognized options ───────────────────────────────────────────╮
│ Unrecognized or misplaced options: --opt.lr                      │
│ ──────────────────────────────────────────────────────────────── │
│ Perhaps you meant:                                               │
│     --opt.lr FLOAT                                               │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-a --help            │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-b --help            │
│     --opt2.lr FLOAT                                              │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-a opt2:opt-a --help │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-a opt2:opt-b --help │
│         (default: 0.0003)                                        │
│             in ipykernel_launcher.py opt:opt-b opt2:opt-a --help │
│         (default: 3e-05)                                         │
│             in ipykernel_launcher.py opt:opt-b opt2:opt-b --help │
│ ──────────────────────────────────────────────────────────────── │
│ For full helptext, run ipykernel_launcher.py --help              │
╰──────────────────────────────────────────────────────────────────╯

# Stack trace is essentially the same as the previous example, so omitted here.

I would prefer it warn the user that opt has not had its type resolved between opt-a and opt-b. I think it may be frustrating to the user to see that the suggested flag that is defined is exactly the flag upon which an error is being raised. It might be useful to include in the suggestion that these flags must be scoped? (Or in the case of Q1 again, non-issue).

I think this is lightly related to #60 and #61, but is a unique issue. Let me know if I misunderstood those issues.

Thanks again for this wonderful library. I hope these questions can further the UX so that I can convince more and more people to adopt tyro. :)

brentyi commented 4 weeks ago

Hi @max-hcompany,

Thanks for the detailed issue + code samples!

To reiterate, your three questions are: (Q1) how to make the CLI more robust to argument ordering, (Q2) whether we can make argument placement error messages more descriptive, (Q3) whether we can show subcommand errors before unrecognized argument errors.

I can start with recommendations for making life easier in general when working with subcommands for configuring research code. I think our default behaviors are generally correct given practical limitations (inherited from building on argparse) and subcommand conventions (eg, git commit -a is different from git -a commit) but I do recognize the UX difficulties and enormous room for improvement...

ConsolidateSubcommandArgs

For addressing Q1, one option is tyro.conf.ConsolidateSubcommandArgs. This will push your arguments all to the end of the subcommand list, which is generally less brittle:

config = tyro.cli(
    Config,
    config=(tyro.conf.ConsolidateSubcommandArgs,),
    args=[
        "opt:opt-b",
        "opt2:opt-a",
        # After here, order no longer matters:
        "--opt2.lr", "0.3",
        "--opt.lr", "3e-6",
        "--opt.beta", "0.5",
        "--name", "test",
    ],
)

The config= syntax requires version 0.8.7, which I only just released[^1].

"Base configs"

tyro 0.8.8 contains an additional helper under tyro.extras. This is the pattern I use for most of my own projects, particularly when collaborating with other people who I don't want to put through the trouble of understanding layers of subcommands:

default_configs = {
    "ab": (
        "Set opt to OptA and opt2 to OptB.",
        Config(name=tyro.MISSING, opt=OptA(), opt2=OptB()),
    ),
    "ba": (
        "Set opt to OptB and opt2 to OptA.",
        Config(name=tyro.MISSING, opt=OptB(), opt2=OptA()),
    ),
}
config = tyro.extras.overridable_config_cli(default_configs)

This will flatten everything into only a single layer of subcommands: all usage will look like python script.py {ab,ba} [--options]. We lose some expressivity, but the simplicity tradeoffs related to your Q1 concern are usually worth it to me. There's a live example in our gsplat repo here; if you want more customizability the helper implementation is only a few lines of code (source).

[^1]: For older versions of tyro you can do either tyro.cli(tyro.conf.configure(ConsolidateSubcommandArgs)(Config)) or tyro.cli(tyro.conf.ConsolidateSubcommandArgs[Config]).

Error message improvements

If you go with either of the options above hopefully the error message improvements become less critical. But I just made a commit that improves this situation based on your Q2 suggestion, that I hope to include in a release once I've tested more:

Q3 is inherited from argparse and harder to address. The longer term fix here that I'd like to make is to move away from building on top of argparse entirely (similar to Cappa which I think is great), but this is fairly involved and the benefits are limited.

Please let me know if I missed anything / anything's unclear!

max-hcompany commented 4 weeks ago

This is incredibly helpful. Thank you so much for taking the time to reply!

I think the newer solutions you've suggested will help a ton with immediate usability, and the error messages you've provided definitely look at the step in the right direction.

brentyi / tyro