Skip to content
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

Multiple nested optional settings #60

Open
jordanott opened this issue Jul 27, 2023 · 17 comments
Open

Multiple nested optional settings #60

jordanott opened this issue Jul 27, 2023 · 17 comments

Comments

@jordanott
Copy link

I have this pretty simple example with two nested optional settings objects. But when I run and only specify the second object optimizer-settings:optimizer-settings it doesn't work.

# test.py
import tyro
import dataclasses
from typing import Optional

@dataclasses.dataclass(frozen=True)
class OutputHeadSettings:
    number_of_outputs: int = 1

@dataclasses.dataclass(frozen=True)
class OptimizerSettings:
    name: str = "adam"

@dataclasses.dataclass(frozen=True)
class ModelSettings:
    output_head_settings: Optional[OutputHeadSettings] = None

    optimizer_settings: Optional[OptimizerSettings] = None


settings = tyro.cli(ModelSettings)
print(settings)
python optimizer-settings:optimizer-settings

usage: test.py [-h] [{output-head-settings:None,output-head-settings:output-head-settings}]

test.py: error: argument [{output-head-settings:None,output-head-settings:output-head-settings}]: invalid choice: 'optimizer-settings:optimizer-settings' (choose from 'output-head-settings:None', 'output-head-settings:output-head-settings')

I see a similar issue if I run

python test.py output-head-settings:None optimizer-settings:optimizer-settings

usage: test.py [-h] [{output-head-settings:None,output-head-settings:output-head-settings}]
 
test.py: error: unrecognized arguments: optimizer-settings:optimizer-settings
@brentyi
Copy link
Owner

brentyi commented Jul 28, 2023

Thanks for filing this @jordanott!

I just pushed a new release (0.5.5) that addresses a bug here. Do you mind checking if your second command (python test.py output-head-settings:None optimizer-settings:optimizer-settings) works now?

Unfortunately due to positionality limitations of the subcommand mechanisms we're relying on here, however, your first example (python optimizer-settings:optimizer-settings) still won't work. As implemented, the first subcommand will always need to correspond to output_head_settings... so in order to set optimizer_settings, an output-head-settings:{None,output-head-settings} argument needs to be passed in first. If this is too annoying a possible workaround is to drop the Optional[] annotations and instead add a flag to turn on/off features (for example: output_head_enabled: bool).

@jordanott
Copy link
Author

jordanott commented Jul 28, 2023

Thanks for the speedy reply and fix!
I can confirm that this fixes the second example

Unfortunately due to positionality limitations of the subcommand mechanisms we're relying on here

Any specific reason the subcommand mechanism does not respect the default argument and use that?

@brentyi
Copy link
Owner

brentyi commented Jul 28, 2023

Any specific reason the subcommand mechanism does not respect the default argument and use that?

This is one of a handful of remaining things in tyro that I'm not super happy with; it mostly reduces to the limited choices we have when mapping types to argparse functionality, which we use for all of the low-level CLI stuff.

For union types over structures like dataclasses, subparsers make sense because they let us only accept arguments from the chosen types.

The downside is that each parser or subparser can only contain a single set of "child" subparsers. This results in a tree structure that doesn't map exactly to the case where we have multiple Union types. In your case, the tree structure we generate looks like:

# (subcommands have been shortened a bit for brevity)

[root parser]
├─ output-head:None
│  ├─ optimizer:None
│  ├─ optimizer:optimizer
├─ output-head:output-head
│  ├─ optimizer:None
│  ├─ optimizer:optimizer

...where optimizer:* is always actually a subcommand of the output-head:*, and not the root parser. And so we need to pass in output-head:* in order to traverse a step down the tree, where we can then pass in the optimizer:* subcommand.

Of course open to suggestions here. :)

The best I've been able to come up with is to tear out argparse and replace it with an in-house solution — or try to fork argparse to somehow add this functionality — but of course either would take a lot of development energy, while breaking compatibility with argparse-reliant tools (for example, we use shtab for tab completion in the shell).

@wookayin
Copy link

wookayin commented Nov 26, 2023

This is also related to a discussion on the semantics of Union #89 (comment) -- Union always induces subcommand. I also find the current behavior of nested Optional[T] very confusing.

Since Optional[T] is equivalent as Union[T, None], which should NOT be treated like Union, this might be one counterexample against Union always being subcommands (or just special case of Union that would not induce subcommands).

The correct semantics here would be: not creating subcommands, but an "optional" field which resolves to None if no arguments were provided. Is treating Union[T, None] specially a possibility here? Would there be any other corner cases, where Union[T, something...] would not make much sense to create subcommands/subparsers?

@brentyi
Copy link
Owner

brentyi commented Nov 26, 2023

Are you arguing that the annotation { Optional[T] / Union[T, None] / T | None } itself should imply that the default value of a field should be None when no arguments are passed in? If so I don't agree: it doesn't in Python, where def f(x: T | None) is different from def f(x: T | None = None).

I can see why creating a subcommand isn't convenient for all use cases, but don't see why it's necessarily wrong; staying consistent with the semantics of Optional[T] being distinct from defaulting to None was an intentional decision. I think this is also a better long-term one, where the confusing naming of the Optional[T] syntax can be replaced by T | None.

