edornd / argdantic

Typed command line interfaces with argparse and pydantic
MIT License
38 stars 4 forks source link

Is it possible not to add the top-level name when passing parameters? #24

Closed y805939188 closed 1 year ago

y805939188 commented 1 year ago

Is your feature request related to a problem? Please describe.

Hello.

In my scenario, I aways use cli tool with pydantic such as:

from typing import Set
from pydantic import BaseModel
from argdantic import ArgParser

class Image(BaseModel):
    url: str
    name: str

class Item(BaseModel):
    name: str
    description: str = None
    price: float
    tags: Set[str] = set()
    image: Image = None

cli = ArgParser()
@cli.command()
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

Now, if I pass params to code, I must pass params with top-level name such as:

截屏2023-02-24 22 57 41

But I don't want to pass the tol-level name such as:

python composition/nested_models.py --name test --price 111 --image.url test --image.name test

Can I pass parameters in this way?

Thank you.

edornd commented 1 year ago

Hi Shinn, thanks for giving this tool a try! Strictly speaking, what you asked can be solved by simply declaring the fields of your Item field directly in the command signature, this is the recommended way:

from typing import Set
from pydantic import BaseModel
from argdantic import ArgParser

class Image(BaseModel):
    url: str
    name: str

cli = ArgParser()
@cli.command()
def create_item(name: str, description: str = None, price: float, tags: Set[str] = set(), image: Image = None):
    print(name, description, price, tags, image)

if __name__ == "__main__":
    cli()

This should give you a CLI where your arguments do not include any top-level name.

Internally, argdantic works by simply wrapping every command field into a dynamic pydantic model, so that it always has a single root object. Automatically removing the top-level name is not included at the moment, since it introduces a series of problems: the first one that comes to mind is, how do you deal with a signature like this then fn(name: str, item: Item), where item also has a name field?

Alternatively, you could provide an alias to your fields, but it's less straightforward and in this specific case it might be prone to name conflict, nevertheless it's a valid option:

from typing import Set
from pydantic import BaseModel
from argdantic import ArgParser, ArgField

class Image(BaseModel):
    url: str
    name: str

class Item(BaseModel):
    name: str = ArgField("--name", "-n")
    description: str =  = ArgField("--description", "-d", default=None)
    price: float = ArgField("--price", "-p", default=0.0)
    tags: Set[str] = ArgField("--tags", default=set())
    image: Image = None

cli = ArgParser()
@cli.command()
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

Also, I should note that aliases don't really affect sub-models, since the identifier of the subfields remains the actual field name, unfortunately. I hope this answers to your questions!

y805939188 commented 1 year ago

Hi Shinn, thanks for giving this tool a try! Strictly speaking, what you asked can be solved by simply declaring the fields of your Item field directly in the command signature, this is the recommended way:

from typing import Set
from pydantic import BaseModel
from argdantic import ArgParser

class Image(BaseModel):
    url: str
    name: str

cli = ArgParser()
@cli.command()
def create_item(name: str, description: str = None, price: float, tags: Set[str] = set(), image: Image = None):
    print(name, description, price, tags, image)

if __name__ == "__main__":
    cli()

This should give you a CLI where your arguments do not include any top-level name.

Internally, argdantic works by simply wrapping every command field into a dynamic pydantic model, so that it always has a single root object. Automatically removing the top-level name is not included at the moment, since it introduces a series of problems: the first one that comes to mind is, how do you deal with a signature like this then fn(name: str, item: Item), where item also has a name field?

Alternatively, you could provide an alias to your fields, but it's less straightforward and in this specific case it might be prone to name conflict, nevertheless it's a valid option:

from typing import Set
from pydantic import BaseModel
from argdantic import ArgParser, ArgField

class Image(BaseModel):
    url: str
    name: str

class Item(BaseModel):
    name: str = ArgField("--name", "-n")
    description: str =  = ArgField("--description", "-d", default=None)
    price: float = ArgField("--price", "-p", default=0.0)
    tags: Set[str] = ArgField("--tags", default=set())
    image: Image = None

cli = ArgParser()
@cli.command()
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

