remove trainer hidden state | sanity refactor [2 / n] #7507

awaelchli · 2021-05-12T15:51:59Z

What does this PR do?

Follow up to #7437 with more writable state removed from trainer:

Trainer.hiddens
Trainer.split_idx
multiple_trainloader_mode does not belong in TrainLoop

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?
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?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

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

Did you have fun?

Make sure you had fun coding 🙃

for more information, see https://pre-commit.ci

codecov · 2021-05-12T15:53:37Z

Codecov Report

Merging #7507 (44c08d5) into master (a584196) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7507   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files         196     196           
  Lines       12828   12827    -1     
======================================
- Hits        11825   11808   -17     
- Misses       1003    1019   +16

… into refactor/trainer-state-sanity-1

for more information, see https://pre-commit.ci

ananthsub · 2021-05-12T19:52:57Z

pytorch_lightning/core/lightning.py

@@ -1619,7 +1619,7 @@ def get_progress_bar_dict(self):
        module_tbptt_enabled = self.truncated_bptt_steps > 0
        trainer_tbptt_enabled = self.trainer.truncated_bptt_steps is not None and self.trainer.truncated_bptt_steps > 0
        if module_tbptt_enabled or trainer_tbptt_enabled:
-            tqdm_dict["split_idx"] = self.trainer.split_idx
+            tqdm_dict["split_idx"] = self.trainer.train_loop.split_idx


is get_progress_bar_dict still needed on the lightning module? wouldn't things come from self.log ?

it's for users to override and customize the default elements in the progress bar. Like how the version number is displayed or apparently the split index here.

It's not something one could customize through self.log directly I would say.

Is it customization we would want to push to a custom progress bar callback instead of being part of the core module?

custom progress bar callback instead of being part of the core module?

IMO yes for maximum separation of concerns

But people might complain about having to define a custom progress bar just for this

one could argue maybe the split_idx is not very useful to display in the progbar but still I would keep the hook

ananthsub · 2021-05-12T19:54:26Z

pytorch_lightning/trainer/training_loop.py


        self.global_step = 0
        self.current_epoch = 0
        self.trainer.should_stop = False

        self.total_batch_idx = 0
        self.batch_idx = 0
+        self.split_idx = None


is this meant to be writable from the outside? should these be made available as properties instead?

while going through this, could you add a comment for what split_idx means for contributors who go through this code later?

no but for now I follow the above pattern and the major goal is to define this state strictly on the loop, not the trainer anymore.

while going through this, could you add a comment for what split_idx means for contributors who go through this code later?

yes

okay added comments

pytorch_lightning/trainer/training_loop.py

carmocca · 2021-05-12T21:08:38Z

pytorch_lightning/core/lightning.py

@@ -1619,7 +1619,7 @@ def get_progress_bar_dict(self):
        module_tbptt_enabled = self.truncated_bptt_steps > 0
        trainer_tbptt_enabled = self.trainer.truncated_bptt_steps is not None and self.trainer.truncated_bptt_steps > 0
        if module_tbptt_enabled or trainer_tbptt_enabled:
-            tqdm_dict["split_idx"] = self.trainer.split_idx
+            tqdm_dict["split_idx"] = self.trainer.train_loop.split_idx


custom progress bar callback instead of being part of the core module?

IMO yes for maximum separation of concerns

But people might complain about having to define a custom progress bar just for this

pytorch_lightning/trainer/training_loop.py

tchaton

LGTM !

awaelchli added 4 commits May 11, 2021 11:12

remove trainer is_last_batch

76004b7

rename hiddens -> _hiddens

0c5cd0c

move multiple dataloader mode

a8171ab

refactor split idx

211e250

awaelchli added the refactor label May 12, 2021

[pre-commit.ci] auto fixes from pre-commit.com hooks

9370329

for more information, see https://pre-commit.ci

awaelchli added 2 commits May 12, 2021 17:54

changelog

79ad4e3

Merge remote-tracking branch 'origin/refactor/trainer-state-sanity-1'…

cb09423

… into refactor/trainer-state-sanity-1

awaelchli added this to the v1.4 milestone May 12, 2021

awaelchli and others added 3 commits May 12, 2021 18:39

fix ridiculous test

cdea059

set default

9756178

[pre-commit.ci] auto fixes from pre-commit.com hooks

f397a27

for more information, see https://pre-commit.ci

awaelchli marked this pull request as ready for review May 12, 2021 19:23

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

awaelchli added the ready PRs ready to be merged label May 12, 2021

awaelchli requested a review from ananthsub May 12, 2021 19:23

ananthsub reviewed May 12, 2021

View reviewed changes

carmocca reviewed May 12, 2021

View reviewed changes

comments for index variables

3548978

carmocca approved these changes May 13, 2021

View reviewed changes

awaelchli enabled auto-merge (squash) May 13, 2021 00:48

awaelchli disabled auto-merge May 13, 2021 00:48

awaelchli enabled auto-merge (squash) May 13, 2021 00:48

mergify bot added the has conflicts label May 13, 2021

Merge branch 'master' into refactor/trainer-state-sanity-1

44c08d5

mergify bot removed the has conflicts label May 13, 2021

Borda approved these changes May 13, 2021

View reviewed changes

ananthsub approved these changes May 13, 2021

View reviewed changes

tchaton approved these changes May 17, 2021

View reviewed changes

awaelchli merged commit 6e6e29a into master May 17, 2021

awaelchli deleted the refactor/trainer-state-sanity-1 branch May 17, 2021 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove trainer hidden state | sanity refactor [2 / n] #7507

remove trainer hidden state | sanity refactor [2 / n] #7507

awaelchli commented May 12, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

ananthsub May 12, 2021

awaelchli May 12, 2021

awaelchli May 12, 2021

ananthsub May 12, 2021

carmocca May 12, 2021 •

edited

Loading

awaelchli May 12, 2021

ananthsub May 12, 2021

awaelchli May 12, 2021

awaelchli May 12, 2021

awaelchli May 12, 2021

carmocca May 12, 2021 •

edited

Loading

tchaton left a comment

remove trainer hidden state | sanity refactor [2 / n] #7507

remove trainer hidden state | sanity refactor [2 / n] #7507

Conversation

awaelchli commented May 12, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented May 12, 2021 • edited Loading

Codecov Report

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

carmocca May 12, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmocca May 12, 2021 • edited Loading

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

awaelchli commented May 12, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

carmocca May 12, 2021 •

edited

Loading

carmocca May 12, 2021 •

edited

Loading