For better or worse it also follows tyro's pattern of architecting for generality over usability; the benefits I see are:

  1. When we have a field like field: Dataclass | None = Dataclass(), the subcommand system allows us to (a) default to the dataclass instance when no arguments are passed in but (b) still allow the user to set the value of the field to None. It's not clear to me how you'd accomplish this with the solution being proposed.
  2. With a subcommand for setting the value of the field to None, the transition from Dataclass1 | None to Dataclass1 | Dataclass2 | None is much more natural! The only way I can think of to do the latter case is to have an extra subcommand for the None type (which could be the default), but for consistency in that case IMO it makes sense to use a subcommand for the former case as well.

@wookayin
Copy link

wookayin commented Nov 27, 2023

Are you arguing that the annotation { Optional[T] / Union[T, None] / T | None } itself should imply that the default value of a field should be None when no arguments are passed in?,

No, I was thinking about the case x: Optional[T] = None (with the default value None) or Optional[T] = T(...). Sorry for not writing down my use case more specifically. In the OP's example we had a similar situation: Optional[OutputHeadSettings] = None.


Sorry for a very long post, I write this very specifically to make things clearer and easier to understand for other people. TL;DR) The current behavior seems a bit buggy, and the behavior of --help could be improved.

I was thinking that we may have two possible design choices on the behavior of nested_arg: Optional[T]:

  • (1) Create a subcommand (this is the current behavior)
  • (2) No subcommand, but create nested (multi-level) args such as --nested_field.nested_arg (this is the current behavior if it were nested_arg: T)

but now I see that creating a subcommand is not necessarily excluding nested arguments. To clarify: please note that when I said Optional[T] should not add a subcommand, I meant more precisely like this should register the nested args (such as --optimizer.algorithm); I actually don't care the (meaningless) optimizer:optimizer subcommand, but this was not appearing in the --help, and with such a subcommand the nested subcommand was not working!.

Examples

Let's discuss a more concrete example with the code (a simplified version of the OP's code).

0. Without Optional, this looks reasonable and everyone seems to agree.

Code
 import dataclasses
 from typing import Optional
 
 import tyro
 
 
 @dataclasses.dataclass(frozen=True)
 class Optimizer:
     algorithm: str = "adam"
     learning_rate: float = 1e-3
 
 @dataclasses.dataclass(frozen=True)
 class Config:
+    optimizer: Optimizer = Optimizer()
 
 config = tyro.cli(Config)
 print(config)
usage: 60.py [-h] [--optimizer.algorithm STR] [--optimizer.learning-rate FLOAT]

╭─ arguments ───────────────────────────────────────╮
│ -h, --help        show this help message and exit │
╰───────────────────────────────────────────────────╯
╭─ optimizer arguments ─────────────────────────────╮
│ --optimizer.algorithm STR                         │
│                   (default: adam)                 │
│ --optimizer.learning-rate FLOAT                   │
│                   (default: 0.001)                │
╰───────────────────────────────────────────────────╯

Let's say we want to set the optimizer to Optimizer(algorithm="SGD", learning_rate=3e-4) via CLI. It works well:

$ python 60.py --help
usage: 60.py [-h] [--optimizer.algorithm STR] [--optimizer.learning-rate FLOAT]

$ python 60.py --optimizer.algorithm SGD --optimizer.learning_rate 3e-4
Config(optimizer=Optimizer(algorithm='SGD', learning_rate=0.0003))

1. nested_arg: Optional[T] = T()

Code
 import dataclasses
 from typing import Optional
 
 import tyro
 
 
 @dataclasses.dataclass(frozen=True)
 class Optimizer:
     algorithm: str = "adam"
     learning_rate: float = 1e-3
 
 @dataclasses.dataclass(frozen=True)
 class Config:
+    optimizer: Optional[Optimizer] = Optimizer()
 
 config = tyro.cli(Config)
 print(config)

Here the semantic should be: Config.optimizer would defaults to a default instance ("adam", 1e-3), but can be overridden by CLI by providing some appropriate flags.

The current behavior is:

usage: 60.py [-h] [{optimizer:optimizer,optimizer:None}]

╭─ arguments ───────────────────────────────────────╮
│ -h, --help        show this help message and exit │
╰───────────────────────────────────────────────────╯
╭─ optional subcommands ────────────────────────────╮
│ (default: optimizer:optimizer)                    │
│ ──────────────────────────────────────            │
│ [{optimizer:optimizer,optimizer:None}]            │
│     optimizer:optimizer                           │
│     optimizer:None                                │
╰───────────────────────────────────────────────────╯

The resulting CLI, specifically the optimizer: prefix, doesn't look quite intuitive.

Unlike [Case 0], this no longer works like a hierarchical config or nested arguments:

$ python 60.py --optimizer.algorithm SGD --optimizer.learning_rate 3e-4
╭─ Unrecognized arguments ───────────────────────────────────╮
│ Unrecognized or misplaced arguments: --optimizer.algorithm │
│ ────────────────────────────────────────────────────────── │
│ Perhaps you meant:                                         │
│     --optimizer.algorithm STR                              │
│         (default: adam)                                    │
│             in 60.py optimizer:optimizer --help            │
│ ────────────────────────────────────────────────────────── │
│ For full helptext, run 60.py --help                        │
╰────────────────────────────────────────────────────────────╯

This might be a bug or unintended behavior. I see two problems here:

  • (1) The end CLI doesn't work. I expect it produces the same output as in Case 0. It would be very intuitive to have the same CLI behavior with as if Optional is not used. Why is this different than without Optional?
  • (2) It feels like --optimizer.algorithm is supposed to be recognized ("Perhaps you meant"), but actually not recognized. In fact, it does not show up in --help.

So what does the optimizer:optimizer command do? I can do either

  • python 60.py optimizer:None => results in Config(optimizer=None), or
  • python 60.py optimizer:optimizer => results in Config(optimizer=Optimizer(algorithm='adam', learning_rate=0.001)). This seems meaningless.
  • python 60.py optimizer:SGD => invalid choice

IMO, a subcommand here for just Optimizer doesn't seem to do any meaningful job and I don't think it's yet possible to override the nested fields by e.g. --optimizer.algorithm, etc.

After a while me playing around it, I find that optimizer:optimizer sets the optimizer "not a None", despite the default value is already given (i.e., optimizer: Optimizer | None = Optimizer()) and then the hierarchical/nested arguments can work:

$ python 60.py optimizer:optimizer --optimizer.algorithm SGD --optimizer.learning_rate 0.9
Config(optimizer=Optimizer(algorithm='SGD', learning_rate=0.9))

2. nested_arg: Optional[T] = None

It's basically the same (what it's supposed to behave like and the current behavior) as in the previous case, except that the default value of the returned config object should have optimizer = None instead of optimizer = (some default setting).

Code
 @dataclasses.dataclass(frozen=True)
 class Config:
+    optimizer: Optional[Optimizer] = None

The current behavior:

  • python 60.py => Config(optimizer=None) OK
  • python 60.py --optimizer.algorithm SGD => Unrecognized or misplaced arguments: --optimizer.algorithm (kinda makes sense, optimizer defaults to None!) §
  • python 60.py optimizer:optimizer --optimizer.algorithm SGD => Config(optimizer=Optimizer(algorithm='SGD', learning_rate=0.001)). This does the job, but I wish the syntax is more concise here, as optimizer:optimizer feels a bit unconvenient.

§) Maybe the use of --optimizer.algorithm might imply a not-a-None optimizer setting shall be used, i.e., turn it on. But it may be quite implicit and confusing ... It'd be even greater if this can be configurable?

3. nested_arg: Optional[T] (no default value)

I'm not sure what this should mean, this feels like the same situation as [3.], but I'll leave it undefined for the time being.

@brentyi
Copy link
Owner

brentyi commented Nov 27, 2023

This might be a bug or unintended behavior. I see two problems here:

  • (1) The end CLI doesn't work. I expect it produces the same output as in Case 0. It would be very intuitive to have the same CLI behavior with as if Optional is not used. Why is this different than without Optional?

We currently generate subcommands/subparsers whenever a type needs to be selected for struct-like/dataclass fields, since they let us generate mutually exclusive groups of arguments.

Does the reasoning for this makes sense for Dataclass1 | Dataclass2, where the groups are: (i) the args of Dataclass1 or (ii) the args of Dataclass2? Putting these arguments all into the parent parser would be difficult and confusing, especially in cases like this where there are two semantically different versions of --x.content:

from dataclasses import dataclass
import tyro

@dataclass
class Dataclass1:
    content: int
    """Integer content."""

@dataclass
class Dataclass2:
    content: str
    """String content."""

def main(x: Dataclass1 | Dataclass2) -> None:
    ...

tyro.cli(main)

Instead, we have access to each variant via python script.py x:dataclass1 --help and python script.py x:dataclass2 --help.

For Optional[T] we are applying the same behavior to Dataclass | None, where the groups are: (i) the args of Dataclass or (ii) the args of the None type, where None happens to have no arguments. With #96 merged we also create subcommands for annotations like Dataclass | int.

I understand there are many downsides here associated with subcommands, particularly things like:

  1. Argument ordering. In the above example, python script.py x:dataclass1 --x.content 5 will work, but python script.py --x.content 5 x:dataclass1 will not (similar to git commit -a being different from git -a commit).
  2. Redundancy. If a default dataclass instance is specified in the code, it will be correctly set to the default subcommand. But to access the arguments for overriding values in the dataclass, the subcommand needs to be explicitly selected; this is why the optimizer:optimizer subcommand is necessary in your case to override values in the optimizer field.

But these tradeoffs were ones we were aware of when making the current design decisions. They're also why options like tyro.conf.AvoidSubcommands[] and tyro.conf.ConsolidateSubcommandArgs[] exist, although these don't exactly solve your specific issues.

  • (2) It feels like --optimizer.algorithm is supposed to be recognized ("Perhaps you meant"), but actually not recognized. In fact, it does not show up in --help.