Also, I should note that aliases don't really affect sub-models, since the identifier of the subfields remains the actual field name, unfortunately. I hope this answers to your questions!

Thank you for your reply~Argdantic is a very useful tool. We'd love to use it in production.

in my scenario, there are a lot of field in my pydantic-model, The following is a real scene of mine:

截屏2023-02-25 22 58 33

Too many field in my "model", there are many scenarios like this, so I may not be able to flatten all the fields into the top-level parameters.

If using the alias, It looks like there is no way to alias nested models:

from pydantic import BaseModel
from argdantic import ArgParser, ArgField
class Image(BaseModel):
    name: str
class Item(BaseModel):
    image: Image = ArgField("--image_alias")

cli = ArgParser()

@cli.command()
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

We're trying to switch from pydantic_cli to argdantic in production, but it might be a bit confusing to existing users due to top-level name. May I ask that is there any other way to bypass the top-level name?

Thank you~

edornd commented 1 year ago

I see, yes it may become a bit cumbersome to flatten the definitions inside the function declaration.

The alias in nested models unfortunately it's a "known issue": I'm not sure what's the right way to address it, or if should be even addressed. In short, every time the CLI building part encounters a nested models, it recursively processes it using the field name as identifier. This is needed to guarantee a sort of uniqueness, since python/pydantic already take care of it. On top of that, the nested model does not provide CLI fields on its own, therefore assigning an alias to it has no effect whatsoever at the moment. What currently works right now is assigning an alias to the subfields directly, but I completely understand that it's not optimal and a bit counterintuitive, for instance:

from pydantic import BaseModel
from argdantic import ArgParser, ArgField

class Image(BaseModel):
    name: str = ArgField("--imname")

class Item(BaseModel):
    image: Image = None

cli = ArgParser()

@cli.command()
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

outputs something like this:

$ python test.py --help
> usage: test.py [-h] --item.image.name TEXT
>
> optional arguments:
>  -h, --help            show this help message and exit
>  --item.image.name TEXT, --imname TEXT  (required)

Regarding your question, I do not have a working solution right now, but I do have an idea on how it could be implemented: the only strict requirement is that the full CLI building must be driven by a singleton pydantic "root model". This does not mean however that this root model must be generated on the fly: we could have an optional base_model argument, or similarly a wrap_arguments flag that is set to false when the args shouldn't be wrapped around an additional model. Of course, in this case the command definition will only allow one argument, that is the singleton model/setting, i.e. def main(item: Item) is fine, while def main(item: Item, seed: int) won't be possible when wrapping is disabled, but that's reasonable in my opinion. I'm not 100% sure that this will automatically solve the top-level naming issue as well, but we'll see.

I'll try to sketch a proposal and get back to you, or if you want to give it a try, you're absolutely welcome!

edornd commented 1 year ago

@y805939188 I quickly gave it a try, it appears to be working as expected for now. If you're willing to give it a try, you will find the update in the 24-opt-wrapping branch. By including a singleton flag in your command, the top-level name disappears, however the function/command will only accept one argument, and that argument must be a pydantic model (therefore you need to wrap everything you need into your own model manually).

The following code:

from pydantic import BaseModel

from argdantic import ArgField, ArgParser

class Image(BaseModel):
    name: str

class Item(BaseModel):
    name: str
    description: str
    image: Image = None

cli = ArgParser()

@cli.command(singleton=True)
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

will output:

$ python test.py --help
> usage: test.py [-h] --name TEXT --description TEXT --image.name TEXT
>
> optional arguments:
>   -h, --help          show this help message and exit
>   --name TEXT         (required)
>   --description TEXT  (required)
>   --image.name TEXT   (required)
y805939188 commented 1 year ago

@y805939188 I quickly gave it a try, it appears to be working as expected for now. If you're willing to give it a try, you will find the update in the 24-opt-wrapping branch. By including a singleton flag in your command, the top-level name disappears, however the function/command will only accept one argument, and that argument must be a pydantic model (therefore you need to wrap everything you need into your own model manually).

The following code:

from pydantic import BaseModel

