Even when val DataLoader has shuffle=False, Lightning gives an incorrect warning that val Dataloader has shuffle=True

[vineetk1]

🐛 Bug

Even when DataLoader(self.valid_data, shuffle=False, ...), Lightning gives the following message: UserWarning: Your val_dataloader has shuffle=True,.......

To Reproduce

Assign shuffle=False in both the val and test dataloaders. Run the code and see the following two warning messages:

.../.local/lib/python3.8/site-packages/pytorch_lightning/trainer/data_loading.py:659: UserWarning: Your val_dataloader has shuffle=True, it is strongly recommended that you turn this off for val/test/predict dataloaders.

.local/lib/python3.8/site-packages/pytorch_lightning/trainer/data_loading.py:659: UserWarning: Your test_dataloader has shuffle=True, it is strongly recommended that you turn this off for val/test/predict dataloaders.

Expected behavior

There should be no warning messages.

Environment

• PyTorch Lightning Version (e.g., 1.5.0): 1.5.9
• PyTorch Version (e.g., 1.10): 1.10.2
• Python version (e.g., 3.9): 3.8.10
• OS (e.g., Linux): Linux (Ubuntu)
• CUDA/cuDNN version: cu113
• GPU models and configuration: GTX 1080
• How you installed PyTorch (conda, pip, source): pip

Additional context

Previous versions of Lightning also gave those warning messages

cc @justusschock @awaelchli @ninginthecloud @tchaton @rohitgr7 @akihironitta

[carmocca]

Can you reproduce it by modifying the following script?

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/bug_report/bug_report_model.py

[vineetk1]

.local/lib/python3.8/site-packages/pytorch_lightning/trainer/data_loading.py:659 has a code as follows:

def _check_eval_shuffling(dataloader, mode):

    if (
        hasattr(dataloader, "sampler")
        and not isinstance(dataloader.sampler, SequentialSampler)
        and not isinstance(dataloader.dataset, IterableDataset)
    ):
        rank_zero_warn(
            f"Your `{mode.dataloader_prefix}_dataloader` has `shuffle=True`,"
            " it is strongly recommended that you turn this off for val/test/predict dataloaders.",
            category=UserWarning,
        )

The above code does NOT check if shuffle == True, before giving a warning. I think that within the above "if statement" there should be a check for shuffle == True

Following is my val dataloader code where shuffle=False but Lightning still gives a warning:

def val_dataloader(self) -> DataLoader:

    return DataLoader(self.valid_data,
                      batch_size=self.d_params['batch_size']['val'],
                      shuffle=False,
                      sampler=RandomSampler(self.valid_data),
                      batch_sampler=None,
                      num_workers=6,
                      collate_fn=self.gpt2_collater,
                      pin_memory=True,
                      drop_last=False,
                      timeout=0)

[ZhaofengWu]

Just curious, why is shuffling recommended against for validation/testing anyway?

[ananthsub]

Just curious, why is shuffling recommended against for validation/testing anyway?

The order in which you process the samples in validation and testing should not matter. Metrics are typically reported on the whole dataset, not per sample. As a result, shuffling the samples here is usually inefficient.

[ZhaofengWu]

The order in which you process the samples in validation and testing should not matter.

This is precisely the reason that I want to shuffle. Because order doesn't matter, having a sampler that groups examples with similar lengths together would speed up validation/testing. Granted, sampler != shuffling, but that's the issue being raised here.

[awaelchli]

@ZhaofengWu to override this behavior today, you should be able to do the following:

from torch.utils.data.sampler import RandomSampler
dataloader = DataLoader(..., sampler=RandomSampler())

On the issue itself, I don't have a preference for Lightning but for personal use cases I would always set shuffle=False myself no matter what Lightning does. I usually want to log a few example predictions (e.g. images) during every validation run so I can compare them on a timeline in the logger and see visually the progression of quality in the predicted pixels. I wouldn't be able to compare them so easily from epoch to epoch if each time the sampling changes. I don't think this is an uncommon practice.

I can also see the opposite argument though. Someone might want to disable shuffling in the validation loop because they are actually training a model there too (yes on the validation set, nothing wrong with that).

Finally, IMO the torch DataLoader should have shuffle=None by default. Why? Because its value should be determined based on whether a sampler is given or not (my exact example above). If pytorch had that, our Trainer could know when the user explicitly sets shuffle = False, and only forces it to False if it previously was unset.

[ZhaofengWu]

@awaelchli I actually use huggingface's LengthGroupedSampler to group together examples with similar lengths, again to speed up validation/testing, so I can't use a RandomSampler. Although having a warning message is not that disruptive anyway.

And on the issue itself, I definitely agree that having a fixed order is nice for analysis, etc. But when not doing analysis, when the validation set size is large, and/or when validation needs to be done frequently, having a faster validation run becomes important. So, personally, I think it'd be best if lightning does not recommend any particular choice over the other here.

[deepbakes]

I also have this issue but it only appears when I use a sampler, in my case a weighted sampler. If I do not use any sampling then no warning is produced.