The current "Perhaps you meant" message should say:

image

the intention is to communicate that the argument could be misplaced, especially because it exists in a different parser. (like git commit --help instead of git --help). The message points out the argument is in 60.py optimizer:optimizer --help, which if we run:

image

If you have suggestions for how to make this message clearer I'd be interested!

So what does the optimizer:optimizer command do? I can do either

  • python 60.py optimizer:None => results in Config(optimizer=None), or
  • python 60.py optimizer:optimizer => results in Config(optimizer=Optimizer(algorithm='adam', learning_rate=0.001)). This seems meaningless.
  • python 60.py optimizer:SGD => invalid choice

To clarify, the optimizer: prefix is referring to the field you have called optimizer in your Config dataclass. The suffix (optimizer or None) refers to the type that is being selected of the available types.

optimizer:optimizer is communicating that you're choosing the Optimizer type for the Config.optimizer field; optimizer:None is communicating that you're choosing the None type for the Config.optimizer field.

IMO, a subcommand here for just Optimizer doesn't seem to do any meaningful job and I don't think it's yet possible to override the nested fields by e.g. --optimizer.algorithm, etc.

After a while me playing around it, I find that optimizer:optimizer sets the optimizer "not a None", despite the default value is already given (i.e., optimizer: Optimizer | None = Optimizer()) and then the hierarchical/nested arguments can work:

$ python 60.py optimizer:optimizer --optimizer.algorithm SGD --optimizer.learning_rate 0.9
Config(optimizer=Optimizer(algorithm='SGD', learning_rate=0.9))

Yes: since the --optimizer.algorithm and --optimizer.learning-rate arguments now belong to a subparser, the subparser needs to be specified before the arguments can be passed in. I agree it's confusing if you aren't expecting a subparser and/or haven't spent a godless number of hours writing a CLI generation library, but this is the intended behavior. In my view it's consistent with how subcommands are handled generally, including in the checkout/commit example we were discussing earlier: https://brentyi.github.io/tyro/examples/02_nesting/02_subcommands/ (the arguments for Checkout can only be passed in after cmd:checkout is selected)

Question: we could make this simpler by not creating subcommands for Dataclass | None types, but then how would we set the value to None in your case (1) where the default value is an instance of Dataclass?

2. nested_arg: Optional[T] = None

It's basically the same (what it's supposed to behave like and the current behavior) as in the previous case, except that the default value of the returned config object should have optimizer = None instead of optimizer = (some default setting).

Code
The current behavior:

  • python 60.py => Config(optimizer=None) OK
  • python 60.py --optimizer.algorithm SGD => Unrecognized or misplaced arguments: --optimizer.algorithm (kinda makes sense, optimizer defaults to None!) §
  • python 60.py optimizer:optimizer --optimizer.algorithm SGD => Config(optimizer=Optimizer(algorithm='SGD', learning_rate=0.001)). This does the job, but I wish the syntax is more concise here, as optimizer:optimizer feels a bit unconvenient.

Okay, I'm glad this sort of makes sense. I agree that the syntax is inconvenient and verbose, but the decision to make this the default behavior still makes sense to me. Particularly when we have multiple unions:

@dataclasses.dataclass(frozen=True)
class Config:
    optimizer1: Optional[Optimizer] = Optimizer()
    optimizer2: Optional[Optimizer] = Optimizer()

we would create the subcommands {optimizer1:optimizer, optimizer1:None} and {optimizer2:optimizer, optimizer2:None}, where the specificity is important.

It can always be shortened with tyro.conf.subcommand() and tyro.conf.OmitSubcommandPrefixes. (although these obviously have their own tradeoffs too)

3. nested_arg: Optional[T] (no default value)

I'm not sure what this should mean, this feels like the same situation as [3.], but I'll leave it undefined for the time being.

One nice thing about the current subcommand behavior is that this case is well-defined and consistent with how we handle [1.] and [2.]. If there's no default value, either optimizer:None or optimizer:optimizer need to be explicitly selected.

@kschwab
Copy link

kschwab commented Jan 2, 2024

Hi @brentyi,

This library is fantastic. We are planning on using tyro in conjunction with pydantic settings for additional environment support and validation. They are a perfect fit 😄 Outside of the discussion on Optional[T] as subcommands, everything is very intuitive and pleasant to work with.

Resolving the below question would be a large value add to the library:

Question: we could make this simpler by not creating subcommands for Dataclass | None types, but then how would we set the value to None in your case (1) where the default value is an instance of Dataclass?

For Optional[T] only, may we do one of the below (ranked in order of preference):

  1. --optimizer=None
  2. --optimizer:None
  3. --no_optimizer

I completely agree with subcommands for unions that include two or more types that are not None (e.g. as in git Union[Checkout, Commit]). In this case, the parser needs to know if it's parsing Checkout or Commit for the relevant sub-fields, mutual exclusion, etc.

However, for an Optional[T], it's more equivalent to a bool; either the object is a valid instance of T or it is None. i.e., the only purpose of None is in expressing invalid T. None is not passed in to provide alternative sub-fields or sub-commands as in the git example.

