Closed mpkocher closed 1 month ago
Thanks @mpkocher for reporting this issue.
@kschwab Could you please take a look at this issue?
Yes, I agree with what @mpkocher raised. I'll work to push a fix in the next few days.
@mpkocher, I've modified the acceptance criteria one to:
Remove usage of SystemExit in the docs for non-zero exit codes
Otherwise, any application that calls --help
would raise an exception. i.e., this issue only removes exit on error in favor of raising a SettingsError
exception.
Actually, the more I think on this the more I see it as a feature request to add a cli_exit_on_error
flag. Hardcoding exit_on_error=False
isn't ideal either. Therefore, we can:
SettingsError
exceptions.cli_exit_on_error
config that defaults to True
, just like argparse.@mpkocher, I've modified the acceptance criteria one to:
Remove usage of SystemExit in the docs for non-zero exit codes
Otherwise, any application that calls
--help
would raise an exception. i.e., this issue only removes exit on error in favor of raising aSettingsError
exception.
The CLI source has a fundamental difference over other all the other current sources in pydantic-settings
.
There's no reading --version
from an dotenv or YAML file and return "1.2.3" to the console. This isn't a mechanic of the other sources. This new CLI mechanic (--version
, --help
) needs to somehow be handled with the current "empty" constructor (e.g., MySettings()
design . I believe this error handling is also coupled to the design (or the lack thereof design) of the main
calling layer.
Misc thoughts that are perhaps more high level.
pydantic-settings
might have been better to use composition instead of inheritance. However, this ship has sailed. .from_env()
or .from_cli()
and avoid abusing/overriding the default Pydantic style constructor to be empty-able (e.g., MySettings()
) This impacts the expected error modes. settings_customise_sources
is verbose. It would be useful if there was a terse and explicit way to communicate that you only want X source, or sources X, Y. This seems like a very common use case. .from_sources(Sources.ENV, Sources.CLI)
, or defining this at the declarative layer inheriting from BaseSettings. Thanks @mpkocher for sharing your thoughts.
I agree with you that pydantic-settings
has some design issues. probably we can make it better in future.
are you happy with @kschwab implementation to address the SystemExit
issue?
BTW, we are open to changes and PR is welcome to fix any of the mentioned issues.
argparse is thorny. I would try to hide the argparse implementation details as much as possible.
Agreed. My main feedback here is the solution can't be hardcoded, and argparse's exit_on_error
behavior (for this issue) is reasonable.
There's no reading --version from an dotenv or YAML file and return "1.2.3" to the console. This isn't a mechanic of the other sources. This new CLI mechanic (--version, --help) needs to somehow be handled with the current "empty" constructor (e.g., MySettings() design .
Can you help me understand the relation here between this and removing SystemExit
? If anything, I believe it will make things worse. e.g., with "no exit" on --help
below, it results in a ValidationError
since v1
is a required field:
import sys
from pydantic_settings import BaseSettings
class Settings(BaseSettings, cli_parse_args=True):
v1: str
sys.argv = ['example.py', '--help']
Settings()
"""
pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
v1
Field required [type=missing, input_value={}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.7/v/missing
"""
Wouldn't a user just do something like this:
import sys
from typing import NoReturn
from pydantic_settings import BaseSettings
class Settings(BaseSettings, cli_parse_args=True):
version: bool = False
def main(self) -> None | NoReturn:
if self.version:
print(self.get_version())
exit(0)
def get_version(self) -> str:
return '1.2.3' # from dotenv, YAML, etc.
sys.argv=['example.py', '--version=true']
Settings().main()
#> 1.2.3
Or, perhaps more "pydantic", like this:
import sys
from os import environ
from typing import NoReturn
from pydantic_settings import BaseSettings
class Settings(BaseSettings, cli_parse_args=True):
prog_version: str # from any pydantic settings source (e.g., environment)
version: bool = False
def main(self) -> None | NoReturn:
if self.version:
print(self.prog_version)
exit(0)
environ['PROG_VERSION'] = '1.2.3'
sys.argv=['example.py', '--version=true']
Settings().main()
#> 1.2.3
As a side, disregard the --version=true
. We need to add a CliFlag[bool]
notation that allows for marking optional bool fields as flags, such as --version
.
Are you saying you want a chance to intercept the CLI parsed args before they get consolidated into the final object, such that you could check for a --version
flag and exit early?
We could definitely do that, but it would be a new feature (i.e., separate from SystemExit
). I think these are two separate issues.
Can you help me understand the relation here between this and removing
SystemExit
? If anything, I believe it will make things worse. e.g., with "no exit" on--help
below, it results in aValidationError
sincev1
is a required field:
I'm suggesting that parsing the args is tangling up the "app" part and "source" processing layer. There isn't really an "app" part of the current API.
The current "CLI Source" is fundamentally different than other sources and perhaps warrants a different approach. For example, a new thin "CLI App" and "Cmd" abstractions (which you're somewhat hinting at in the examples you've listed above).
For context, I'm trying to port internal tools from pydantic-cli to pydantic-settings
, then trying to write a small migration guide on pydantic-cli, then archive the project. However, I'm running into a bunch of friction points with duplication of layers in each tool because of the lack of an "app" layer. Specifically for CLI tools that are design with subcommands.
Are you saying you want a chance to intercept the CLI parsed args before they get consolidated into the final object, such that you could check for a
--version
flag and exit early?
I don't think there should be a requirement to have a general hook mechanism for --do-action
. However, the basic functionality of --version
(via cli_prog_version
, or similar ) and --help
should be supported.
We could definitely do that, but it would be a new feature (i.e., separate from
SystemExit
). I think these are two separate issues.
I believe the error handling mechanism is tangling these layers together. I'm not sure more configuration is the answer to the problem. There's already too many configuration parameters.
Something perhaps in this direction.
import sys
from pydantic_settings import EnvSource, CliSource, CliApp, Cmd
class AlphaCmd(Cmd, cli_name="alpha"):
# define model inline, but can also use inheritance.
name: str
age: int
def run(self) -> int:
print(f"Mock running {self}")
return 0
class App(CliApp):
sources = (CliSource(parse_none_str=True), EnvSource())
model: AlphaCmd
if __name__ == '__main__':
exit_code = App().run(sys.argv) # useful style for testing
sys.exit(exit_code)
# or
App().run_and_exit()
And Subcommands would be the same:
from pydantic import BaseModel
from pydantic_settings import EnvSource, CliSource, CliApp, Cmd
# You can use inheritance or define the model inline,
# or import it from your lib code. There's no difference.
class Alpha(BaseModel):
name: str
age: int
class AlphaCmd(Alpha, Cmd, cli_name="alpha"):
def run(self) -> int:
return 0
class BetaCmd(Cmd, cli_name="beta"):
color: str
def run(self) -> int:
return 0
class App(CliApp):
sources = (CliSource(cli_name="example-02", version="1.0", parse_none_str=True), EnvSource())
model: AlphaCmd | BetaCmd
if __name__ == '__main__':
App().run_and_exit()
@mpkocher, many of the points raised above are relevant to #335. Can we address those points over there instead?
For resolving this issue, Remove SystemExit usage, I'm specifically focused on what happens when (in the latest example) a user runs App
and passes --version
(correctly) or --verizon
(incorrectly). Is it expected that:
For resolving this issue, we can:
The only assertions I am making is to avoid no 3, and that solution no 1, which includes raising SettingsError
on exit(status=0)
, doesn't make sense.
i.e., can "Remove SystemExit usage" be closed by solution no 2, cli_exit_on_error
flag, or is there something else needed?
As a side, solution 2 was also listed as a possible solution in the opening of this issue:
In Python 3.9, argparse added exit_on_error flag to help solve this friction point with exit being called.
SystemExit
inherits fromBaseException
and should be used very sparingly.https://docs.python.org/3/library/exceptions.html#SystemExit
Currently, the docs and examples are using it in several places.
https://docs.pydantic.dev/latest/concepts/pydantic_settings/#subcommands-and-positional-arguments
I suspect the core issue is the leaking of an argparse implementation detail (which internally calls
sys.exit
).This is also perhaps the source of inconsistencies in exit codes when an error occurs. Sometimes it's 1 and sometimes it's 2.
Acceptance Criteria
ValidationError
)Possible Solutions
argparse
addedexit_on_error
flag to help solve this friction point withexit
being called.argparse.ArgumentParser
and override.exit()
orerror()
to raise the appropriate exception (fromstatus != 0
) instead of callingsys.exit
.