lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
399 stars 49 forks source link

Allow optional params to be checked with default=None, in addition to using the Optional type hint #132

Closed dblakely closed 2 years ago

dblakely commented 2 years ago

Is your feature request related to a problem? Please describe.

It's common to have args in a dataclass that are optional but aren't explicitly typed as such via the Optional type hint. For example:

from dataclasses import dataclass, field
from simple_parsing import ArgumentParser

@dataclass
class Args:
    x: int = field(default=None)

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

In this case, Args.x is clearly intended to be optional despite not having an Optional type hint. However, if you run this you'll get error: the following arguments are required: -x/--x. In order to run this, you have to change the line with x to be x: Optional[int] = field(default=None).

Of course, this is an easy change to make, but it's a little tedious, especially as many developers don't bother using Optional because it's kind of redundant with simply setting the default value to None.

But even beyond that, it would be nice to use simple_parsing with, say, the Huggingface's TrainingArguments class from the transformers library. However, that's currently not possible because that class contains lines of code like this one, where you have optional args that don't use the Optional type hint.

Describe the solution you'd like

Can we instead determine if an arg is optional by just checking if the default value inside the field is None?

If needed, I can volunteer to help make this change!

lebrice commented 2 years ago

Hello @dblakely , thanks for pointing this out!

Yeah sure, that sounds like a good idea.

I'd like to point out though that these field annotations aren't valid, from a type-checker's perspective: the arguments are optional, and so the annotation should either be int | None or Optional[int].

I agree with you, it sounds totally reasonable to consider them as Optional for simple-parsing though.

This should be a quite simple fix as well: I think that this is probably all that needs to be changed:

https://github.com/lebrice/SimpleParsing/blob/009a6013be2c72547116cfc3cfeb9e360d31ebc5/simple_parsing/wrappers/field_wrapper.py#L231

This should be:

    elif utils.is_optional(self.type) and self.field.default is not None:

This sounds like a simple, and potentially very useful contribution! Let me know if you run into any issues, and I'll be happy to help :)

Thanks again!

dblakely commented 2 years ago

Thanks @lebrice! I'll take a look at it and let you know if I have any issues.

joel-odlund commented 2 years ago

this seems to be an important feature. In the mean time, is there a workaround to have None as a default value for an optional parameter?

lebrice commented 2 years ago

Hi there @joel-odlund , just to clarify, you can already create optional arguments for a field of type T using the Optional[T] or T | None annotation on the field. When adding an entire dataclass using parser.add_arguments, you can also pass the default value to use.

Are you asking for another way to have optional arguments when not using the proper type annotation (what @dblakely is asking for in this thread)?

If that's what you're asking, then yes. There is a work-around for you to use in the meantime. However, there is no good reason to ever use the work-around I'm about to show you, because:

  1. It assumes that you have access to the code.
  2. It also assumes that you wouldn't want to use the proper type annotation. (either Optional[T] or T | None).

I don't want to encourage anyone to use type hints incorrectly, but it's possible there could be a good reason to do so, so I'll leave it up to you to decide.

from simple_parsing import field

@dataclass
class Foo:
    a: Optional[int] = None  # Correct way of making an optional argument (either `Optional[int]` or `int | None`).

    b: int = None  # Incorrect (and not working). This issue is about making this line behave like the one above.

    c: int = field(default=None, required=False)   # Still just as wrong as `b`, but will act as an optional argument (just like `a`).

I'll take a crack at this issue soon.