Check types after overwriting

[mpariente]

The problem

If the value is wrongly typed in the config file (for example if a non-optional positional arg was not specified with --print_config), and is being overwritten by a valid value, an error is raised nonetheless.

What I'd like

Check types after all the parsing has been done, to allow this use case.

Minimal example

# parsing_workout.py
import jsonargparse

class Model:
    def __init__(self, n: int):
        pass

def get_args():
    parser = jsonargparse.ArgumentParser(parse_as_dict=True, description="Trial")
    parser.add_argument(
        "--config", action=jsonargparse.ActionConfigFile, help="Configuration file"
    )
    parser.add_class_arguments(Model, "model")
    args = parser.parse_args(with_meta=False)
    return args

def main():
    args = get_args()
    print(args)

if __name__ == "__main__":
    main()

Way that works

python parsing_workout.py --model.n 2 --print_config > conf.yml
# This works
python parsing_workout.py --config conf.yml --model.n 3

Way that doesn't

python parsing_workout.py --print_config > conf.yml
# This doesn't work
python parsing_workout.py --config conf.yml --model.n 3

and this is the error message:

parsing_workout.py: error: Parser key "data.n_src": int() argument must be a string, a bytes-like object or a number, not 'NoneType'

[mauvilsa]

You are right. This should not fail.

[mauvilsa]

@mpariente after a fast look at this I think that the fix is not simple. It seems I would need to reimplement argparse's _parse_known_args to have a way to disable the validation. I will look at it again later on to see if there is some other easier fix.

For the time being, the workaround is to not include the invalid entries in the config file. It is a bit cumbersome because --print_config includes them and you need to remove them manually. Maybe --print_config couple be improved to allow specifying what to include.

[mauvilsa]

Finally figured out a way to make this work. Fixed in c5119c6.

[glothe]

I've been getting a possibly related issue related to pytorch lightning's CLI when checking types of callbacks.

Here is a minimal example:

from pytorch_lightning import LightningModule, LightningDataModule

if __name__ == "__main__":
    LightningCLI(LightningModule, LightningDataModule)

When providing a invalid callback path, the normal mode correctly raises an error:

usage: cli.py [-h] [-c CONFIG] [--print_config [={comments,skip_null,skip_default}+]] {fit,validate,test,predict,tune} ...
cli.py: error: Configuration check failed :: Parser key "trainer.callbacks": Value "NotARealCallback" does not validate against any of the types in typing.Union[typing.List[pytorch_lightning.callbacks.callback.Callback], pytorch_lightning.callbacks.callback.Callback, NoneType]:
  - Expected a List but got "NotARealCallback"
  - Expected a dot import path string: NotARealCallback
  - Expected a <class 'NoneType'> but got "NotARealCallback"

However, when using the --print_config option, the callback types are not checked. The config is interpreted as:

trainer:
  ...
  callbacks: NotARealCallback
  ...

It's not a huge problem, since the type will be checked as soon as the config is instantiated, but it's still a bit surprising!

[mauvilsa]

@glothe thanks for the input. I am also concerned about the following:

from jsonargparse import ActionConfigFile, ArgumentParser
from typing import List

parser = ArgumentParser()
parser.add_argument('--cfg', action=ActionConfigFile)
parser.add_argument('--val', type=List[int])
print(parser.parse_args(['--val=2', '--cfg=val+: [3, 4]', '--val+=5']))

The parsing correctly gets the value as [3, 4, 5]. However, the argument --val=2 is an invalid value but the user will not notice that this argument was ignored. I think that checking the types only after overwriting leads to situations which are confusing for the user. Since this was originally proposed there have been some improvements which would help with the motivation this had. But we should change the behavior. For now I will reopen this issue. I will think about what would be a better new behavior and comment.

[mauvilsa]

The original description above suggested to only validate at the end, giving as example required arguments. I think the required arguments is the only case in which it should be checked at the end. Any intermediate argument which provides an invalid value should make the parser fail, even for --print_config. This new behavior was introduced in a recent commit and it is already part of the latest release v4.17.0. In pull request #193 new unit tests are added to make sure that there are no regressions in this behavior. @glothe you might want to check that for your case it now works as expected.