Repository: Lightning-AI/pytorch-lightning
Pull request: Avoid enforcing `shuffle=False` for eval dataloaders
## What does this PR do?

<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->

Fixes #11054

### Does your PR introduce any breaking changes? If yes, please list them.

<!-- FILL IN or None -->

## Before submitting

- [x] Was this **discussed/approved** via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] 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)
- [x] Did you write any **new necessary tests**? (not for typos and docs)
- [x] Did you verify new and **existing tests pass** locally with your changes?
- [x] Did you list all the **breaking changes** introduced by this pull request?
- [x] Did you **update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)**? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- In the CHANGELOG, separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

- [x] Is this pull request ready for review? (if not, please submit in draft mode)
- [x] Check that all items from **Before submitting** are resolved
- [x] Make sure the title is self-explanatory and the description concisely explains the PR
- [x] 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 🙃


cc @borda @justusschock @awaelchli @ninginthecloud

Problem statement:
Setting `shuffle=True` for `val_dataloader` doesn't work when `ddp` enabled.
## 🐛 Bug

Setting `shuffle=True` for `val_dataloader` doesn't work when `ddp` enabled.

### To Reproduce

minimum snippet:

```python
from pytorch_lightning import LightningModule, Trainer
from torch import nn
from torch.optim import Adam
from torch.utils.data import DataLoader


class PlModule(LightningModule):
    def __init__(self):
        super().__init__()
        self.model = nn.Linear(1, 1)
        self.train_dataset = list(range(1000))
        self.val_dataset = list(range(1000))

    def configure_optimizers(self):
        return Adam(self.model.parameters(), lr=0.1)

    def training_step(self, batch, *args, **kwargs):
        return self.model(batch)

    def validation_step(self, batch, *args, **kwargs):
        print(f'{batch=}')
        print(f'{self.trainer.val_dataloaders[0].sampler.shuffle=}')
        exit(0)

    def train_dataloader(self):
        return DataLoader(self.train_dataset, shuffle=True, batch_size=5)

    def val_dataloader(self):
        return DataLoader(self.val_dataset, shuffle=True, batch_size=5)



if __name__ == '__main__':
    module = PlModule()
    trainer = Trainer(
        limit_train_batches=0.0,
        limit_val_batches=1.0,
        num_sanity_val_steps=-1,
        gpus=2,
        strategy='ddp',
        enable_model_summary=False
    )
    trainer.fit(module)
```

By execute the snippet above, we get the stdout:

