Custom argparser extension with Trainer arguments (argument types added)

[alexeykarnachev]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1139

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[pep8speaks]

Hello @alexeykarnachev! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-03-24 18:54:51 UTC

[Borda]

LGTM 🚀

[Borda]

Suggested change

	def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
	def _get_init_arguments_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:

sounds a bit better as it can be used elsewhere not only for argparser

[alexeykarnachev]

Maybe in this case it’s worth to make this method public?

[alexeykarnachev]

And also move the allowed_types to the method's input arguments?

[Borda]

yes, both are good suggestions, lest doing it...

[alexeykarnachev]

I decided to make this method more general. Let's a client decides what arguments he needs. For instance, add_argparse_args is a "client" of this method, and it restricts the allowed types by itself.
What are your thoughts on this?

[Borda]

that would be nice, just thinking if is better to defien wanted args/types or ignored ones

[codecov]

Codecov Report

Merging #1147 into master will not change coverage by %.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1147   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          61      61           
  Lines        3151    3151           
======================================
  Hits         2868    2868           
  Misses        283     283           

[Borda]

@alexeykarnachev could you pls update from master? we have fixed the failure in #1163

[joeddav]

This PR works great for me with the exception of boolean casting. I.e. if I run with --checkpoint_callback 0 or false or anything but an empty value, then args.checkpoint_callback -> True, since bool('false') -> True.

This can be resolved with distutils:

bool('false')
# True
bool(distutils.util.strtobool('false'))
# False

Would it be possible to add a line to check for bool types and cast them using the above? Otherwise the behavior is quite counterintuitive.

[Borda]

good point, @joeddav could you do it in review mode as a suggestion (so it is clear and @alexeykarnachev can simply accept it) or follow-up PR?

[joeddav]

Add more robust boolean type casting

[mergify]

This pull request is now in conflict... :(