Default argparser for Trainer #952

mtnwni · 2020-02-26T12:19:47Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
Did you read the contributor guideline?
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?

Adds a default argument parser for Trainer(#916)
Closes #916

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 🙃

…r from default args

williamFalcon · 2020-02-26T12:24:42Z

pytorch_lightning/trainer/trainer.py

+    @staticmethod
+    def add_default_args(parent_parser):
+
+        parser = ArgumentParser(parents=[parent_parser])


these should be auto-generated given a trainer class. we don't want to start making sure this list maintains parity.

@Borda

Also, could we add it to another mixin? This file is already very long.

we can regenerate it from class attributes, we may also get types (not sure hot the typing would cooperate)
but not sure how to pass help strings and single-letter shortcuts in an argument...

@PyTorchLightning/core-contributors any idea about passing help string
otherwise I would go for the automatic generated, good idea

I've drafted a solution to this that automatically parses intit's doc string for the help fields and the functions signature to populate the argument parser

williamFalcon · 2020-02-26T12:26:45Z

pytorch_lightning/trainer/trainer.py

@@ -781,6 +782,135 @@ def slurm_job_id(self) -> int:
            job_id = None
        return job_id

+    @staticmethod
+    def add_default_args(parent_parser):


maybe rename to add_trainer_args? and ideally it can be called by:

from pytorch_lightning import Trainer from argparse import ArgumentParser parser = ArgumentParser() parser = Trainer.add_argparse_args(parser)

Then we can use the same kind of language to enable any lightning module to do the same:

from pytorch_lightning import Trainer from argparse import ArgumentParser from project import MyPLModule parser = ArgumentParser() parser = Trainer.add_argparse_args(parser) parser = MyPLModule.add_argparse_args(parser)

pep8speaks · 2020-02-26T12:30:21Z

Hello @skepticleo! Thanks for updating this PR.

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

Comment last updated at 2020-03-03 14:19:55 UTC

Borda

How about a class method cosntruct:

class Trainer(...):
    @classmethod
    def create_from_args(cls):
        args = ArgParser(...)
        # generate the arguments
        # parse the arguments as hparams
        return cls(hparams)

so later you just call trainer = Trainer.create_from_args() and done lol

Borda · 2020-02-26T16:30:28Z

pytorch_lightning/trainer/trainer.py

+    @staticmethod
+    def add_default_args(parent_parser):
+
+        parser = ArgumentParser(parents=[parent_parser])


we can regenerate it from class attributes, we may also get types (not sure hot the typing would cooperate)
but not sure how to pass help strings and single-letter shortcuts in an argument...

@PyTorchLightning/core-contributors any idea about passing help string
otherwise I would go for the automatic generated, good idea

williamFalcon · 2020-02-27T20:42:41Z

How about a class method cosntruct:
class Trainer(...):
    @classmethod
    def create_from_args(cls):
        args = ArgParser(...)
        # generate the arguments
        # parse the arguments as hparams
        return cls(hparams)
so later you just call trainer = Trainer.create_from_args() and done lol

let's keep this to another PR :)

Borda · 2020-02-27T23:02:00Z

let's keep this to another PR :)

in this release or another?

Borda

pls check the discussion about auto-generated items
https://github.com/PyTorchLightning/pytorch-lightning/pull/952/files#r384611463

…rch-lightning into trainerArgparser # Conflicts: # pytorch_lightning/trainer/trainer.py

Borda · 2020-03-02T14:11:50Z

pytorch_lightning/trainer/trainer.py

+        trainer_default_args = vars(Trainer())
+
+        for arg in trainer_default_args:
+            parser.add_argument('--{0}'.format(arg), default=trainer_args[arg], dest=arg)


is there a way how to get also help texts?

pytorch_lightning/trainer/trainer.py

tests/test_trainer.py

williamFalcon · 2020-03-03T12:37:02Z

@skepticleo mind rebasing so we can merge?
:)

Borda

LGTM 🚀

Borda · 2020-03-03T12:48:18Z

tests/test_trainer.py

@@ -856,6 +857,26 @@ def test_end(self, outputs):
    Trainer().test(model)


+@mock.patch('argparse.ArgumentParser.parse_args',
+            return_value=argparse.Namespace(**(Trainer.default_attributes)))


Suggested change

return_value=argparse.Namespace(**(Trainer.default_attributes)))

return_value=argparse.Namespace(**Trainer.default_attributes))

@Borda this doesn't seem to work, I see a TypeError: type object argument after ** must be a mapping, not property while running the unit tests

mtnwni · 2020-03-03T13:18:30Z

pytorch_lightning/trainer/trainer.py


        return parser

    @classmethod
-    def from_argparse_args(cls, args):
+    def from_argparse_args(cls, args) -> Trainer:


Throwing a NameError: name 'Trainer' is not defined while running the tests

williamFalcon · 2020-03-03T14:32:53Z

merged in #1023 for time-sensitive rebase

