Workaround for Typer optional default values with Python calls

[rmitsch]

Goals

Allow calling init_config_cfg() from Python with optional arguments not being set. This currently fails due to the behaviour of Typer for handling optional arguments in Python calls of functions decorated with @init_cli.command. See #10727 for a more in-depth description.

Description

Modified init_config_cli() to force default values for optional arguments if they are still of types typer.models.OptionInfo or typer.models.ArgumentInfo. This seems to be done by Typer automatically on CLI calls, but not on Python calls (see also the corresponding discussion in the Typer repo).

If such a type is detected, the corresponding variable is set to the configured primitive default value.

Types of change

Bug fix/enhancement, new test.

Checklist

• I confirm that I have the right to submit this contribution under the project's MIT license.
• I ran the tests, and all new and existing tests passed.
• My changes don't require a change to the documentation, or if they do, I've added all required information.

[svlandeg]

Thanks for digging into this! I might have a look at the typer side of things later this week, too, but either way it's a good idea to make sure our code base is robust against this type of failure.

[rmitsch]

Comments/suggestions have been addressed.

[svlandeg]

Looking into this in a bit more detail, I'm not sure this is the path we want to take. For one, it makes the implementation of the _cli function quite ugly and error-prone. Eg. having to think about using optional_args instead of the parameters directly -> I could see this leading to errors in the future.

I've started wondering about the original issue as well - I'm not convinced that functions with _cli in their name should work from a Python call. We definitely don't advertise this API anywhere.

However, the problem remains that even if users call into init_config and save_config directly, as you'd probably want to recommend them to, then they don't get access to the default values that you'd get from the CLI.

The current implementation does not repeat the default values in e.g. init_config, as this also avoids them going out-of-sync across functions. But what if we define some of these default variables as a global on top? Then both functions can access them, they'd always be in sync, and we'd have functions that you can call directly from within Python (though not the _cli ones)

[rmitsch]

Looking into this in a bit more detail...

• Ugliness and error proneness of optional_args: agreed. The original intention was to not do this for each argument manually. A possible alternative would be to wrap the arguments in a function forcing the default value where necessary:

      ...
      config = init_config(
          lang=get_typer_option_value(lang),
         ...
      )

  It's not much better, but at least avoids a parallel data store for argument values.

• Support of _cli calls from Python: it's IMO totally legitimate not to support that.

• Default values for init_config(), save_config(): agreed, we shouldn't repeat default values. I'm not a huge fan of setting the default values as globals, but to be fair, I don't have a better idea.

So tl;dr: I think your suggestion of making the default values global is the least bad way to go forward. Should we bundle them in an object so they don't clutter the module namespace too much, like this?

class InitDefaultValues:
    output_file = "-"
    pretraining = False
    ...
    

[rmitsch]

Removed forcing of optional default values, added synchronized default values for init_config_cli() and init_cli(). I.e. calling init_config_cli() from Python is not fully supported in this version, since the original issue with optional arguments hasn't been resolved. We can alternatively recommend using init_cli() for calls with optional arguments though, since it includes default values now.

[svlandeg]

Looking good! Removed some of the introduced newlines and just had one more question on the test.

[svlandeg]

Should be good to merge once the CI goes green!

[rmitsch]

@svlandeg I think we can merge this?