My guess is, the challenge for implementing a solution likely relates to how exclusivity and subparsers are handled. I suspect if we keep the focus narrowed to just Optional[T] and the question that was raised we can hopefully sidestep some of that cruft.

I would be more than happy to jump in and help if pointed in the right direction 👍🏾

@brentyi
Copy link
Owner

brentyi commented Jan 3, 2024

Thanks for your thoughts + offering to help @kschwab! I'd certainly appreciate any efforts to improve the ergonomics here. If you want to get started, I'm happy to provide feedback if you can iron out more behavior details + maybe even unit tests.

Some misc things I'm wondering:

@dataclass
class Optimizer:
    lr: float = 1e-4

def main(optimizer: Optimizer | None) -> None:
    ...

if __name__ == "__main__":
    tyro.cli(main)
  • --optimizer None --optimizer.lr 1e-4 should raise an error, right?
  • Is None the only possible value for the --optimizer argument in your --optimizer=None proposal?
  • If so: the implication is that optimizer is non-None whenever a value contained in it is set, right? If we have no default value for optimizer: Optimizer | None, is the only way to set optimizer to a non-None value to pass in a value for --optimizer.lr? I guess this means that the default value above (1e-4) is effectively ignored?
  • How do we handle directly passing in the optional type, like tyro.cli(Optimizer | None)? Perhaps --optimizer would be replaced with a positional argument?
  • What happens when we add more items to the union, like Optimizer | Checkout | Commit | None?

If you want to take a stab at implementing some changes, I'd recommend breaking things down into two main steps:

  1. Getting the internal argparse parser populated the way you want it. This can be checked by passing in --help to your script.

    • _parsers.py is a reasonable place to start. Here's where we decide whether a specific type annotation should be treated as a single argument, a set of subcommands (SubparsersSpecification), or a nested structure (ParserSpecification). What you probably want is for Dataclass | None types to be handled as a specialized variant of ParserSpecification instead of as SubparsersSpecification.
  2. After the parser is set up correctly, we need to take the arguments that were passed in and call some constructor to make an instance of the annotated type.

    • _calling.py is where the bulk of this logic is. Here's where we distinguish between subcommands and nested structures. Again, we now want to handle Dataclass | None as a nested structure instead of subcommands.

@kschwab
Copy link

kschwab commented Jan 3, 2024

Awesome, thanks @brentyi. Yes, I will work to pick this up after getting your thoughts on best direction.

--optimizer None --optimizer.lr 1e-4 should raise an error, right?

Correct. I believe that would be the most intuitive.

Is None the only possible value for the --optimizer argument in your --optimizer=None proposal?

This was the main distinction between option 1 and the others. If we go with --optimizer=, it puts us on similar footing with how pydantic settings handles environment variables. e.g., this in theory could be allowed:

--optimizer='{"lr": 1e-5}' where optimizer.lr would be overridden to 1e-5

Also, just as with pydantic settings, any nested flags would then take precedence. So a larger example could look like:

--optimizer='{"lr": 1e-5}' --optimizer.lr=1e-6 where optimizer.lr would be overridden to 1e-6

However, in pydantic you have the full backing of the library to sort out the details and validate the JSON string. Since tyro is general, I'm not sure if we'll run into trouble or how much of a hassle it would be (hence the other two options that limit it to more of a flag). Alternatively, we could also take the approach of your next question and basically limit it to None, but I feel like that could be confusing.

If so: the implication is that optimizer is non-None whenever a value contained in it is set, right? If we have no default value for optimizer: Optimizer | None, is the only way to set optimizer to a non-None value to pass in a value for --optimizer.lr? I guess this means that the default value above (1e-4) is effectively ignored?

This is also taking a page out of the pydantic settings book with source prioritization. They apply deep updates on their sets based on the source order. For tyro, ideally this would mean the initial set is sourced from "init settings", i.e. 1e-4, and then can be overridden by CLI arg if passed. Of course, I'm not familiar enough with tyro's internal representation to know if this is possible or even the case, but that would be the general idea.

How do we handle directly passing in the optional type, like tyro.cli(Optimizer | None)? Perhaps --optimizer would be replaced with a positional argument?

Not familiar enough with this one. Could you help provide an example?

What happens when we add more items to the union, like Optimizer | Checkout | Commit | None?

This question actually made me lean more towards options 2 or 3. For sure this should result in a subcommand since we have two or more types that are not None (modified the git command example for our purposes):

 usage: 02_subcommands_mod1.py [-h] [{cmd:checkout,cmd:commit,cmd:None}]

 ╭─ options ─────────────────────────────────────────╮
 │ -h, --help        show this help message and exit │
 ╰───────────────────────────────────────────────────╯
 ╭─ optional subcommands ────────────────────────────╮
 │ (default: None)                                   │
 │ ────────────────────────────────────              │
 │ [{cmd:checkout,cmd:commit,cmd:None}]              │
 │     cmd:checkout  Checkout a branch.              │
 │     cmd:commit    Commit changes.                 │
 │     cmd:None      Disable cmd options             │
 ╰───────────────────────────────────────────────────╯

