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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow --arg Value for booleans in HfArgumentParser #9823

Merged
merged 3 commits into from
Jan 27, 2021
Merged

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Jan 27, 2021

What does this PR do?

The primary reason I dived into this PR is because when launching a training with sagemaker, bool arguments need to be passed along with something (e.g. we can't do --do_train, we have to do --do_train True because the arguments are passed as a dict). This is refused by the current argparser.

This PR changes a little bit the way HfArgumentParser handles bool fields in dataclasses. Up until now:

  • a bool arg foo with True as a default gives a flag --no_foo that stores False in foo
  • a bool arg bar with no default value or False as a default value gives a flag bar that stores True in bar
  • an optional bool arg opt gives the same as a bool if its default is True or False. If the default is None, it gives a flag --opt that requires an argument that accepts anything and stores the value as a string (which is obviously a bug)

After this PR, the following happens:

  • a bool arg foo with True as a default gives a flag --no_foo that stores False in foo, it also gives a flag --foo that can be used as is (will store True in foo), or by using any truthy/falsy value (--foo yes, --foo True, --foo no...) that will store the result as a proper bool.
  • a bool arg bar with no default value or False gives a flag bar that can be used as is (will store True in bar), or by using any truthy/falsy value (--bar yes, --bar True, --bar no...) that will store the result as a proper bool.
  • an optional bool arg opt gives the same as a bool if its default is True or False. If the default is None, it gives a flag --opt that requires an argument that accepts a truthy value and stores the value as a proper bool.

In all cases above, when a truthy value is expected but something else is passed (that is not true, false, yes, no, 1, 0, t, f, y, n), an error is raised.

So no breaking changes at all and all bool values can be used with an argument so that sagemaker is happy. Tests are updated and improved to check the behaviors summarized above are all correct.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, LGTM!

@@ -85,11 +97,20 @@ def _add_dataclass_arguments(self, dtype: DataClassType):
if field.default is not dataclasses.MISSING:
kwargs["default"] = field.default
elif field.type is bool or field.type is Optional[bool]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hadn't seen that before, taught me something about types!

>>> class Field:
...     type = Optional[bool]
...
>>> f = Field()
>>> f.type is bool
False
>>> f.type is Optional[bool]
True

elif v.lower() in ("no", "false", "f", "n", "0"):
return False
else:
raise ArgumentTypeError("Boolean value expected.")
Copy link
Contributor

@patrickvonplaten patrickvonplaten Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) would it maybe make more sense to say "Truthy value expected that is either of type bool or one of ['yes', ....]" or else the user always thinks he/she can only pass True or False

@sgugger sgugger merged commit 893120f into master Jan 27, 2021
@sgugger sgugger deleted the fix_hf_argparser branch January 27, 2021 14:31
Qbiwan pushed a commit to Qbiwan/transformers that referenced this pull request Jan 31, 2021
* Allow --arg Value for booleans in HfArgumentParser

* Update last test

* Better error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants