fixed extra dataloader bug #1196

TevenLeScao · 2020-03-19T23:31:03Z

What does this PR do?

Fixes #1181 (issue).

codecov · 2020-03-19T23:55:29Z

Codecov Report

Merging #1196 into master will increase coverage by <1%.
The diff coverage is 91%.

@@           Coverage Diff           @@
##           master   #1196    +/-   ##
=======================================
+ Coverage      92%     92%   +<1%     
=======================================
  Files          62      62            
  Lines        3188    3191     +3     
=======================================
+ Hits         2920    2923     +3     
  Misses        268     268

pytorch_lightning/trainer/training_loop.py

Borda

pls add CHNAGELOG note

pytorch_lightning/trainer/training_loop.py

ethanwharris · 2020-03-20T09:05:01Z

This looks good, it's not clear how the reload_dataloaders_every_epoch argument is supposed to work for val and test since we often don't do an epoch of those. But this fix is good for the train dataloader - perhaps the argument name should be changed to reset_train_dataloader_every_epoch? (and deprecate the old argument)

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

…xtra_dataloader_bug-fix

TevenLeScao · 2020-03-20T10:45:40Z

I've added a note in CHANGELOG.md

self.get_model() => model as it was already defined

CHANGELOG.md

ethanwharris

LGTM

pytorch_lightning/trainer/training_loop.py

TevenLeScao · 2020-03-23T15:18:55Z

Sorry, I'm not sure what the next steps are at the moment - @williamFalcon , do you want to follow @ethanwharris 's suggestion of changing reset_dataloader_every_epoch to reset_train_dataloader_every_epoch ? I'm not sure I understand your last message.

williamFalcon · 2020-03-24T18:52:13Z

@TevenLeScao yes, let's rename to reset_train_dataloader_every_epoch

…xtra_dataloader_bug-fix # Conflicts: # CHANGELOG.md # pytorch_lightning/trainer/training_loop.py

mergify · 2020-03-30T10:35:28Z

This pull request is now in conflict... :(

williamFalcon · 2020-03-30T11:54:30Z

pytorch_lightning/trainer/evaluation_loop.py

@@ -338,14 +338,14 @@ def run_evaluation(self, test_mode: bool = False):

        # select dataloaders
        if test_mode:
-            if self.reload_dataloaders_every_epoch or self.test_dataloaders is None:
+            if self.reload_train_dataloader_every_epoch or self.test_dataloaders is None:


this is wrong no? don’t we want to reset the test and val dataloaders with every call to evaluate?

Edit: I'm not sure what evaluation_loop is used for; why would we want to reload the test and/or val dataloader when it is called ? It doesn't strike me as an "every epoch" kind of thing.

If we want to keep that, maybe the compromise is to keep the name reload_dataloaders_every_epoch, and consider that this reloads the train dataloader every training epoch in training_loop, and the val/test dataloaders at every evaluation in evaluation_loop. This would fix the initial bug and keep all functionality the same and I feel like that should be the main objective here. I can just revert the last changes in this case, and the previously-approved PR should be good to go. Sorry if I'm reinventing the wheel here !

I thought the point was just that the reload_train_dataloader_every_epoch was doing stuff with test and val when it wasn't needed - not to revert the change?

I'm actually not sure anymore ! At least here I understand that @williamFalcon wants to reset them and @ethanwharris you want to call it reload_train_dataloader_every_epoch and not reset them.

But in any case I think it's better to have reload_dataloaders_every_epoch stand for everything, as it keeps previous functionality and doesn't split it into an argument for everything (ie train val and test)

evaluate runs the test and val loop.

Maybe i'm missing something, but if the user wants to reload the validation and test datasets every time evaluation is checked that should be allowed no?

Haha, I'm confused. Originally tbe point was made that reload_dataloaders_every_epoch didn't previously reload the test and val dataloaders, only train. So then I suggested the name change, but it turns out we do reload them? Anyway, let's make it so that reload_dataloaders_every_epoch does what it says on the tin and applies to val and test aswell, in which case I think the above comment from @tullie still applies. Can then revisit it if someone finds a use case where that doesn't work for them :)

@ethanwharris is this also solved?

It should be fixed now, but the CI says some checks were cancelled and I'm not sure why :/

Edit: seems fine second time around

williamFalcon

check comments

TevenLeScao · 2020-03-31T15:41:22Z

Should be good to go now !

Borda · 2020-03-31T15:47:50Z

@williamFalcon ^^

TevenLeScao · 2020-04-02T09:29:18Z

So is there anything left to do here before merging ?

assume it was done...

* fixed extra dataloader bug * Update pytorch_lightning/trainer/training_loop.py Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com> * updated CHANGELOG * Small non-repetition change self.get_model() => model as it was already defined * Update CHANGELOG.md * changed argument name to reload_train_dataloader_every_epoch * fixed doc underline too short * reverted to `reload_dataloaders_every_epoch` * fixed val and test reloading * fixed val and test reloading Co-authored-by: TevenLeScao <teven.lescao@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

fixed extra dataloader bug

0b6832f

Borda added the bug Something isn't working label Mar 20, 2020

Borda reviewed Mar 20, 2020

View reviewed changes

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

Borda requested changes Mar 20, 2020