from argdantic import ArgField, ArgParser

class Image(BaseModel):
    name: str

class Item(BaseModel):
    name: str
    description: str
    image: Image = None

cli = ArgParser()

@cli.command(singleton=True)
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

will output:

$ python test.py --help
> usage: test.py [-h] --name TEXT --description TEXT --image.name TEXT
>
> optional arguments:
>   -h, --help          show this help message and exit
>   --name TEXT         (required)
>   --description TEXT  (required)
>   --image.name TEXT   (required)

👍 👍 👍 Great! It looks like what i want. I'll try it later and give feedback. Thank you very much~

y805939188 commented 1 year ago

@y805939188 I quickly gave it a try, it appears to be working as expected for now. If you're willing to give it a try, you will find the update in the 24-opt-wrapping branch. By including a singleton flag in your command, the top-level name disappears, however the function/command will only accept one argument, and that argument must be a pydantic model (therefore you need to wrap everything you need into your own model manually).

The following code:

from pydantic import BaseModel

from argdantic import ArgField, ArgParser

class Image(BaseModel):
    name: str

class Item(BaseModel):
    name: str
    description: str
    image: Image = None

cli = ArgParser()

@cli.command(singleton=True)
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

will output:

$ python test.py --help
> usage: test.py [-h] --name TEXT --description TEXT --image.name TEXT
>
> optional arguments:
>   -h, --help          show this help message and exit
>   --name TEXT         (required)
>   --description TEXT  (required)
>   --image.name TEXT   (required)

Hi, I tried the code of this "24-opt-wrapping" branch and it works well. But there is a bit issue where I found that some types like datetime.date will cause an error if they are placed in List type. such as:

from typing import List
from datetime import date
from pydantic import BaseModel
from argdantic import ArgParser

class Test(BaseModel):
    test: List[date]

class Image(BaseModel):
    name: str

class Item(BaseModel):
    name: str
    description: str
    image: Image = None
    test: Test

cli = ArgParser()

@cli.command(singleton=True)
def create_item(item: Item):
    print(item)

if __name__ == "__main__":
    cli()

I made some modifications to the code(https://github.com/edornd/argdantic/pull/26). But I'm not sure if this is the correct way to fix it. If you have time, could you please take a look and help me out? Thank you.

edornd commented 1 year ago

Thank you very much @y805939188 for the tests, and the bugfix! I never attempted this combination of arguments 😄 Your bug fix proposal is great, I believe that's the correct place to solve it. I just added some minor changes to refactor it using the dict.get function to shorten the line a bit, and a condition to use the str as fallback for non-primitive types.

This fix should be enough to make it work, however I will open a new issue about this, the type handling in general is not really amazing for more complex types: dates are one of them, where I'd like pydantic to take care of the parsing, while argparse to print something more meaningful than TEXT as metavar.

Anyway, thanks again, if you're willing to test/check #25, I'll merge on main asap!

y805939188 commented 1 year ago

Thank you very much @y805939188 for the tests, and the bugfix! I never attempted this combination of arguments 😄 Your bug fix proposal is great, I believe that's the correct place to solve it. I just added some minor changes to refactor it using the dict.get function to shorten the line a bit, and a condition to use the str as fallback for non-primitive types.

This fix should be enough to make it work, however I will open a new issue about this, the type handling in general is not really amazing for more complex types: dates are one of them, where I'd like pydantic to take care of the parsing, while argparse to print something more meaningful than TEXT as metavar.

Anyway, thanks again, if you're willing to test/check #25, I'll merge on main asap!

👍 👍 👍 I'd love to try. I'll try the new code later, and I'll report back to you in time if there's anything issue. Thank you~

edornd commented 1 year ago

I created a new release anyway, but I'll keep this issue opened just in case :)

y805939188 commented 1 year ago

I created a new release anyway, but I'll keep this issue opened just in case :)

I tried the release and it worked well 👍 👍 👍 . I plan to continue using it in my project, and I'll provide feedback if I encounter any issues. Thank you~

edornd commented 1 year ago

Awesome, thanks! Keep up the good work in your project! Closing this then for the time being 👍