```
GPU available: True, used: True
TPU available: False, using: 0 TPU cores
IPU available: False, using: 0 IPUs
initializing distributed: GLOBAL_RANK: 1, MEMBER: 2/2
initializing distributed: GLOBAL_RANK: 0, MEMBER: 1/2
----------------------------------------------------------------------------------------------------
distributed_backend=nccl
All distributed processes registered. Starting with 2 processes
----------------------------------------------------------------------------------------------------

LOCAL_RANK: 0 - CUDA_VISIBLE_DEVICES: [0,1,2,3,4,5,6,7]
LOCAL_RANK: 1 - CUDA_VISIBLE_DEVICES: [0,1,2,3,4,5,6,7]
Validation sanity check: 0it [00:00, ?it/s]/home/balabala/miniconda3/lib/python3.8/site-packages/pytorch_lightning/trainer/data_loading.py:452: UserWarning: Your `val_dataloader` has `shuffle=True`,it is strongly recommended that you turn this off for val/test/predict dataloaders.
  rank_zero_warn(
/home/balabala/miniconda3/lib/python3.8/site-packages/pytorch_lightning/trainer/data_loading.py:110: UserWarning: The dataloader, val_dataloader 0, does not have many workers which may be a bottleneck. Consider increasing the value of the `num_workers` argument` (try 96 which is the number of cpus on this machine) in the `DataLoader` init to improve performance.
  rank_zero_warn(
Validation sanity check:   0%|                          | 0/100 [00:00<?, ?it/s]batch=tensor([1, 3, 5, 7, 9], device='cuda:1')
self.trainer.val_dataloaders[0].sampler.shuffle=False
batch=tensor([0, 2, 4, 6, 8], device='cuda:0')
self.trainer.val_dataloaders[0].sampler.shuffle=False
```

### Expected behavior

the stdout should contains the follow:

```
batch= # some random batch
self.trainer.val_dataloaders[0].sampler.shuffle=True
```

### Environment

- PyTorch Lightning Version 1.5.2
- PyTorch Version 1.10.0
- Python version 3.8.10



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

Hints from issue discussion:
Out of curiosity, could you describe why you need shuffling during validation? 
> Out of curiosity, could you describe why you need shuffling during validation?

In training, I would like to construct random positive-negative pairs on the fly. In validation, I want to keep the same setting. Of course, I can construct random data before model fitting and fix it, but it's more convenient to do it on the fly, and I don't have to write additional code. Since I would like to choose a large batch size and use this setting for debugging, the randomness the validation setting brings is acceptable.


This is where in the Trainer shuffling is hardcoded to be False for validation, test, and prediction dataloaders: https://github.com/PyTorchLightning/pytorch-lightning/blob/1c5a5c3dfeeefa7b8760ab5f5869f3f61c48e665/pytorch_lightning/trainer/data_loading.py#L312-L313
This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@rohitgr7 Mind having a look.

Commit message: chlog

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index aa7c4f9b056bc..85c499b992142 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -457,6 +457,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Disbled sampler replacement when using `IterableDataset` ([#11507](https://github.com/PyTorchLightning/pytorch-lightning/pull/11507))
 
 
+- Avoid enforcing `shuffle=False` for eval dataloaders ([#11575](https://github.com/PyTorchLightning/pytorch-lightning/pull/11575))
+
+
 ## [1.5.8] - 2022-01-05
 
 ### Fixed
diff --git a/pytorch_lightning/trainer/connectors/data_connector.py b/pytorch_lightning/trainer/connectors/data_connector.py
index 688f6c9a4b658..672ec72c05fef 100644
--- a/pytorch_lightning/trainer/connectors/data_connector.py
+++ b/pytorch_lightning/trainer/connectors/data_connector.py
@@ -435,7 +435,13 @@ def _reset_eval_dataloader(
             )
 
         # add samplers
-        dataloaders = [self._prepare_dataloader(dl, False, mode=mode) for dl in dataloaders if dl is not None]
+        dataloaders = [
+            self._prepare_dataloader(
+                dl, shuffle=hasattr(dl, "sampler") and isinstance(dl.sampler, RandomSampler), mode=mode
+            )
+            for dl in dataloaders
+            if dl is not None
+        ]
 
         # add worker_init_fn for correct seeding in worker processes
         apply_to_collection(
diff --git a/tests/trainer/connectors/test_data_connector.py b/tests/trainer/connectors/test_data_connector.py
index 4d614ecc25f6e..429ac9237ac3b 100644
--- a/tests/trainer/connectors/test_data_connector.py
+++ b/tests/trainer/connectors/test_data_connector.py
@@ -19,6 +19,7 @@
 from pytorch_lightning import Trainer
 from pytorch_lightning.trainer.connectors.data_connector import _DataLoaderSource
 from tests.helpers import BoringDataModule, BoringModel
+from tests.helpers.boring_model import RandomDataset
 
 
 class NoDataLoaderModel(BoringModel):
@@ -66,3 +67,18 @@ def test_dataloader_source_request_from_module():
     module.foo.assert_not_called()
     assert isinstance(source.dataloader(), DataLoader)
     module.foo.assert_called_once()
+
+
+@pytest.mark.parametrize("shuffle", [True, False])
+def test_eval_shuffle_with_distributed_sampler_replacement(shuffle):
+    """Test that shuffle is not changed if set to True."""
+
+    class CustomModel(BoringModel):
+        def val_dataloader(self):
+            return DataLoader(RandomDataset(32, 64), shuffle=shuffle)
+
+    trainer = Trainer(accelerator="cpu", devices=2, strategy="ddp")
+    model = CustomModel()
+    trainer._data_connector.attach_data(model)
+    trainer.reset_val_dataloader(model)
+    assert trainer.val_dataloaders[0].sampler.shuffle == shuffle


Code review comments:
diff_hunk: @@ -435,7 +435,13 @@ def _reset_eval_dataloader(
             )
 
         # add samplers
-        dataloaders = [self._prepare_dataloader(dl, False, mode=mode) for dl in dataloaders if dl is not None]
+        dataloaders = [
+            self._prepare_dataloader(
+                dl, shuffle=hasattr(dl, "sampler") and isinstance(dl.sampler, RandomSampler), mode=mode
path: pytorch_lightning/trainer/connectors/data_connector.py
text: @user1:
Shouldn't this use the same conditions as `_check_eval_shuffling`?

```python
        if (
            hasattr(dataloader, "sampler")
            and not isinstance(dataloader.sampler, SequentialSampler)
            and not isinstance(dataloader.dataset, IterableDataset)
        ):
```

This could be extracted into an utility called used by both pieces of code

@author:
yes, though so but if its a custom sampler, then it would set shuffle=True, which might be incorrect.

@author:
ok, looks like we raise an error in the case of the custom sampler already. will update.