lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
409 stars 51 forks source link

Nesting has confusing, hidden, behaviour #27

Closed MartinHowarth closed 4 years ago

MartinHowarth commented 4 years ago

Describe the bug It took me a lot of experimentation to find out that parameters are only prefixed with their nested group name if the name of the nested parameter conflicts with another. I expected it to prefix it with the nested group name at all times, and I think it should do this.

It should do this because of the following situation:

There is a second situation where this is useful as well: I define a dataclass with parameter names that make sense in the context of the name of the class they are defined under. However, if you take those names out of context without the name of the containing class, then they are ill-defined and therefore the resulting CLI is confusing. e.g.

@dataclass
class Bananas:
    count: int

@dataclass
class MyCli:
    bananas: Bananas

Gives the following CLI helptext:

usage: scratch_1.py [-h] --count int
scratch_1.py: error: the following arguments are required: --count

Note that count is now completely meaningless. I can obviously add a docstring to explain it, but that means that every parameter docstring in Bananas must make sure it references Bananas.

To Reproduce Comment/uncomment out B from MyCli and see the output of --help change.

from simple_parsing import ArgumentParser
from dataclasses import dataclass
from typing import *

@dataclass
class A:
    a: str
    b: str
@dataclass
class B:
    a: str
    b: str

@dataclass
class MyCli:

    A: A
    B: B  # Comment/uncomment this.

parser = ArgumentParser()
parser.add_arguments(MyCli, dest="args")
args = parser.parse_args()

Expected behavior Subgroups should always include the group name when specifying their parameters.

I.e. in the above example, without B, you should get:

usage: scratch_1.py [-h] -a str -b str
scratch_1.py: error: the following arguments are required: -A.a/--A.a, -A.b/--A.b

whereas you currently get this:

usage: scratch_1.py [-h] -a str -b str
scratch_1.py: error: the following arguments are required: -a/--a, -b/--b
lebrice commented 4 years ago

Hi @MartinHowarth! Thanks for posting this! In the case when there is a conflict (i.e., when two arguments would have the same name), you can already achieve what you propose by passing a different value of the ConflictResolution Enum to the constructor of the ArgumentParser.

In your case, you would use ConflictResolution.EXPLICIT, which will use the 'absolute path' as the prefix to all the arguments that have a conflict.

The reason why this might seem confusing or hidden is because the default value for this is parameter is ConflictResolution.AUTO, which adds the shortest discriminating prefix when needed.

I'll close this for now, but please feel free to reopen the issue if you feel like this doesn't address your issue.

Thanks a lot for posting, as always!

lebrice commented 4 years ago

Actually, now that I think about it, It would be pretty easily doable to add all the absolute path as another option string for argparse, in addition to the shortest discriminating one, even when there are no conflicts.

I'll test it out and I'll get back to you!

MartinHowarth commented 4 years ago

It's great to hear that ConflictResolution exists that would solve my problem - though I don't recall seeing that in the documentation. The documentation is under construction though so... perhaps that's a known issue / not surprising and will come later?

I'm still concerned that the default behaviour is still the confusing behaviour I originally posted. And the danger is that the only way to solve it is to break your command-line API by either adding the new flag you just merged in, or setting ConflictResolution. That is a change that you'd only notice to do months down the line after adding a new feature.

jordiae commented 3 years ago

Hi there! I agree with @MartinHowarth, is there any way to force prefixing even without any conflict? @lebrice

lebrice commented 3 years ago

Yes @jordiae you can pass the ConflictResolution.EXPLICIT value to the ArgumentParser constructor's conflict_resolution argument.

jordiae commented 3 years ago

Yes @jordiae you can pass the ConflictResolution.EXPLICIT value to the ArgumentParser constructor's conflict_resolution argument.

I already did, but doesn't this work only in case of conflict? I mean nested attributes appearing always with the corresponding prefix, even if there is no conflict at all.

jordiae commented 3 years ago

Hi @lebrice, sorry for re-opening the issue. I think there was a misunderstanding (my fault). Passing the ConflictResolution.EXPLICIT value to the ArgumentParser constructor's conflict_resolution argument is only used if there is a conflict. Instead, I want to force prefixing for all nested arguments, even if there is no potential for a conflict.

lebrice commented 3 years ago

Hey @jordiae thats fair, you can use add_dest_to_option_strings argument to the ArgumentParser constructor, which won't do exactly what you want, but it will at least add the option for you to reference everything with the fully qualified path.

Let me know if thats sufficient!

jordiae commented 3 years ago

Hey @jordiae thats fair, you can use add_dest_to_option_strings argument to the ArgumentParser constructor, which won't do exactly what you want, but it will at least add the option for you to reference everything with the fully qualified path.

Let me know if thats sufficient!

Hi, many thanks for the idea!