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.

Sign up for GitHub

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

fixed extra dataloader bug #1196

Merged
merged 14 commits into from
Apr 2, 2020
Merged

fixed extra dataloader bug #1196

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0b6832f
fixed extra dataloader bug
TevenLeScao Mar 19, 2020
c805f94
Update pytorch_lightning/trainer/training_loop.py
TevenLeScao Mar 20, 2020
02349a9
updated CHANGELOG
TevenLeScao Mar 20, 2020
79b1b3a
Merge remote-tracking branch 'origin/extra_dataloader_bug-fix' into e…
TevenLeScao Mar 20, 2020
d8241a8
Small non-repetition change
TevenLeScao Mar 20, 2020
6b41ed2
Update CHANGELOG.md
Borda Mar 20, 2020
9a45c12
Merge branch 'master' into extra_dataloader_bug-fix
TevenLeScao Mar 23, 2020
00f3689
changed argument name to reload_train_dataloader_every_epoch
TevenLeScao Mar 30, 2020
76ed916
Merge remote-tracking branch 'origin/extra_dataloader_bug-fix' into e…
TevenLeScao Mar 30, 2020
f52d30a
fixed doc underline too short
TevenLeScao Mar 30, 2020
8304bcc
Merge branch 'master' into extra_dataloader_bug-fix
TevenLeScao Mar 30, 2020
899ab4f
reverted to `reload_dataloaders_every_epoch`
TevenLeScao Mar 30, 2020
53d3b14
fixed val and test reloading
TevenLeScao Mar 31, 2020
4b5e599
fixed val and test reloading
TevenLeScao Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- `Trainer.add_argparse_args` classmethod fixed. Now it adds a type for the arguments ([#1147](https://github.com/PyTorchLightning/pytorch-lightning/pull/1147)).
- Fixed bug related to type cheking of `ReduceLROnPlateau` lr schedulers([#1114](https://github.com/PyTorchLightning/pytorch-lightning/issues/1114))
- Fixed a bug to ensure lightning checkpoints to be backward compatible ([#1132](https://github.com/PyTorchLightning/pytorch-lightning/pull/1132))
- Fixed a bug that created an extra dataloader with active `reload_dataloaders_every_epoch` ([#1181](https://github.com/PyTorchLightning/pytorch-lightning/issues/1181)
- Changed argument name from `reload_dataloaders_every_epoch` to `reload_train_dataloader_every_epoch`
- Fixed all warnings and errors in the docs build process ([#1191](https://github.com/PyTorchLightning/pytorch-lightning/pull/1191))
- Fixed an issue where `val_percent_check=0` would not disable validation ([#1251](https://github.com/PyTorchLightning/pytorch-lightning/pull/1251))

Expand Down
6 changes: 3 additions & 3 deletions pytorch_lightning/core/lightning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,7 +1157,7 @@ def train_dataloader(self) -> DataLoader:
PyTorch DataLoader

Return a dataloader. It will not be called every epoch unless you set
```Trainer(reload_dataloaders_every_epoch=True)```.
```Trainer(reload_train_dataloader_every_epoch=True)```.

It's recommended that all data downloads and preparation happen in prepare_data().

Expand Down Expand Up @@ -1201,7 +1201,7 @@ def test_dataloader(self) -> Union[DataLoader, List[DataLoader]]:
r"""

Return a dataloader. It will not be called every epoch unless you set
```Trainer(reload_dataloaders_every_epoch=True)```.
```Trainer(reload_train_dataloader_every_epoch=True)```.

It's recommended that all data downloads and preparation happen in prepare_data().

Expand Down Expand Up @@ -1243,7 +1243,7 @@ def val_dataloader(self) -> Union[DataLoader, List[DataLoader]]:
r"""

Return a dataloader. It will not be called every epoch unless you set
```Trainer(reload_dataloaders_every_epoch=True)```.
```Trainer(reload_train_dataloader_every_epoch=True)```.

It's recommended that all data downloads and preparation happen in prepare_data().

Expand Down
2 changes: 1 addition & 1 deletion pytorch_lightning/trainer/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ def on_train_end(self):
trainer = Trainer(progress_bar_refresh_rate=1)


reload_dataloaders_every_epoch
reload_train_dataloader_every_epoch
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Set to True to reload dataloaders every epoch.

Expand Down
6 changes: 3 additions & 3 deletions pytorch_lightning/trainer/evaluation_loop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class TrainerEvaluationLoopMixin(ABC):
test_dataloaders: DataLoader
val_dataloaders: DataLoader
use_tpu: bool
reload_dataloaders_every_epoch: ...
reload_train_dataloader_every_epoch: ...
progress_bar_refresh_rate: ...

# Callback system
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@TevenLeScao TevenLeScao Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@TevenLeScao TevenLeScao Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 !

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@TevenLeScao TevenLeScao Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

@ethanwharris is this also solved?

Copy link
Contributor Author

@TevenLeScao TevenLeScao Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

self.reset_test_dataloader(model)

dataloaders = self.test_dataloaders
max_batches = self.num_test_batches
else:
# val
if self.reload_dataloaders_every_epoch or self.val_dataloaders is None:
if self.reload_train_dataloader_every_epoch or self.val_dataloaders is None:
self.reset_val_dataloader(model)

dataloaders = self.val_dataloaders
Expand Down
16 changes: 14 additions & 2 deletions pytorch_lightning/trainer/trainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ def __init__(
resume_from_checkpoint: Optional[str] = None,
profiler: Optional[BaseProfiler] = None,
benchmark: bool = False,
reload_train_dataloader_every_epoch: bool = False,
reload_dataloaders_every_epoch: bool = False,
**kwargs
):
Expand Down Expand Up @@ -245,7 +246,11 @@ def __init__(

profiler: To profile individual steps during training and assist in

reload_dataloaders_every_epoch: Set to True to reload dataloaders every epoch
reload_train_dataloader_every_epoch: Set to True to reload dataloaders every epoch

reload_dataloaders_every_epoch:
.. warning:: .. deprecated:: 0.7.2
Use `reload_train_dataloader_every_epoch` instead. Will remove 0.9.0.

benchmark: If true enables cudnn.benchmark.
"""
Expand Down Expand Up @@ -275,7 +280,6 @@ def __init__(
" and this method will be removed in v0.8.0", DeprecationWarning)
self.gradient_clip = gradient_clip

self.reload_dataloaders_every_epoch = reload_dataloaders_every_epoch
self.progress_bar_refresh_rate = progress_bar_refresh_rate
self.check_val_every_n_epoch = check_val_every_n_epoch
self.track_grad_norm = track_grad_norm
Expand Down Expand Up @@ -320,6 +324,14 @@ def __init__(
" NaN grads will be printed automatically when detected.",
DeprecationWarning)

self.reload_train_dataloader_every_epoch = reload_train_dataloader_every_epoch
# Backward compatibility, TODO: remove in v0.9.0
if reload_dataloaders_every_epoch is not None:
warnings.warn("Argument `reload_dataloaders_every_epoch` has renamed to "
"`reload_train_dataloader_every_epoch` since v0.7.2"
" and will be removed in v0.9.0", DeprecationWarning)
self.reload_train_dataloader_every_epoch = reload_dataloaders_every_epoch

self.truncated_bptt_steps = truncated_bptt_steps
self.resume_from_checkpoint = resume_from_checkpoint
self.shown_warnings = set()
Expand Down
13 changes: 7 additions & 6 deletions pytorch_lightning/trainer/training_loop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ class TrainerTrainLoopMixin(ABC):
batch_idx: int
precision: ...
train_dataloader: DataLoader
reload_dataloaders_every_epoch: bool
reload_train_dataloader_every_epoch: bool
progress_bar_refresh_rate: ...
max_steps: int
min_steps: int
Expand Down Expand Up @@ -289,7 +289,9 @@ def train(self):
model = self.get_model()

# load data
self.reset_train_dataloader(model)
# if reload_train_dataloader_every_epoch, this is moved to the epoch loop
if not self.reload_train_dataloader_every_epoch:
self.reset_train_dataloader(model)
self.reset_val_dataloader(model)
Borda marked this conversation as resolved.
Show resolved Hide resolved
Borda marked this conversation as resolved.
Show resolved Hide resolved

# Train start events
Expand All @@ -305,6 +307,9 @@ def train(self):
try:
# run all epochs
for epoch in range(self.current_epoch, self.max_epochs):
# reset train dataloader
if self.reload_train_dataloader_every_epoch:
self.reset_train_dataloader(model)
# set seed for distributed sampler (enables shuffling for each epoch)
if self.use_ddp \
and hasattr(self.train_dataloader.sampler, 'set_epoch'):
Expand Down Expand Up @@ -393,10 +398,6 @@ def run_training_epoch(self):
if self.is_function_implemented('on_epoch_start'):
self.get_model().on_epoch_start()

# reset train dataloader
if self.reload_dataloaders_every_epoch:
self.reset_train_dataloader(self.get_model())

# track local dataloader so TPU can wrap each epoch
train_dataloader = self.train_dataloader

Expand Down