brentyi / tyro

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

Positional arguments by default #64

Closed sander76 closed 1 year ago

sander76 commented 1 year ago

First, thanks for this library. I was looking for an alternative to Typer and ran into this. Just great !

Currently I am playing around with it a bit and have a question:

When using the dataclass approach, is it possible to consider each property which doesn't have a default value as a positional value?

See also the below code:

from dataclasses import dataclass
from typing import Annotated

import tyro
from tyro import conf

@dataclass
class Car:
    """A car class"""

    name: Annotated[str, conf.Positional]  # <-- this seems to be only way to indicate some property is positional.
    """The name of the car."""

    color: str
    """The color of the car."""

if __name__ == "__main__":
    result = tyro.cli(Car)
brentyi commented 1 year ago

Hi, sorry for missing this!

The short answer is not a clean one.

If you want something shorter, you can drop the Annotated[]:

@dataclass
class Car:
    """A car class"""

    name: conf.Positional[str]
    """The name of the car."""

    color: conf.Positional[str]
    """The color of the car."""

if __name__ == "__main__":
    result = tyro.cli(Car)

You can also apply Positional to all arguments, but this will include ones with defaults:

@dataclass
class Car:
    """A car class"""

    name: str
    """The name of the car."""

    color: str
    """The color of the car."""

if __name__ == "__main__":
    result = tyro.cli(conf.Positional[Car])

Marking positional arguments in a function with / could also work:

def main(name: str, color: str, /, [kwargs]) -> None:
    ...

tyro.cli(main)

Or you can move the positional arguments into a separate class:

@dataclass
class Positional:
    """A car class"""

    name: str
    """The name of the car."""

    color: str
    """The color of the car."""

@dataclass
class Args:
    """A car class"""

    positional: Annotated[Positional, conf.arg(name=""), Positional]
    # keyword arguments here

tyro.cli(Args)
sander76 commented 1 year ago

No problem man! Thanks for the elaborate writeup!

These are really valuable examples. I'll have a look which one will fit me best.

Personally I see value in a config flag which you put into the tyro.cli() call which will make all required arguments (the ones without default value in your @dataclass a positional one.

Not sure if that fits the overall design philosophy of tyro, but if you think so, I am willing to invest time in investigating that.

brentyi commented 1 year ago

Definitely interested in what you end up converging to!

For putting an option that automatically marks required arguments as positional: I could be convinced (especially if you have time for a PR!), but would be interested in your thoughts on the solutions above first. The main thing for me is weighing the extra complexity + maintenance effort against the relative usability gain.

sander76 commented 1 year ago

From the examples above I would definitely go for your first example:

@dataclass
class Car:
    """A car class"""

    name: conf.Positional[str]
    """The name of the car."""

    color: conf.Positional[str]
    """The color of the car."""

if __name__ == "__main__":
    result = tyro.cli(Car)

I understand. And playing around with tyro some more I don't really know how I would implement this. My first idea is to have some global setting where you do something like : arg_without_default__set_as_positional. This would fit better with my thinking about a normal function call where, arguments without defaults are positional and with defaults are keyword arguments.

def enter_name(
    name: str,  # <-- positional
    last_name: str|None = None  # <-- keyword

But I don't think that will play nice with the already existing options of setting positional arguments.

More in general I think tyro needs some mind-shift in usage when coming from -for example- ArgParse or Click. By that I mean the handling of subcommands for example, where in Tyro this is kind of hard to do:

git branch list

in Tyro this becomes (if you go for the dataclass approach at least)
git branch cmd:list
brentyi commented 1 year ago

One option is we can follow the current tyro.conf pattern and enable something like tyro.cli(conf.PositionalRequiredArguments[YourArgsStructure]). Implementation would not be too hard, I'm just not sure it's a better enough experience over existing options to warrant existing.

What was your issue with this pattern?

def enter_name(
    name: str,  # <-- positional
    /,  # mark end of positional arguments
    last_name: str | None = None  # <-- keyword
)

Also, to enable this:

git branch list

You can wrap your Union with tyro.conf.OmitSubcommandPrefixes[].

sander76 commented 1 year ago

One option is we can follow the current tyro.conf pattern and enable something like tyro.cli(conf.PositionalRequiredArguments[YourArgsStructure]). Implementation would not be too hard, I'm just not sure it's a better enough experience over existing options to warrant existing.

I guess it's a design choice (of which I only have little knowledge of the context in which it is made). But in my (narrow) context I would have preferred positionals by default for arguments which have no default value.

So my preferred implementation would be a flag to set such behaviour globally, but that might be difficult in combination with the current implementations (?)

What was your issue with this pattern?

def enter_name(
    name: str,  # <-- positional
    /,  # mark end of positional arguments
    last_name: str | None = None  # <-- keyword
)

Nothing wrong with that pattern, but for me the beauty of tyro lies in its usage of dataclasses (or in my case pydantic classes) for cli creation.

Also, to enable this:

git branch list

You can wrap your Union with tyro.conf.OmitSubcommandPrefixes[].

I am sure I tried that but failed. I'll try again and report back.

sander76 commented 1 year ago

I gave it another try and below my implementation (if I understood you correctly).

The tyro.conf.OmitSubcommandPrefixes are for removing prefixes on the arguments of a subcommand. Not on the subcommand itself.

from dataclasses import dataclass
import tyro
from tyro import conf

@dataclass
class List:
    verbose: bool = True

    def main(self) -> None:
        print(self)

@dataclass
class Create:
    name: str

    def main(self) -> None:
        print(self)

@dataclass
class Branch:
    cmd: tyro.conf.OmitSubcommandPrefixes[List | Create]

    def main(self) -> None:
        print(self)
        self.cmd.main()

if __name__ == "__main__":
    result = tyro.cli(Branch)

The subcommand in Branch still needs to be called by cmd:list or cmd:create. But the arguments for -for example- cmd:list don't have the cmd: prefix anymore. (--verbose instead of --cmd:verbose).

brentyi commented 1 year ago

So my preferred implementation would be a flag to set such behaviour globally, but that might be difficult in combination with the current implementations (?)

It seemed quick, so I gave implementing the tyro.cli(conf.PositionalRequiredArgs[YourArgsStructure]) pattern in #66; do you have time to give it a try? The PositionalRequiredArgs configuration "marker" would be applied recursively to all fields inside of YourArgsStructure, and any required arguments would become positional. This seems consistent with what you're describing.

For the prefixing: sorry for my confusion; tyro.conf.subcommand(prefix_name=False) will do what you're asking about (docs). It's unfortunately verbose, but here's a runnable example:

import dataclasses
from dataclasses import dataclass
from typing import Annotated, TypeVar, Union

import tyro
from tyro import conf

T = TypeVar("T")
RemovePrefix = Annotated[T, conf.subcommand(prefix_name=False)]

@dataclass
class List:
    verbose: bool = True

    def main(self) -> None:
        print(self)

@dataclass
class Create:
    name: str

    def main(self) -> None:
        print(self)

@dataclass
class Branch:
    cmd: tyro.conf.OmitSubcommandPrefixes[RemovePrefix[List] | RemovePrefix[Create]]

    def main(self) -> None:
        print(self)
        self.cmd.main()

if __name__ == "__main__":
    result = tyro.cli(Branch)
sander76 commented 1 year ago

I did a quick test on the required as positional. This seems to work ! Thanks for making this work so quickly.

I'll be able to check in more detail, and the subcommand example later this weekend. Thanks a 1000!