I personally don't have much of an issue with the above, it's very reasonable. This is why the proposal was only for Type | None like types. The only nit would be consistency, where if we wanted better alignment, we could nudge cmd:None to be more of a flag as opposed to a sub-command:

usage: 02_subcommands_mod2.py [-h] [{cmd:checkout,cmd:commit}]

╭─ options ─────────────────────────────────────────╮
│ -h, --help        show this help message and exit │
╰───────────────────────────────────────────────────╯
╭─ cmd options ─────────────────────────────────────╮
│ --cmd:None        Disable cmd options             │
│                                                   │
├┄ optional subcommands ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┤
│     (default: Disabled)                           │
│     ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄              │
│     cmd:checkout  Checkout a branch.              │
│     cmd:commit    Commit changes.                 │
╰───────────────────────────────────────────────────╯

Which would also blend in nicely for the case we're targeting, e.g. Optional[Checkout]:

usage: 02_subcommands_mod3.py [-h] [--checkout.branch STR]

╭─ options ─────────────────────────────────────────╮
│ -h, --help        show this help message and exit │
╰───────────────────────────────────────────────────╯
╭─ checkout options ────────────────────────────────╮
│ Checkout a branch.                                │
│ ───────────────────────────────────────           │
│ --checkout:None        Disable checkout options   │
│ --checkout.branch STR  (required)                 │
╰───────────────────────────────────────────────────╯

However, I think if we're serious about tackling Optimizer | Checkout | Commit | None we should do this in tandem with #61. I think having independent subcommand groupings would tie all of this together perfectly.

@brentyi
Copy link
Owner

brentyi commented Jan 3, 2024

it puts us on similar footing with how pydantic settings handles environment variables. e.g., this in theory could be allowed:

--optimizer='{"lr": 1e-5}' where optimizer.lr would be overridden to 1e-5

Also, just as with pydantic settings, any nested flags would then take precedence. So a larger example could look like:

--optimizer='{"lr": 1e-5}' --optimizer.lr=1e-6 where optimizer.lr would be overridden to 1e-6

However, in pydantic you have the full backing of the library to sort out the details and validate the JSON string. Since tyro is general, I'm not sure if we'll run into trouble or how much of a hassle it would be (hence the other two options that limit it to more of a flag). Alternatively, we could also take the approach of your next question and basically limit it to None, but I feel like that could be confusing.

Yeah, I think this would be a fairly large hassle. It might be a different situation if we already used JSON somewhere, but this issue (special-casing a specific class of Union types) also doesn't seem like the right place to introduce any specific serialization formats into tyro.

If so: the implication is that optimizer is non-None whenever a value contained in it is set, right? If we have no default value for optimizer: Optimizer | None, is the only way to set optimizer to a non-None value to pass in a value for --optimizer.lr? I guess this means that the default value above (1e-4) is effectively ignored?

This is also taking a page out of the pydantic settings book with source prioritization. They apply deep updates on their sets based on the source order. For tyro, ideally this would mean the initial set is sourced from "init settings", i.e. 1e-4, and then can be overridden by CLI arg if passed. Of course, I'm not familiar enough with tyro's internal representation to know if this is possible or even the case, but that would be the general idea.

Yes, we do have some similar prioritization logic!

To rephrase what's at the root of my questions here though, my assumption is if you run this script with no arguments:

# script.py

@dataclass
class Optimizer:
    lr: float = 1e-4

def main(optimizer: Optimizer | None) -> None:
    ...

if __name__ == "__main__":
    tyro.cli(main)

that it should throw an error, since there's no information about what the default value of optimizer should be (whether it's Optimizer or None).

In this case, what's the mechanism for choosing that you want to set optimizer to the Optimizer type?

How do we handle directly passing in the optional type, like tyro.cli(Optimizer | None)? Perhaps --optimizer would be replaced with a positional argument?

Not familiar enough with this one. Could you help provide an example?

Sure, I just meant quite literally that instead of running:

def main(optimizer: Optimizer | None) -> None:
    ...

if __name__ == "__main__":
    tyro.cli(main)

we should also be able to run just:

if __name__ == "__main__":
    tyro.cli(Optimizer | None)

the same way we can run tyro.cli(Optimizer) or tyro.cli(Checkout | Commit), where the latter will produce checkout/commit subcommands instead of cmd:checkout/cmd:commit. I think this is a solvable problem, it may just require some consideration.

However, I think if we're serious about tackling Optimizer | Checkout | Commit | None we should do this in tandem with #61. I think having independent subcommand groupings would tie all of this together perfectly.

Could you actually elaborate on this? I don't fully follow the relevance of #61.

Thanks again!

@kschwab
Copy link

kschwab commented Jan 3, 2024

@brentyi thanks for the clarification, understood now on the below case:

if __name__ == "__main__":
    tyro.cli(Optimizer | None)

i.e., a "required" Optional[T] (which is contradictory). This seems like a "strict vs lax" mode kind of distinction. We have a required subcommand that must be either Optimizer or None. However, implicitly we would say that not providing Optimizer is None. Meaning that Optional[T] should be treated like Optional[T] = None. However, "strictly speaking", it may also be the case that the user wants None explicitly provided.