williamFalcon · 2020-03-03T14:33:11Z

@Borda is attributing author for that commit

Borda · 2020-03-03T14:48:46Z

https://www.git-tower.com/learn/git/faq/change-author-name-email

muditt added 4 commits February 25, 2020 15:35

Added default parser for trainer and class method to construct traine…

ec07d11

…r from default args

Removed print statement

78b3552

Added test for constructing Trainer from command line args

77a1fc3

Removed extra line

a68baeb

mtnwni changed the title ~~Default argparser for Trainer #916~~ Default argparser for Trainer Feb 26, 2020

williamFalcon reviewed Feb 26, 2020

View reviewed changes

Merge branch 'master' into trainerArgparser

541e2ec

muditt added 2 commits February 26, 2020 18:57

Removed redundant imports, removed whitespace from empty lines

679c9a7

Fixed typo

a017312

Borda reviewed Feb 26, 2020

View reviewed changes

Borda added feature Is an improvement or enhancement good first issue Good for newcomers labels Feb 27, 2020

williamFalcon added this to the 0.6.2 milestone Feb 27, 2020

Borda requested changes Feb 29, 2020

View reviewed changes

muditt added 3 commits March 2, 2020 15:39

Updated default parser creation to get class attributes automatically

1c7fccc

Updated default parser creation to get class attributes automatically

c3942cd

Merge branch 'trainerArgparser' of https://github.com/skepticleo/pyto…

b31295f

…rch-lightning into trainerArgparser # Conflicts: # pytorch_lightning/trainer/trainer.py

mtnwni requested a review from Borda March 2, 2020 10:19

muditt added 2 commits March 2, 2020 18:22

Added method to get default args for trainer

fc66c0c

Trimmed trainer get default args method

7eb4149

Borda reviewed Mar 2, 2020

View reviewed changes

Borda requested changes Mar 2, 2020

View reviewed changes

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Outdated Show resolved Hide resolved

tests/test_trainer.py Outdated Show resolved Hide resolved

muditt added 2 commits March 3, 2020 09:52

Updated from argparse method to not return trainer with static arguments

7cf6a7c

Update trainer get default args to classmethod

0ee7128

mtnwni requested a review from Borda March 3, 2020 04:29

Borda added 2 commits March 3, 2020 13:46

adjustment

b417be7

fix

e456cbc

Borda approved these changes Mar 3, 2020

View reviewed changes

mtnwni requested a review from Borda March 3, 2020 13:06

mtnwni commented Mar 3, 2020

View reviewed changes

Fixed variable name

df0a487

williamFalcon modified the milestones: 0.7.1, 0.7.0 Mar 3, 2020

williamFalcon added 3 commits March 3, 2020 08:47

Update trainer.py

9a8d05a

Update test_trainer.py

baf5219

Update test_trainer.py

1a7c208

williamFalcon mentioned this pull request Mar 3, 2020

Skepticleo trainer argparser #1023

Merged

5 tasks

williamFalcon added 2 commits March 3, 2020 09:19

Update trainer.py

60d4ca5

Update test_trainer.py

cd25d79

williamFalcon closed this Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default argparser for Trainer #952

Default argparser for Trainer #952

mtnwni commented Feb 26, 2020 •

edited

Loading

williamFalcon Feb 26, 2020

hadim Feb 26, 2020

Borda Feb 26, 2020

XDynames Mar 2, 2020 •

edited

Loading

williamFalcon Feb 26, 2020 •

edited

Loading

pep8speaks commented Feb 26, 2020 •

edited

Loading

Borda left a comment

Borda Feb 26, 2020

williamFalcon commented Feb 27, 2020

Borda commented Feb 27, 2020

Borda left a comment

Borda Mar 2, 2020

williamFalcon commented Mar 3, 2020

Borda left a comment

Borda Mar 3, 2020

mtnwni Mar 3, 2020

mtnwni Mar 3, 2020

williamFalcon commented Mar 3, 2020

williamFalcon commented Mar 3, 2020

Borda commented Mar 3, 2020

	return_value=argparse.Namespace(**(Trainer.default_attributes)))
	return_value=argparse.Namespace(**Trainer.default_attributes))

Default argparser for Trainer #952

Default argparser for Trainer #952

Conversation

mtnwni commented Feb 26, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XDynames Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

williamFalcon Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Feb 26, 2020 • edited Loading

Comment last updated at 2020-03-03 14:19:55 UTC

Borda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamFalcon commented Feb 27, 2020

Borda commented Feb 27, 2020

Borda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamFalcon commented Mar 3, 2020

Borda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamFalcon commented Mar 3, 2020

williamFalcon commented Mar 3, 2020

Borda commented Mar 3, 2020

mtnwni commented Feb 26, 2020 •

edited

Loading

XDynames Mar 2, 2020 •

edited

Loading

williamFalcon Feb 26, 2020 •

edited

Loading

pep8speaks commented Feb 26, 2020 •

edited

Loading