Fix `_should_reload_dl_epoch` causing inconsistent validation dataloader reloading #11036

adamviola · 2021-12-11T07:31:28Z

What does this PR do?

When the flag reload_dataloaders_every_n_epochs is set, PyTorch Lightning reloads dataloaders on epochs that are multiples of reload_dataloaders_every_n_epochs. This causes inconsistent validation dataloader reloading when the flagscheck_val_every_n_epoch or val_check_interval are set.

This PR now has the trainer store the epoch of the last reload of the train and validation dataloaders. Each dataloader is now eligible to reload when current_epoch - dl_last_reload_epoch >= reload_dataloaders_every_n_epochs.

Does your PR introduce any breaking changes? If yes, please list them.

No

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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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?

Actually yes

carmocca

Great work!

tests/trainer/test_dataloaders.py

pytorch_lightning/trainer/trainer.py

…der reloading (Lightning-AI#10948)

…ehavior

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

tests/trainer/test_dataloaders.py

tchaton

Really nice contribution!

pytorch_lightning/trainer/data_loading.py

tests/trainer/test_dataloaders.py

Co-authored-by: thomas chaton <[email protected]>

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

@adamviola

…der reloading (#11036) Co-authored-by: Adam Viola <@adamviola> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: thomas chaton <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>

…der reloading (#11036) Co-authored-by: Adam Viola <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: thomas chaton <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]>

adamviola force-pushed the bugfix/inconsistent-dl-reloading branch 3 times, most recently from 95ac137 to 123984d Compare December 11, 2021 20:39

adamviola marked this pull request as ready for review December 11, 2021 21:17

adamviola requested review from Borda, SeanNaren, awaelchli, carmocca, justusschock, kaushikb11, rohitgr7, tchaton and williamFalcon as code owners December 11, 2021 21:17

adamviola changed the title ~~Fix _should_reload_dl_epoch causing inconsistent validation dataloa…~~ Fix _should_reload_dl_epoch causing inconsistent validation dataloader reloading Dec 11, 2021

carmocca approved these changes Dec 13, 2021

View reviewed changes

tests/trainer/test_dataloaders.py Show resolved Hide resolved

carmocca added this to the 1.5.x milestone Dec 13, 2021

carmocca added bug Something isn't working data handling Generic data-related topic labels Dec 13, 2021

awaelchli reviewed Dec 14, 2021

View reviewed changes

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

mergify bot added the has conflicts label Dec 20, 2021

adamviola and others added 4 commits December 20, 2021 14:15

Fix _should_reload_dl_epoch causing inconsistent validation dataloa…

5c406d1

…der reloading (Lightning-AI#10948)

Update tests to reflect changed reload_dataloaders_every_n_epochs b…

9ac0df4

…ehavior

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

e060622

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

Moved changes to TrainerDataLoadingMixin

1df1282

adamviola force-pushed the bugfix/inconsistent-dl-reloading branch from 0300720 to 1df1282 Compare December 20, 2021 19:27

mergify bot removed the has conflicts label Dec 20, 2021

awaelchli reviewed Dec 21, 2021

View reviewed changes

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

tests/trainer/test_dataloaders.py Show resolved Hide resolved

tchaton approved these changes Dec 21, 2021

View reviewed changes

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

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

tests/trainer/test_dataloaders.py Show resolved Hide resolved

tests/trainer/test_dataloaders.py Outdated Show resolved Hide resolved

mergify bot added the ready PRs ready to be merged label Dec 21, 2021

Apply suggestions from code review

d9acba0

Co-authored-by: thomas chaton <[email protected]>

Borda approved these changes Dec 21, 2021

View reviewed changes

adamviola and others added 6 commits December 21, 2021 23:50

Apply changes to tests from code review

6df76ac

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

b05e1b5

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

Merge branch 'master' into bugfix/inconsistent-dl-reloading

2ff11fc

move property to the top

a075ce4

rearrange

00fec53

add changelog

d414963

awaelchli approved these changes Dec 28, 2021

View reviewed changes

awaelchli enabled auto-merge (squash) December 28, 2021 00:30

awaelchli merged commit 1fc046c into Lightning-AI:master Dec 28, 2021

rohitgr7 mentioned this pull request Feb 7, 2022

Weekly Patch Release v1.5.10 #11789

Closed

12 tasks

carmocca mentioned this pull request Jul 27, 2022

check_val_every_n_epoch bug with list of dataloaders #12145

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix `_should_reload_dl_epoch` causing inconsistent validation dataloader reloading #11036

Fix `_should_reload_dl_epoch` causing inconsistent validation dataloader reloading #11036

Uh oh!

adamviola commented Dec 11, 2021 •

edited

Loading

Uh oh!

carmocca left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix _should_reload_dl_epoch causing inconsistent validation dataloader reloading #11036

Fix _should_reload_dl_epoch causing inconsistent validation dataloader reloading #11036

Uh oh!

Conversation

adamviola commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Uh oh!

carmocca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix `_should_reload_dl_epoch` causing inconsistent validation dataloader reloading #11036

Fix `_should_reload_dl_epoch` causing inconsistent validation dataloader reloading #11036

adamviola commented Dec 11, 2021 •

edited

Loading