brentyi / tyro

CLI interfaces & config objects, from types
https://brentyi.github.io/tyro
MIT License
514 stars 27 forks source link

Allow for both `FlagConversion` and direct override #48

Closed vwxyzjn closed 1 year ago

vwxyzjn commented 1 year ago

Consider the following snippet.

from dataclasses import dataclass

import tyro

@dataclass
class Args:
    flag: bool = False

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

Would it be possible to do the following? tyro.conf.FlagConversionOff[bool] provides the ability to manual override but also disabled the --flag behavior.

python main.py --flag # flag=True
python main.py --flag False # flag=False

Currently it is possible to achieve this behavior with argparse which is the default behavior in CleanRL (https://github.com/vwxyzjn/cleanrl/blob/master/cleanrl/c51.py#L34).

import argparse
from distutils.util import strtobool
parser = argparse.ArgumentParser()
parser.add_argument("--flag", type=lambda x: bool(strtobool(x)), default=False, nargs="?", const=True)
args= parser.parse_args()
print(args.flag)
(openrlbenchmark-py3.9) ➜  openrlbenchmark git:(hnsplots) ✗ python test.py --flag
True
(openrlbenchmark-py3.9) ➜  openrlbenchmark git:(hnsplots) ✗ python test.py --flag False
False
(openrlbenchmark-py3.9) ➜  openrlbenchmark git:(hnsplots) ✗ python test.py --flag True 
True
(openrlbenchmark-py3.9) ➜  openrlbenchmark git:(hnsplots) ✗ python test.py --flag 0   
False
(openrlbenchmark-py3.9) ➜  openrlbenchmark git:(hnsplots) ✗ python test.py --flag 1
True
brentyi commented 1 year ago

This would be straightforward, but I'd prefer to not add a nargs="?" to any existing boolean behavior. I'd expect this to cause issues for downstream dependencies, since setting nargs like this can break positional arguments and/or subcommands.

How strongly do you feel about this? We can certainly add another tyro.conf flag, I'm just concerned about bloat + that it might not be widely used.

vwxyzjn commented 1 year ago

Thanks for the message. I feel there are two separate issues.

1. The current default implementation will break existing scripts if the default value is changed.

That is python tyro_test.py --flag will run fine with the following.

import tyro
def args(flag: bool = False):
    pass
tyro.cli(args)

but will break with

import tyro
def args(flag: bool = True):
    pass
tyro.cli(args)
$ python tyro_test.py --flag
usage: tyro_test.py [-h] [--no-flag]
tyro_test.py: error: unrecognized arguments: --flag

Maybe the current default implementation should at least make --flag and --no-flag available, which is also the default behavior of the latest argparse.

import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--flag', action=argparse.BooleanOptionalAction, default=True)
args= parser.parse_args()
$ python tyro_test.py --help
usage: tyro_test.py [-h] [--flag | --no-flag]

optional arguments:
  -h, --help         show this help message and exit
  --flag, --no-flag

2. More ways to specify flags

This is probably more related to the original issue. Maybe we could add tyro.conf.UniversalFlagConversion[bool] which enables more diverse ways of specifying flags.

--flag
--no-flag
--flag True
--flag true
--flag False
--flag false
--flag 1
--flag 0

While I think tyro.conf.UniversalFlagConversion[bool] is helpful but if it creates too much bloat don't worry about adding it. As long as the first issue is resolved, I can refactor on my end :)

Thanks a lot.

brentyi commented 1 year ago

Ok! I just gave (1) a shot in #50, would appreciate a glance. If it looks good I can do a release with everything merged by the end of today.

For (2) I'm open to a PR but will hold off on implementing it myself; the positional / subcommand-compatibility concerns I had above would make me hesitant to recommend that people use it. (also time constraints etc etc)

vwxyzjn commented 1 year ago

That's very reasonable. Thanks a lot!