View reviewed changes

pytorch_lightning/trainer/training_loop.py Show resolved Hide resolved

TevenLeScao and others added 3 commits March 20, 2020 11:29

Update pytorch_lightning/trainer/training_loop.py

c805f94

Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>

updated CHANGELOG

02349a9

Merge remote-tracking branch 'origin/extra_dataloader_bug-fix' into e…

79b1b3a

…xtra_dataloader_bug-fix

Small non-repetition change

d8241a8

self.get_model() => model as it was already defined

Borda requested a review from ethanwharris March 20, 2020 15:00

Borda approved these changes Mar 20, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

6b41ed2

Borda added this to the 0.7.2 milestone Mar 20, 2020

Borda added the ready PRs ready to be merged label Mar 20, 2020

ethanwharris approved these changes Mar 20, 2020

View reviewed changes

TevenLeScao requested a review from Borda March 20, 2020 15:42

tullie reviewed Mar 20, 2020

View reviewed changes

pytorch_lightning/trainer/training_loop.py Show resolved Hide resolved

Merge branch 'master' into extra_dataloader_bug-fix

9a45c12

Borda approved these changes Mar 23, 2020

View reviewed changes

williamFalcon removed the ready PRs ready to be merged label Mar 24, 2020

Borda requested review from tullie, williamFalcon and jeremyjordan March 26, 2020 10:34

jeremyjordan approved these changes Mar 27, 2020

View reviewed changes

Borda requested review from ethanwharris and Borda March 27, 2020 07:22

TevenLeScao added 2 commits March 30, 2020 12:30

changed argument name to reload_train_dataloader_every_epoch

00f3689

Merge remote-tracking branch 'origin/extra_dataloader_bug-fix' into e…

76ed916

…xtra_dataloader_bug-fix # Conflicts: # CHANGELOG.md # pytorch_lightning/trainer/training_loop.py

TevenLeScao and others added 2 commits March 30, 2020 12:42

fixed doc underline too short

f52d30a

Merge branch 'master' into extra_dataloader_bug-fix

8304bcc

williamFalcon reviewed Mar 30, 2020

View reviewed changes

williamFalcon previously requested changes Mar 30, 2020

View reviewed changes

TevenLeScao added 3 commits March 30, 2020 16:23

reverted to reload_dataloaders_every_epoch

899ab4f

fixed val and test reloading

53d3b14

fixed val and test reloading

4b5e599

Borda requested review from williamFalcon and jeremyjordan March 31, 2020 13:35

Borda merged commit 04935ea into Lightning-AI:master Apr 2, 2020

Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed extra dataloader bug #1196

fixed extra dataloader bug #1196

TevenLeScao commented Mar 19, 2020

codecov bot commented Mar 19, 2020 •

edited

Loading

Borda left a comment

ethanwharris commented Mar 20, 2020 •

edited

Loading

TevenLeScao commented Mar 20, 2020

ethanwharris left a comment

TevenLeScao commented Mar 23, 2020

williamFalcon commented Mar 24, 2020

mergify bot commented Mar 30, 2020

williamFalcon Mar 30, 2020

TevenLeScao Mar 30, 2020 •

edited

Loading

TevenLeScao Mar 30, 2020 •

edited

Loading

ethanwharris Mar 30, 2020

TevenLeScao Mar 30, 2020 •

edited

Loading

williamFalcon Mar 30, 2020

ethanwharris Mar 30, 2020

Borda Mar 31, 2020

TevenLeScao Mar 31, 2020 •

edited

Loading

williamFalcon left a comment

TevenLeScao commented Mar 31, 2020

Borda commented Mar 31, 2020

TevenLeScao commented Apr 2, 2020

fixed extra dataloader bug #1196

fixed extra dataloader bug #1196

Conversation

TevenLeScao commented Mar 19, 2020

What does this PR do?

codecov bot commented Mar 19, 2020 • edited Loading

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

ethanwharris commented Mar 20, 2020 • edited Loading

TevenLeScao commented Mar 20, 2020

ethanwharris left a comment

Choose a reason for hiding this comment

TevenLeScao commented Mar 23, 2020

williamFalcon commented Mar 24, 2020

mergify bot commented Mar 30, 2020

williamFalcon Mar 30, 2020

Choose a reason for hiding this comment

TevenLeScao Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

TevenLeScao Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

ethanwharris Mar 30, 2020

Choose a reason for hiding this comment

TevenLeScao Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

williamFalcon Mar 30, 2020

Choose a reason for hiding this comment

ethanwharris Mar 30, 2020

Choose a reason for hiding this comment

Borda Mar 31, 2020

Choose a reason for hiding this comment

TevenLeScao Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

williamFalcon left a comment

Choose a reason for hiding this comment

TevenLeScao commented Mar 31, 2020

Borda commented Mar 31, 2020

TevenLeScao commented Apr 2, 2020

codecov bot commented Mar 19, 2020 •

edited

Loading

ethanwharris commented Mar 20, 2020 •

edited

Loading

TevenLeScao Mar 30, 2020 •

edited

Loading

TevenLeScao Mar 30, 2020 •

edited

Loading

TevenLeScao Mar 30, 2020 •

edited

Loading

TevenLeScao Mar 31, 2020 •

edited

Loading