lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
427 stars 52 forks source link

Representing nested args by their full "nesting path" even with no conflicts #75

Closed Ozziko closed 2 years ago

Ozziko commented 3 years ago

simple-parsing is simply awesome (thanks!). I'd like to suggest a feature that is crucial for parsing nested args, in my opinion, and the support of other researchers in my group. Here is a minimal example to make things clear:

from dataclasses import dataclass
from simple_parsing import ArgumentParser, ConflictResolution

@dataclass
class WandbArgs:
    dir: str = ''  # Wandb run cache dir, relative to project_path; leave empty for default
    use: bool # Whether to use wandb for experiment control, logging, saving...

@dataclass
class Data:
    dir: str = ''  # Data dir, relative to project_path; leave empty for default

@dataclass
class ProjectArgs:
    data: Data = Data()
    wandb: WandbArgs = WandbArgs()

parser = ArgumentParser(conflict_resolution=ConflictResolution.EXPLICIT)
parser.add_arguments(ProjectArgs, dest='args')
args = parser.parse_args().args

A command line to set all values can be: --use=True --wandb.dir='wandb' --data.dir='data'.

Since I passed ConflictResolution.EXPLICIT:

I claim that having wandb.use in the command line is much more clear, unambiguous, simple and intuitive than use (current behavior), especially for those who have many args and invested time in organizing them with nesting, to make it organized, clear and simple!.

I found a hack to get this functionality: I force duplication for all args by adding parser.add_arguments(ProjectArgs, dest='duplicated') to my example above (and ignoring the duplicated args) - forcing all args to apepar in command line with their full path. However, since I have many args in my current project, I need to also download and modify simple-parsing source code - set max_attempts=200 in simple_parsing -> conflicts.py - otherwise it raises an error, assuming so many conflicts are not possible and must be an error.

Therefore I suggest to add a force_explicit argument to ArgumentParser, such that ArgumentParser(force_explicit=True) makes all nested args appear in command line with their full "nesting path", no hack, no ConflictResolution.EXPLICIT, no max number of conflicts - simple, organized and clear!

Thanks!

lebrice commented 3 years ago

Hey @Ozziko , thanks for posting!

There is an option for allowing the full path: You can use add_dest_to_option_strings=True to the ArgumentParser constructor, and then you'll be able to pass the full path to your args, in addition to the short path.

However, there isn't currently an option for forcing the use of the full path, however it's something that could easily be added. I think I've discussed this in a previous issue, however I can't find it right now.

I'd really appreciate a PR for this if you can, since the change would be pretty simple:

You'd need to either change what value is returned by the option_strings property so it only includes the full path (the dest attribute on the FieldWrapper), or filter out the option strings that are passed to the add_argument function.

Hope this helps! Let me know if you need anything else.

Ozziko commented 3 years ago

Thanks @lebrice! add_dest_to_option_strings=True solves the key issue! I also agree that it's better to have an option to force using the full path (also showing the full paths in the help) to keep everything organized, but I don't think I'll have time to fix it anytime soon... I'm keeping this issue open in the meantime...

eladrich commented 2 years ago

Hi @lebrice, First of all, really liked the design of this project! Been looking for a while for a typed and hierarchical argparse extension I was wondering if you are planning to support enforcing the full path option, and if not whether you are still interested in a PR for that.

lebrice commented 2 years ago

Hi @eladrich! Yes I'd welcome a PR for that feature! I believe I left some instructions up here, feel free to take a crack at it, if you're interested! I'm available if you have any questions. Otherwise I'll work on this whenever I find some time, probably in a few weeks.

ivanprado commented 2 years ago

Thank you @Ozziko for your proposal. Indeed, I think it is a very useful feature. @lebrice I've tried to implement it, as you suggested, in this PR https://github.com/lebrice/SimpleParsing/pull/117. Hope I got it right.

nacitar commented 2 years ago

This issue looks to be resolved... the PR is merged, so it can likely be closed.