`_launch` refactor and types [1/n] #7232

carmocca · 2021-04-27T12:19:22Z

What does this PR do?

Split fit() and _launch() to avoid if checks for state in fit(). Suggested by @ananthsub: TrainerState refactor [5/5] #7173 (comment)
Typing
Fixed bug where stage was not being set back to tuning after the tuner calls fit internally

Fixes #7242

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
[n/a] Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
[n/a] Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

codecov · 2021-04-27T12:46:10Z

Codecov Report

Merging #7232 (69e7537) into master (e147127) will decrease coverage by 2%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #7232    +/-   ##
=======================================
- Coverage      91%     90%    -2%     
=======================================
  Files         198     199     +1     
  Lines       12725   13216   +491     
=======================================
+ Hits        11633   11859   +226     
- Misses       1092    1357   +265

pytorch_lightning/plugins/training_type/training_type_plugin.py

pytorch_lightning/tuner/batch_size_scaling.py

awaelchli · 2021-04-28T04:17:51Z

pytorch_lightning/trainer/trainer.py

@@ -860,14 +830,50 @@ def run_sanity_check(self, ref_model):
            # prevents sanity check to affect random sampling in training
            reset_seed()

+    def fit(


don't we want to move this up? It would be nice if the entry points fit, validate, test came first.

Agree. Let's try to organise, so it is simple to follow.

I'll do this in a separate PR. Keeping them where they are for now so they are together

tchaton · 2021-04-28T07:26:09Z

pytorch_lightning/trainer/trainer.py

@@ -860,14 +830,50 @@ def run_sanity_check(self, ref_model):
            # prevents sanity check to affect random sampling in training
            reset_seed()

+    def fit(


Agree. Let's try to organise, so it is simple to follow.

pytorch_lightning/trainer/trainer.py

tchaton

LGTM !

pytorch_lightning/trainer/trainer.py

pep8speaks · 2021-04-28T14:33:25Z

Hello @carmocca! Thanks for updating this PR.

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

Comment last updated at 2021-04-28 14:36:20 UTC

ananthsub · 2021-04-28T15:59:34Z

pytorch_lightning/trainer/trainer.py

@@ -408,36 +409,13 @@ def __init__(
        # Callback system
        self.on_init_end()

-    def fit(
+    def _launch(


Nit : I prefer _run or _execute vs launch, which I associate more with distributed launch/process creation

Bit late as it is already merged, but if you want to open a PR with the renaming change, that's fine by me! 👍

carmocca added 2 commits April 27, 2021 14:13

_fit_impl refactor and types

395ce2a

Fix return

65aa690

carmocca added bug Something isn't working refactor labels Apr 27, 2021

carmocca added this to the v1.3 milestone Apr 27, 2021

carmocca self-assigned this Apr 27, 2021

carmocca requested review from awaelchli, Borda, justusschock, kaushikb11, SeanNaren, SkafteNicki, tchaton and williamFalcon as code owners April 27, 2021 12:19

Remove return docstring

b3f3314

carmocca changed the title ~~_fit_impl refactor and types~~ _fit_impl refactor and types [1/n] Apr 27, 2021

carmocca added 2 commits April 27, 2021 14:31

Fixes

e7c3657

Fixes

8060424

mergify bot added the has conflicts label Apr 27, 2021

Merge branch 'master' into use-fit-impl

c9a9ca9

mergify bot removed the has conflicts label Apr 27, 2021

awaelchli reviewed Apr 27, 2021

View reviewed changes

pytorch_lightning/plugins/training_type/training_type_plugin.py Show resolved Hide resolved

carmocca mentioned this pull request Apr 27, 2021

Remove trainer.fit return value [2/n] #7237

Merged

11 tasks

Undo results change

67d5ca8

carmocca requested a review from awaelchli April 27, 2021 13:52

carmocca mentioned this pull request Apr 27, 2021

TrainerState refactor [5/5] #7173

Merged

9 tasks

ananthsub approved these changes Apr 27, 2021

View reviewed changes

pytorch_lightning/tuner/batch_size_scaling.py Outdated Show resolved Hide resolved

ananthsub mentioned this pull request Apr 27, 2021

[Trainer] Use name mangling to allow for subclassing #7242

Closed

awaelchli approved these changes Apr 28, 2021

View reviewed changes

tchaton reviewed Apr 28, 2021

View reviewed changes

carmocca changed the title ~~_fit_impl refactor and types [1/n]~~ _launch refactor and types [1/n] Apr 28, 2021

tchaton approved these changes Apr 28, 2021

View reviewed changes

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

Revert changes for a separate PR

d7fbc5d

carmocca force-pushed the use-fit-impl branch from 1abfe2c to d7fbc5d Compare April 28, 2021 14:33

flake8

69e7537

carmocca enabled auto-merge (squash) April 28, 2021 14:48

carmocca added the ready PRs ready to be merged label Apr 28, 2021

tchaton disabled auto-merge April 28, 2021 14:53

carmocca merged commit bdc4272 into master Apr 28, 2021

carmocca deleted the use-fit-impl branch April 28, 2021 15:41

ananthsub reviewed Apr 28, 2021

View reviewed changes

awaelchli mentioned this pull request Apr 28, 2021

update training type plugin docs regarding result caching #7261

Merged

11 tasks

ananthsub mentioned this pull request Apr 29, 2021

Rename trainer._launch to trainer._run #7265

Merged

8 tasks

carmocca mentioned this pull request Apr 30, 2021

Move trainer.{fit,validate,test,predict,tune} to the top of trainer.py #7295

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`_launch` refactor and types [1/n] #7232

`_launch` refactor and types [1/n] #7232

carmocca commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

awaelchli Apr 28, 2021

tchaton Apr 28, 2021

carmocca Apr 28, 2021

tchaton Apr 28, 2021

tchaton left a comment

pep8speaks commented Apr 28, 2021 •

edited

Loading

ananthsub Apr 28, 2021

carmocca Apr 28, 2021

_launch refactor and types [1/n] #7232

_launch refactor and types [1/n] #7232

Conversation

carmocca commented Apr 27, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

awaelchli Apr 28, 2021

Choose a reason for hiding this comment

tchaton Apr 28, 2021

Choose a reason for hiding this comment

carmocca Apr 28, 2021

Choose a reason for hiding this comment

tchaton Apr 28, 2021

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

pep8speaks commented Apr 28, 2021 • edited Loading

Comment last updated at 2021-04-28 14:36:20 UTC

ananthsub Apr 28, 2021

Choose a reason for hiding this comment

carmocca Apr 28, 2021

Choose a reason for hiding this comment

`_launch` refactor and types [1/n] #7232

`_launch` refactor and types [1/n] #7232

carmocca commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

pep8speaks commented Apr 28, 2021 •

edited

Loading