I would be in favor of resolving this case with some "is_strict_mode" flag into tyro, with the default being "lax".

In summary, after the discussions above, I would propose the below:

1. field: Optional[T] = T or field: Optional[T] = None

  • Will not result in subcommand.
  • Will use --field:None to pass None type.
  • Will error if --field:None is passed with any other --field flag.

2. field: Optional[T]

  • Will not result in subcommand.
  • Will use --field:None to pass None type.
  • Will error if --field:None is passed with any other --field flag.
  • If "strict" mode is "lax":
    • Will be treated same as field: T | None = None.
  • If "strict" mode is not "lax"
    • Will error if one of --field:None or --field flags are not present.

3. field: Union[Ta | Tb | None] = Ta or field: Union[Ta | Tb | None] = None

  • Will result in subcommand.
  • Will use --field:None to pass None type.
  • Will error if --field:None is passed with any other field:Ta or field:Tb subcommands.

4. field: Union[Ta | Tb | None]

  • Will result in subcommand.
  • Will use --field:None to pass None type.
  • Will error if --field:None is passed with any other field:Ta or field:Tb subcommands.
  • If "strict" mode is "lax":
    • Will be treated same as field: Union[Ta | Tb | None] = None.
  • If "strict" mode is not "lax"
    • Will error if one of --field:None flag or field:Ta, field:Tb subcommands are not present.

5. tyro.cli(T | None)

  • If "strict" mode is "lax":
    • Will not result in subcommand.
    • Will default to None unless T command is provided.
  • If "strict" mode is not "lax"
    • Will result in subcommand.
    • Will create T and None subcommands.

6. tyro.cli(Ta | Tb | None)

  • Will result in subcommand.
  • If "strict" mode is "lax":
    • Will default to None unless Ta or Tb command is provided.
  • If "strict" mode is not "lax"
    • Will create Ta, Tb, and None subcommands.

With respect to #61, I jumped ahead there a little bit. It's not really a blocker. Mostly I was looking at the help generation. Ideally, we would stick the --field:None help text inline with the relevant subcommand group. However, without #61, you can't see all subcommand groups so won't see that flag unless you run help on each subcommand. The alternative is we could place the --field:None flags in the top level options dialogue, but I have a feeling that would look messy. So, #61 would be ideal to have in the sense that the help documentation would be prettier.

@kschwab
Copy link

kschwab commented Jan 3, 2024

Also, with respect to --field:None or field:None in subcommands, it ultimately comes down to which consistency we want to keep. If we prefer consistency of always having the same way to pass None type, we should use --field:None. If we prefer consistency at subcommand interface, we should use --field:None for flags and field:None for subcommands.

I'm not sure I have a strong preference either way. I suppose I lean more towards consistency of passing NoneType, (i.e. --field:None). Mostly because a None subcommand seems irrelevant and is likely more of a corner case. IMO a flag would better serve this purpose.

@brentyi
Copy link
Owner

brentyi commented Jan 4, 2024

Thanks for writing that up! The details were really helpful for understanding what you're thinking.


i.e., a "required" Optional[T] (which is contradictory). This seems like a "strict vs lax" mode kind of distinction. We have a required subcommand that must be either Optimizer or None. However, implicitly we would say that not providing Optimizer is None. Meaning that Optional[T] should be treated like Optional[T] = None. However, "strictly speaking", it may also be the case that the user wants None explicitly provided.

I'd prefer the behavior of keeping is_strict_mode=True as the default. I don't think a required argument which can be None is contradictory here; the overloading of the word "optional" is unfortunate but IMO whether an argument has a default value or not should be orthogonal to whether the type signature says it can be set to None or not.

In terms of consistency this would also match Python semantics and the current tyro behavior, for both subcommand-based behavior of T | None that's being replaced and other annotations like int | None (which will produce an argument that takes either an int or None, with no implication for the default value).

If it would make things easier we could also consider changing --field:None from a boolean flag to taking a value, like some variant of --field:None {True,False}.


On tyro.cli(T | None), tyro.cli(Ta | Tb | None), etc: maybe we can just forget about this for now. My feeling is that the behavior should be the same whether we pass the union type directly or if it appears as an annotation for an argument; it's just a matter of what the prefix for the argument should be.
field: Union[Ta | Tb | None]

For example, if we have:

@dataclass
class Level3:
    ...

@dataclass
class Level2:
    level3: Level3 | None

@dataclass
class Level1:
    level2: Level2

tyro.cli(Level1) should produce an argument like --level2.level3:None, while tyro.cli(Level2) should produce --level3:None, tyro.cli(Level3 | None) should just produce --None.


On the matter of whether None should be a flag (--field:None) or subcommand (field:None, the current behavior), I agree that the flag is better for consistency with what's being proposed.

But unfortunately there are cases where we will still need a subcommand corresponding to None, for example:

@dataclass
class Args:
    a: T1 | T2 | None
    b: T1 | T2 | None

tyro.cli(Args)

The subcommands for field b are currently placed as children of the subcommands of a (see this comment from above). So we'll need to either create a subcommand for a:None to set the values of b (this is the current behavior):

