Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with casting argument to bool when target variable has type Union[bool, str] #379

Closed
thomasgaudelet opened this issue Sep 12, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@thomasgaudelet
Copy link

thomasgaudelet commented Sep 12, 2023

馃悰 Bug report

If an argument is typed as bool | str, when trying to set that argument to True from command line it will instead be set to string true.

To reproduce

Create a file test.py

from jsonargparse import ArgumentParser

def main_cli():
    parser = ArgumentParser()
    parser.add_argument("--plop", type=str|bool)
    cfg = parser.parse_args()
    print(cfg)


if __name__ == "__main__":
    main_cli()

From command line,
python test.py --plop true

Expected behavior

Expected behaviour would be to have cfg = {"plop": True}. Instead we get cfg = {"plop": "true"}.
It works as intended if parameter is set through a config file when using the CLI object.
But it always fails from command line.

Note, it works if the typing is bool|str

Environment

  • jsonargparse version (e.g., 4.8.0): 4.24.0
  • Python version (e.g., 3.9): 3.10
  • How jsonargparse was installed (e.g. pip install jsonargparse[all]): pip install jsonargparse
  • OS (e.g., Linux): Linux
@thomasgaudelet thomasgaudelet added the bug Something isn't working label Sep 12, 2023
@mauvilsa
Copy link
Member

That is weird. I tried the code above with python 3.10 in ubuntu 22.04 and got:

$ ./issue_379.py --plop true
Namespace(plop=True)

@thomasgaudelet
Copy link
Author

Ah wait, it depends on the order in the typing. If str|bool this fails as indicated. But it works as intended with bool|str. Amending message above for correctness.

@mauvilsa
Copy link
Member

mauvilsa commented Sep 12, 2023

This is the expected behavior. For unions it goes from left to right checking if the value is valid for the subtype. It stops when the value is valid. So if str is first, it will stay as str.

@thomasgaudelet
Copy link
Author

I see. I wonder if that could be amended as this can lead to silent failures. It feels like bool type should always be treated first when present.

@mauvilsa
Copy link
Member

This behavior will not be changed. Explaining that all subtypes are checked left to right is easy to explain and understand. Some arbitrary priority that overrides the general rule only for some types would lead to more confusion.

My main question would be, why is the type str | bool? According to that, any string is accepted. Thus, "true" is a valid value. If only some strings are accepted, shouldn't the type hint be different? See for instance Lightning-AI/pytorch-lightning#18370 where the type was changed to Literal["all"] | bool.

@thomasgaudelet
Copy link
Author

This is the exact package and parameter I have trouble with! Good to see the typing will be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants