-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
No arguments to CLI subparser fails to raise an error #335
Comments
Thanks @mpkocher for reporting this issue. @kschwab Could you please take a look at this issue? @mpkocher regarding mypy issue, did you enable Pydantic mypy plugin? |
Hey @mpkocher, thanks for the feedback and ticket. You raised a lot of good points.
Currently this is expected since CLI subcommands are treated as optional. However, it should be made configurable to support the use case you are raising. This would actually better align with argparse which makes subcommands optional by default, but has the ability to make them required (added in 3.7).
Yes, I agree with this somewhat as we had a similar approach before landing on the current solution. However, when digging deeper, unless we're wanting to support multiple subcommands, the current solution provides more benefits. There is a longer outline of my thoughts here, but the summary is:
If you have strong opinions otherwise, I would be interested in hearing your thoughts. Lastly, I would say the main desire for a union approach primarily comes down to being able to do what was shown in the examples: class Root(BaseSettings, cli_parse_args=True, cli_prog_name="example"):
command: Alpha | Beta
def runner(m: Model):
match m.command:
case Alpha(): print(f"Running alpha={m.command}")
case Beta(): print(f"Running beta={m.command}") i.e., having the ability to directly reference a "subcommand" from an object. I completely agree with this, and it feeds into your last point I'll cover below. As a side note, the
Yep, I agree 💯 with this, especially when viewing it from a CLI point of view (i.e., use case #2), which I think is the dominate use case for subcommands. My thoughts are to add a simple function that returns the subcommand for a model. Given your feedback, we could additionally add an import sys
from pydantic import BaseModel
from pydantic_settings import (BaseSettings, CliSubCommand)
from pydantic_settings import get_subcommand # proposed CLI subcommand retrieval function (hand-wavy name)
class Alpha(BaseModel):
"""Alpha Utils"""
name: str
def cli_main(self) -> int:
print(f"Running alpha {self}")
return 0
class Beta(BaseModel):
"""Beta Utils"""
age: int
def cli_main(self) -> int:
print(f"Running beta {self}")
return 0
class Root(BaseSettings, cli_parse_args=True, cli_prog_name="example"):
alpha: CliSubCommand[Alpha]
beta: CliSubCommand[Beta]
def cli_main(self) -> int:
print(f"Running root {self}")
return get_subcommand(self, is_required=True).cli_main()
if __name__ == '__main__':
sys.exit(Root().cli_main()) |
Updated subcommand docs can be found here in PR. |
@mpkocher, I reviewed your feedback on including an "app" layer, to which I agree. Overall, my impressions are the import sys
from pydantic import BaseModel, Field
from typing import Optional, NoReturn, Any, override
from pydantic_settings import BaseSettings, CliSubCommand, get_subcommand
from abc import ABC, abstractmethod
class CliApp(BaseSettings, ABC):
def __init__(self) -> None:
...
def _finalize(self, argv: list[str] | bool) -> Any:
super().__init__(
_cli_parse_args=argv,
_env_parse_none_str='null',
# etc.
)
return self
@classmethod
def run(cls, argv: Optional[list[str]] = None, is_exit: bool = True) -> int | NoReturn:
status = cls.main(cls()._finalize(argv if argv else True))
if not is_exit:
return status
sys.exit(status)
@abstractmethod
def main(self) -> int:
...
class Alpha(BaseModel):
name: str
age: int
class AlphaCmd(Alpha):
def run(self) -> int:
print(self.name, self.age)
return 0
class BetaCmd(BaseModel):
color: Optional[str]
def run(self) -> int:
print(self.color)
return 0
class App(CliApp):
alpha: CliSubCommand[AlphaCmd]
beta: CliSubCommand[BetaCmd]
is_get_version: bool = Field(False, alias='version')
def _get_version(self) -> int:
print('1.1.1')
return 0
@override
def main(self) -> int:
if self.is_get_version:
return self._get_version()
return get_subcommand(self).run()
if __name__ == '__main__':
sys.argv = ['app', 'beta', '--color', 'blue']
App().run()
#> blue Customization of sources would still be done by overriding Additionally, the above will handle nested subcommands and top level flags or args. Nested subcommands are accomplished by using the Lastly, if desired, If the above looks good, we can plan on bringing it in after some cleanup and doc updates with this fix. |
Please feel free to open another ticket to update/improve the docs. I can take a pass at that if you want. |
@mpkocher, I've finalized most the implementation addressing your concerns above.
This has now diverged. Instead of run returning
This is an alias use case. I've updated subcommand and positional args to handle alias assignment.
I completely agree with this. It was because nested subcommands weren't of type I've updated the solution to provide everything under class CliCmd(ABC):
def finalize(self, args: list[str] | None = None,
cli_settings_source: CliSettingsSource[Any] | None = None) -> Any:
"""Finalize the CliCmd object by parsing the CLI args."""
...
def run(self, args: list[str] | None = None,
cli_settings_source: CliSettingsSource[Any] | None = None) -> Any:
"""Parse the CliCmd args and run the cli_cmd method."""
...
def get_subcommand(self) -> Any:
"""Get the subcommand model."""
...
def run_subcommand(self, *args: Any, **kwargs: Any) -> Any:
"""Run the subcommand cli_cmd method."""
...
@abstractmethod
def cli_cmd(self, *args: Any, **kwargs: Any) -> Any:
"""The cli command method entry point."""
... Revisiting our example from before, I've updated it with import sys
from pydantic import BaseModel, Field
from typing import Optional, override
from pydantic_settings import BaseSettings, CliCmd, CliSubCommand
class Alpha(BaseModel):
name: str
age: int
class AlphaCmd(Alpha, CliCmd):
@override
def cli_cmd(self) -> int:
print(self.name, self.age)
return 0
class BetaCmd(BaseModel, CliCmd):
color: Optional[str]
@override
def cli_cmd(self) -> int:
print(self.color)
return 0
class App(BaseSettings, CliCmd):
alpha: CliSubCommand[AlphaCmd]
beta: CliSubCommand[BetaCmd] = Field(alias='run-tests')
is_get_version: bool = Field(False, alias='version')
def get_version(self) -> int:
print('1.1.1')
return 0
@override
def cli_cmd(self) -> int:
if self.is_get_version:
return self.get_version()
return self.run_subcommand()
if __name__ == '__main__':
sys.argv = ['app', 'run-tests', '--color', 'blue']
sys.exit(App().run())
#> blue |
For your team or org projects, you can use whatever level of gradual typing that would make sense for those requirements.
Unfortunately, I think I'm running out of gas on this one. I believe you should do whatever it aligned with your requirements and vision for this feature. Perhaps other community members could provide feedback. |
No worries @mpkocher, the design has stabilized since then. I ended up heading in a direction similar to click or typer using a decorator-based approach. IMO, it's an even better fit when paired with the class structure and typing that Pydantic provides; it's very intuitive. It also allows for us to extend the CLI usage to
Using the from pydantic import BaseModel
from pydantic_settings import CliApp
@CliApp.main
class App(BaseModel):
def cli_cmd(self) -> None:
print("Hello World")
App()
"""
Hello World
""" Alternatively, we can use the from pydantic import BaseModel
from pydantic_settings import CliApp
@CliApp.main
class App(BaseModel):
@CliApp.command
def my_cli_cmd(self) -> None:
print("Hello World")
App()
"""
Hello World
""" Adding subcommands, we can run them using import sys
from typing import Optional
from pydantic import BaseModel
from pydantic_settings import CliApp, CliSubCommand
class AlphaCmd(BaseModel):
name: str
age: int
def cli_cmd(self) -> None:
print("AlphaCmd", self.name, self.age)
class BetaCmd(BaseModel):
color: Optional[str]
def cli_cmd(self) -> None:
print("BetaCmd", self.color)
@CliApp.main
class App(BaseModel):
alpha: CliSubCommand[AlphaCmd]
beta: CliSubCommand[BetaCmd]
def cli_cmd(self) -> None:
CliApp.run_subcommand(self)
sys.argv = ["example.py", "beta", "--color=green"]
App()
"""
BetaCmd green
""" Or, we can use CliApp.run(App, cli_args=["beta", "--color=green"])
"""
BetaCmd green
"""
CliApp.run(BetaCmd, cli_args=["--color=green"])
"""
BetaCmd green
""" Furthermore, both import sys
from typing import NoReturn, Optional
from pydantic import BaseModel
from pydantic_settings import CliApp, CliSubCommand
class AlphaCmd(BaseModel):
name: str
age: int
def cli_cmd(self) -> int:
print("AlphaCmd", self.name, self.age)
return 0
class BetaCmd(BaseModel):
color: Optional[str]
def cli_cmd(self) -> int:
print("BetaCmd", self.color)
return 0
@CliApp.main
class App(BaseModel):
alpha: CliSubCommand[AlphaCmd]
beta: CliSubCommand[BetaCmd]
def cli_cmd(self) -> NoReturn:
sys.exit(CliApp.get_subcommand(self).cli_cmd())
sys.argv = ["example.py", "beta", "--color=green"]
App()
"""
BetaCmd green
""" Of course, given @CliApp.main
class App(BaseModel):
subcommand: CliSubCommand[AlphaCmd | BetaCmd]
def cli_cmd(self) -> NoReturn:
sys.exit(self.subcommand.cli_cmd()) However, I would not actively encourage the Lastly, with respect to the last point raised:
I would disagree for several reasons:
Given the above, I don't see the upside for using a |
@hramezani would you like me to proceed forward with adding a CLI app layer (as discussed above), or only resolve this issue with get subcommand? |
@kschwab I couldn't follow the whole discussion. Adding the CLI app layer is a structural change and I would prefer to prevent it for now. let's fix the issue for now. |
PR is ready. I stripped the branch back to the original fix. We can raise the other issues separately. |
With
pydantic-settings
2.3.4, when no arguments are provided to a subparser style CLI, there is no exception raised.I believe a user should not have to explicitly catch for these
None
cases on the "Root"Settings
model.Acceptance criteria:
To demonstrate this issue:
Running would be:
It's possible to workaround the current design by mapping these
Optional
fields ofRoot
to a tuple, then case matching (alternatively, writing withif/elif
).However, it's a bit clunky and not particularly obvious that you need to handle these
None
cases. I believe the structural validation/checking should be occurring at the pydantic-settings level.Misc Feedback
From a type standpoint, the current design is a bit peculiar because it leans on
None
.I believe the it would be useful to use a union type in the design.
Calling a function/main would be:
In general, it seems like there's a core part of the API that is missing the plumbing layer of calling your function with the initialized and validated settings.
There's also bit of friction with using
mypy
.Yields
Perhaps this is general issue with
pydantic-settings
and should be filed as a separate ticket.The text was updated successfully, but these errors were encountered: