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

Skepticleo trainer argparser #1023

Merged
merged 32 commits into from
Mar 3, 2020
Merged

Conversation

williamFalcon
Copy link
Contributor

@williamFalcon williamFalcon commented Mar 3, 2020

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?

Duplicate to #952

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 🙃

@Borda Borda added the duplicate This issue or pull request already exists label Mar 3, 2020
return parser

@classmethod
def from_argparse_args(cls, args) -> Trainer:
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
def from_argparse_args(cls, args) -> Trainer:
def from_argparse_args(cls, args: Namespace) -> Trainer:

tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
tests/trainer/test_trainer.py Outdated Show resolved Hide resolved
@williamFalcon
Copy link
Contributor Author

@Borda how do we change the author to the original one?

@williamFalcon williamFalcon added the ready PRs ready to be merged label Mar 3, 2020
@williamFalcon williamFalcon added this to the 0.7.0 milestone Mar 3, 2020
@williamFalcon williamFalcon merged commit 4c5e82c into master Mar 3, 2020
@williamFalcon williamFalcon mentioned this pull request Mar 3, 2020
5 tasks
@Borda Borda deleted the skepticleo-trainerArgparser branch March 3, 2020 14:48

@classmethod
def from_argparse_args(cls, args):

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't there be documentation where the blank line is? did it get lost?

Copy link
Member

Choose a reason for hiding this comment

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

yeah it should...

@Borda Borda mentioned this pull request Mar 3, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* Added default parser for trainer and class method to construct trainer from default args

* Removed print statement

* Added test for constructing Trainer from command line args

* Removed extra line

* Removed redundant imports, removed whitespace from empty lines

* Fixed typo

* Updated default parser creation to get class attributes automatically

* Updated default parser creation to get class attributes automatically

* Added method to get default args for trainer

* Trimmed trainer get default args method

* Updated from argparse method to not return trainer with static arguments

* Update trainer get default args to classmethod

* adjustment

* fix

* Fixed variable name

* Update trainer.py

* Update test_trainer.py

* Update trainer.py

* Update tests/trainer/test_trainer.py

Co-Authored-By: Jirka Borovec <[email protected]>

* Update trainer.py

* Update test_trainer.py

* Update trainer.py

* Update test_trainer.py

* Update tests/trainer/test_trainer.py

Co-Authored-By: Jirka Borovec <[email protected]>

* Update pytorch_lightning/trainer/trainer.py

Co-Authored-By: Jirka Borovec <[email protected]>

* Update trainer.py

* Update test_trainer.py

Co-authored-by: Mudit Tanwani <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants