Remove SystemExit usage

[mpkocher]

SystemExit inherits from BaseException 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

1. Remove usage of SystemExit in the docs
2. CLI Examples should adhere to the standard style for Pydantic error catching (e.g., ValidationError)

Possible Solutions

1. In Python 3.9, argparse added exit_on_error flag to help solve this friction point with exit being called.
2. Subclass argparse.ArgumentParser and override .exit() or error() to raise the appropriate exception (from status != 0) instead of calling sys.exit.

[hramezani]

Thanks @mpkocher for reporting this issue.

@kschwab Could you please take a look at this issue?

[kschwab]

Yes, I agree with what @mpkocher raised. I'll work to push a fix in the next few days.

[kschwab]

@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.

[kschwab]

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:

1. Standardize CLI parsing errors to raise SettingsError exceptions.
2. Provide a cli_exit_on_error config that defaults to True, just like argparse.

[mpkocher]

@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.

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.

• Overall, the design of Sources in pydantic-settings might have been better to use composition instead of inheritance. However, this ship has sailed.
• If it's going to use inheritance, then perhaps using explicit constructor/init/factory methods of .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.
• Perhaps adding .from_sources(Sources.ENV, Sources.CLI), or defining this at the declarative layer inheriting from BaseSettings.
• argparse is thorny. I would try to hide the argparse implementation details as much as possible. If someone wants to dig/hook into a private method, there isn't anything stopping them. Making this public could dramatically increases the surface area of supported issues and use cases.

[hramezani]

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.

[kschwab]

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.

[mpkocher]

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:

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.

[mpkocher]

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()

More comments and notes on API design choices.

[kschwab]

@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:

• The CLI will always exit, even if error is encountered?
• The CLI will only exit if no error, otherwise raise exception?
• The CLI will never exit and only ever raise exception?

For resolving this issue, we can:

1. Support all three as a config.
2. Support a subset as a config.
3. Support only one (e.g., the current implementation always exits).

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.