Custom argparser extension with Trainer arguments (argument types added) #1147

alexeykarnachev · 2020-03-14T15:33:28Z

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?

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 · 2020-03-14T15:33:33Z

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 🚀

pytorch_lightning/trainer/trainer.py

Borda · 2020-03-15T22:20:45Z

pytorch_lightning/trainer/trainer.py

-    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
-        """Extend existing argparse by default `Trainer` attributes."""
-        parser = ArgumentParser(parents=[parent_parser], add_help=False)
+    def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:


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

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

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

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

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?

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

tests/trainer/test_trainer.py

…he Trainer now

…ainer-argparser-types

codecov · 2020-03-16T11:05:39Z

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 · 2020-03-17T07:54:44Z

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

…he Trainer now

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

…in tuples (not in sets)

…ainer-argparser-types

joeddav · 2020-03-20T15:04:09Z

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 · 2020-03-20T15:18:11Z

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

pytorch_lightning/trainer/trainer.py

Co-Authored-By: Joe Davison <joe@huggingface.co>

mergify · 2020-03-24T18:53:45Z

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

add_argparse_args method fixed (argument types added)

ea6b8f1

akarnachev added 2 commits March 14, 2020 18:35

CHANGELOG.md upd

28e9348

autopep8 fixes

6c290dd

alexeykarnachev marked this pull request as ready for review March 14, 2020 15:45

alexeykarnachev changed the title ~~Custom argparser extension with Trainer arguments (argument types added)~~ WIP Custom argparser extension with Trainer arguments (argument types added) Mar 14, 2020

--gpus=0 removed from test (for ci tests)

4444701

alexeykarnachev changed the title ~~WIP Custom argparser extension with Trainer arguments (argument types added)~~ Custom argparser extension with Trainer arguments (argument types added) Mar 14, 2020

alexeykarnachev mentioned this pull request Mar 14, 2020

Can't cast Trainer automatically generated args to their required types #1139

Closed

akarnachev added 2 commits March 14, 2020 19:37

typo fixed

0ec101f

reduce on plateau scheduler fixed

bc7dd6f

Borda added the feature Is an improvement or enhancement label Mar 15, 2020

Borda added this to the 0.7.2 milestone Mar 15, 2020

Borda approved these changes Mar 15, 2020

View reviewed changes

akarnachev added 10 commits March 16, 2020 12:23

Trainer cli related tests moved to test_trainer_cli.py

57a22ea

refactored: get_init_arguments_and_types is a public classmethod of t…

2d220d5

…he Trainer now

test_get_init_arguments_and_types added

5b5042e

autopep8 fixes

dd81a39

Merge remote-tracking branch 'upstream/master'

b4d9605

Trainer cli related tests moved to test_trainer_cli.py

977b2de

refactored: get_init_arguments_and_types is a public classmethod of t…

7a9ba50

…he Trainer now

test_get_init_arguments_and_types added

551ff24

autopep8 fixes

d3c1fdc

Merge remote-tracking branch 'origin/trainer-argparser-types' into tr…

1f92d92

…ainer-argparser-types

akarnachev added 4 commits March 17, 2020 11:58

merged

2b27e3c

Trainer cli related tests moved to test_trainer_cli.py

f501ce6

refactored: get_init_arguments_and_types is a public classmethod of t…

4bccfd7

…he Trainer now

test_get_init_arguments_and_types added

38dcba7

akarnachev and others added 17 commits March 17, 2020 20:04

Trainer cli related tests moved to test_trainer_cli.py

3fd9a99

autopep8 fixes

36ca20b

Trainer cli related tests moved to test_trainer_cli.py

f7cc4e1

autopep8 fixes

ce837fe

Trainer cli related tests moved to test_trainer_cli.py

8aa062a

test_get_init_arguments_and_types added

d447aa3

autopep8 fixes

aaada85

Apply suggestions from code review

87b6cbb

cosmetics

a9f996b

cosmetics

a4702ff

Update pytorch_lightning/trainer/trainer.py

162f3ab

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

Trainer.get_init_arguments_and_types now returns arg types wrapped …

a8b21d9

…in tuples (not in sets)

deprecated args are now ignored in argparser

2a5d46d

get_deprecated_arg_names small refactor

3b7c865

get_deprecated_arg_names bug fixed

d07870d

Merge remote-tracking branch 'origin/trainer-argparser-types' into tr…

012d425

…ainer-argparser-types

Merge branch 'master' into trainer-argparser-types

9449b98

Borda mentioned this pull request Mar 18, 2020

store_true for bools in the trainer's argparse parser #1168

Closed

joeddav suggested changes Mar 20, 2020

View reviewed changes

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

alexeykarnachev and others added 3 commits March 20, 2020 18:56

Update pytorch_lightning/trainer/trainer.py

94c5e09

Co-Authored-By: Joe Davison <joe@huggingface.co>

Update pytorch_lightning/trainer/trainer.py

4f8314d

Co-Authored-By: Joe Davison <joe@huggingface.co>

Merge branch 'master' into trainer-argparser-types

77847b7

Merge branch 'master' into trainer-argparser-types

63d30fd

williamFalcon merged commit ced662f into Lightning-AI:master Mar 24, 2020

Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021

carmocca mentioned this pull request Aug 26, 2022

Remove the broken and unused get_deprecated_arg_names #14415

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom argparser extension with Trainer arguments (argument types added) #1147

Custom argparser extension with Trainer arguments (argument types added) #1147

alexeykarnachev commented Mar 14, 2020 •

edited

Loading

pep8speaks commented Mar 14, 2020 •

edited

Loading

Borda left a comment

Borda Mar 15, 2020

alexeykarnachev Mar 16, 2020

alexeykarnachev Mar 16, 2020

Borda Mar 16, 2020

alexeykarnachev Mar 16, 2020 •

edited

Loading

Borda Mar 16, 2020

codecov bot commented Mar 16, 2020 •

edited

Loading

Borda commented Mar 17, 2020

joeddav commented Mar 20, 2020

Borda commented Mar 20, 2020

joeddav left a comment

mergify bot commented Mar 24, 2020

	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]:

Custom argparser extension with Trainer arguments (argument types added) #1147

Custom argparser extension with Trainer arguments (argument types added) #1147

Conversation

alexeykarnachev commented Mar 14, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Mar 14, 2020 • edited Loading

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

Borda left a comment

Choose a reason for hiding this comment

Borda Mar 15, 2020

Choose a reason for hiding this comment

alexeykarnachev Mar 16, 2020

Choose a reason for hiding this comment

alexeykarnachev Mar 16, 2020

Choose a reason for hiding this comment

Borda Mar 16, 2020

Choose a reason for hiding this comment

alexeykarnachev Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Borda Mar 16, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2020 • edited Loading

Codecov Report

Borda commented Mar 17, 2020

joeddav commented Mar 20, 2020

Borda commented Mar 20, 2020

joeddav left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 24, 2020

alexeykarnachev commented Mar 14, 2020 •

edited

Loading

pep8speaks commented Mar 14, 2020 •

edited

Loading

alexeykarnachev Mar 16, 2020 •

edited

Loading

codecov bot commented Mar 16, 2020 •

edited

Loading