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

Default argparser for Trainer #952

Closed
wants to merge 22 commits into from

Conversation

mtnwni
Copy link
Contributor

@mtnwni mtnwni commented Feb 26, 2020

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 🙃

@mtnwni mtnwni changed the title Default argparser for Trainer #916 Default argparser for Trainer Feb 26, 2020
@staticmethod
def add_default_args(parent_parser):

parser = ArgumentParser(parents=[parent_parser])
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Borda

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@XDynames XDynames Mar 2, 2020

Choose a reason for hiding this comment

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

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

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

@staticmethod
def add_default_args(parent_parser):
Copy link
Contributor

@williamFalcon williamFalcon Feb 26, 2020

Choose a reason for hiding this comment

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

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
Copy link

pep8speaks commented Feb 26, 2020

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

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

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

@staticmethod
def add_default_args(parent_parser):

parser = ArgumentParser(parents=[parent_parser])
Copy link
Member

Choose a reason for hiding this comment

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

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

@Borda Borda added feature Is an improvement or enhancement good first issue Good for newcomers labels Feb 27, 2020
@williamFalcon
Copy link
Contributor

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

@williamFalcon williamFalcon added this to the 0.6.2 milestone Feb 27, 2020
@Borda
Copy link
Member

Borda commented Feb 27, 2020

let's keep this to another PR :)

in this release or another?

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

@mtnwni mtnwni requested a review from Borda March 2, 2020 10:19
trainer_default_args = vars(Trainer())

for arg in trainer_default_args:
parser.add_argument('--{0}'.format(arg), default=trainer_args[arg], dest=arg)
Copy link
Member

Choose a reason for hiding this comment

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

is there a way how to get also help texts?

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
@mtnwni mtnwni requested a review from Borda March 3, 2020 04:29
@williamFalcon
Copy link
Contributor

@skepticleo mind rebasing so we can merge?
:)

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@@ -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)))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 mtnwni requested a review from Borda March 3, 2020 13:06

return parser

@classmethod
def from_argparse_args(cls, args):
def from_argparse_args(cls, args) -> Trainer:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@williamFalcon williamFalcon modified the milestones: 0.7.1, 0.7.0 Mar 3, 2020
@williamFalcon williamFalcon mentioned this pull request Mar 3, 2020
5 tasks
@williamFalcon
Copy link
Contributor

merged in #1023 for time-sensitive rebase

@williamFalcon
Copy link
Contributor

@Borda is attributing author for that commit

@Borda
Copy link
Member

Borda commented Mar 3, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default argparser
6 participants