[root parser]
├─ a:None
│  ├─ [b subcommands]
├─ a:T1
│  ├─ [b subcommands]
├─ a:T2
│  ├─ [b subcommands]

or do some flattening:

# I guess this one would require adding another `--b:None` flag to the root parser?
# and the value of this flag would need to be consistent with the subcommand chosen?
[root parser]
├─ a:None,b:T1
├─ a:None,b:T2
├─ a:T1
│  ├─ [b subcommands]
├─ a:T2
│  ├─ [b subcommands]

This example also has me changing my mind on the relevance of #61... but this is challenging to implement and also a mixed bag to me. It would need to be configurable; there are several existing projects running tyro that have on order of ~thousands of arguments spread across many subcommands that we don't want to break, and where being able to run python script.py subcommand1 --help to get focused helptext for just subcommand1 is useful for people.

As one solution for simplifying everything, another option is to narrow the scope of the PR even further. In particular, we could introduce the proposed T | None behavior only in a mode where subcommands are turned off (similar to tyro.conf.AvoidSubcommands).

@kschwab
Copy link

kschwab commented Jan 4, 2024

Thanks for the feedback! Reading the above, we have great alignment.

I'd prefer the behavior of keeping is_strict_mode=True as the default. I don't think a required argument which can be None is contradictory here; the overloading of the word "optional" is unfortunate but IMO whether an argument has a default value or not should be orthogonal to whether the type signature says it can be set to None or not.

Agreed. Point taken, that makes sense.

If it would make things easier we could also consider changing --field:None from a boolean flag to taking a value, like some variant of --field:None {True,False}.

Actually, I had similar thoughts with --field:None {True,False} initially. My only concern was for potential ambiguity at the CLI. e.g., revisiting the earlier help text:

usage: 02_subcommands_mod2.py [-h] [{cmd:checkout,cmd:commit}]

╭─ options ─────────────────────────────────────────╮
│ -h, --help        show this help message and exit │
╰───────────────────────────────────────────────────╯
╭─ cmd options ─────────────────────────────────────╮
│ --cmd:None {True,False}   Disable cmd             │
│                                                   │
├┄ optional subcommands ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┤
│     (default: Disabled)                           │
│     ┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄┄              │
│     cmd:checkout  Checkout a branch.              │
│     cmd:commit    Commit changes.                 │
╰───────────────────────────────────────────────────╯

Obviously, under the hood --cmd:None {True,False} and --cmd:None are the same. However, from a user perspective they now have the option to set --cmd:None=False. What does this mean? Do I have to set this when using a subcommand? Oh, it's optional... but it can only be used when issuing a subcommand. But if I'm using a subcommand, wouldn't this have to be False anyways? So what is this for then? What am I missing? Who am I? Is anything real? etc, etc, etc 😄

Whereas if we just have --cmd:None it aligns with the only use case it's intended for (expressing cmd should be None).

tyro.cli(Level1) should produce an argument like --level2.level3:None, while tyro.cli(Level2) should produce --level3:None, tyro.cli(Level3 | None) should just produce --None.

I think we can actually still include this, the above is perfectly reasonable. I was stopping short of --None because it feels odd as a flag, but I have no strong opinions on this. Everything else listed above was how I thought it should look as well.

... It would need to be configurable; there are several existing projects running tyro that have on order of ~thousands of arguments spread across many subcommands that we don't want to break, and where being able to run python script.py subcommand1 --help to get focused helptext for just subcommand1 is useful for people.

Also completely agree here. Actually, this was a concern of mine as well. Too much movement will break backward compatibility if not configurable. That will be a challenge for #61, but I do think it will be valuable if/when it is resolved.

As one solution for simplifying everything, another option is to narrow the scope of the PR even further. In particular, we could introduce the proposed T | None behavior only in a mode where subcommands are turned off (similar to tyro.conf.AvoidSubcommands).

Agree here as well. What about AvoidNoneSubCommands? I think this is nice because it's kind of like saying:

  • Union[T | None] --> Union[T] --> T
  • Union[Ta | Tb | None] --> Union[Ta | Tb]
  • ... and use our magic flag --None if you need it set to None

Which aligns with everything else we've covered.

Thoughts? If this looks good, I'll start heading in that direction.

@brentyi
Copy link
Owner

brentyi commented Jan 5, 2024

Yes, I think we're pretty aligned!

There are still details that are a bit fuzzy to me, but my experience in tyro has often been that things become much clearer when we start writing test cases and implementing. I still have concerns on things like naming (maybe --field:set-to-none instead of --field:None?) and helptext rendering, but I don't think there's anything that will require major changes to the implementation direction1.

Footnotes

  1. The one exception here is that this entire T | None behavior change could come for free with a more general solution to multiple independent subcommands #61, and this might be the better long-term issue to invest in. But this would probably be a higher-effort change.

@kschwab
Copy link

kschwab commented Jan 5, 2024

Sounds good @brentyi!

Yep I agree, things will definitely get firmer as we get further along 👍🏾

@kschwab kschwab mentioned this issue Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants