Repository: Lightning-AI/pytorch-lightning
Pull request: check omegaconf gpus
## What does this PR do?

Fixes #2270

# Before submitting

- [ ] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
- [ ] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), Pull Request section?
- [ ] Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you create a separate PR for every change.
- [x] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests? 
- [ ] Did you verify new and existing tests pass locally with your changes?
- [ ] If you made a notable change (that affects users), did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)?

<!-- For CHANGELOG separate each item in unreleased section by a blank line to reduce collisions -->

## PR review    
Anyone in the community is free to review the PR once the tests have passed.     
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

## Did you have fun?
Make sure you had fun coding 🙃


Problem statement:
Exception when trainer.gpus are passed as a list through hydra (omegaconf)
## 🐛 Bug
When we pass the `gpus` flag for `Trainer` using `hydra` (which in turn uses `Omegaconf`) it is passed as `omegaconf.listconfig.ListConfig`. However, internally `Trainer` only checks for `int`, `str`, `list`. This throws an exception. 

#### Code sample
```
#!/usr/bin/env python

import warnings
warnings.filterwarnings("ignore")

import os

import torch
from torch import nn
from torch.utils.data import DataLoader, random_split
from torchvision.datasets import MNIST
from torchvision import datasets, transforms
from torch.nn import functional as F
from torch import optim

from pytorch_lightning.core.lightning import LightningModule
from pytorch_lightning import Trainer
from omegaconf import OmegaConf

class LitMNIST(LightningModule):
  def __init__(self):
      super().__init__()
      self.layer_1 = torch.nn.Linear(28 * 28, 128)
      self.layer_2 = torch.nn.Linear(128, 256)
      self.layer_3 = torch.nn.Linear(256, 10)  
      
  def prepare_data(self):
    transform=transforms.Compose([transforms.ToTensor(), transforms.Normalize((0.1307,), (0.3081,))])
    # transform
    mnist_train = MNIST(os.getcwd(), train=True, download=False, transform=transforms.ToTensor())
    
    # train/val split
    mnist_train, mnist_val = random_split(mnist_train, [55000, 5000])

    # assign to use in dataloaders
    self.train_dataset = mnist_train
    self.val_dataset = mnist_val
    
  def forward(self, x):
      batch_size, channels, width, height = x.size()
      x = x.view(batch_size, -1)
      x = self.layer_1(x)
      x = torch.relu(x)
      x = self.layer_2(x)
      x = torch.relu(x)
      x = self.layer_3(x)
      x = torch.log_softmax(x, dim=1)
      return x    
    
  def training_step(self, batch, batch_idx):
      x, y = batch
      logits = self(x)
      loss = F.nll_loss(logits, y)

      # add logging
      logs = {'loss': loss}
      return {'loss': loss, 'log': logs}    
    
  def validation_step(self, batch, batch_idx):
    x, y = batch
    logits = self(x)
    loss = F.nll_loss(logits, y)
    return {'val_loss': loss}    

  def train_dataloader(self):
    return DataLoader(self.train_dataset, batch_size=64)
  
  def val_dataloader(self):
    return DataLoader(self.val_dataset, batch_size=64) 
  
  def configure_optimizers(self):
    return optim.Adam(self.parameters(), lr=1e-3)    

if __name__=='__main__':
#   trainer_params = OmegaConf.create({'max_epochs': 1, 'overfit_pct': 0.01, 'gpus': 1}) # this works
#   trainer_params = OmegaConf.create({'max_epochs': 1, 'overfit_pct': 0.01, 'gpus': [1]}) # this throws an exception about gpus not being int, str, or list(int)
  model = LitMNIST()
  trainer = Trainer(**trainer_params)
  trainer.fit(model)
```

Hints from issue discussion:
well it is OmegaConf issue
```
trainer_params = OmegaConf.create({'max_epochs': 1, 'overfit_pct': 0.01, 'gpus': [1]})
type(trainer_params.gpus)
Out[1]: omegaconf.listconfig.ListConfig
isinstance(trainer_params.gpus, list)
Out[2]: False
```
in fact, the solution shall be that `ListConfig` is inherited from `list`, so no change needed for PL
@omry ^^ @williamFalcon 
cc: @PyTorchLightning/core-contributors 
@Borda : 

Inheriting from primitive containers is not recommended.
Python is making a ton of assumptions about list and dict that would break if the objects are not actually what it expects.
Here is a [post](https://treyhunner.com/2019/04/why-you-shouldnt-inherit-from-list-and-dict-in-python/) by someone talking about some of the problems.

As of OmegaConf 2.0 ListConfig is [implementing MutableSequence](https://github.com/omry/omegaconf/blob/master/omegaconf/listconfig.py#L38). and DictConfig is [implementing MutableMapping](https://github.com/omry/omegaconf/blob/master/omegaconf/dictconfig.py#L47) to make it easier to identify them in a way that works for lists and dicts as well.

Additionally:
OmegaConf is very low level, making such changes there is a huge deal. This kind of change will require a major version. (not happening in the next 6 months, and probably not happening at all for the reasons I outlined above).
```python
In [1]: from omegaconf import OmegaConf
In [2]: trainer_params = OmegaConf.create({'max_epochs': 1, 'overfit_pct': 0.01, 'gpus': [1]})
In [3]: isinstance(trainer_params.gpus, list)
Out[3]: False
In [4]: from typing import MutableSequence
In [5]: isinstance(trainer_params.gpus, MutableSequence)
Out[5]: True
In [6]: isinstance([1,2,3], MutableSequence)
Out[6]: True
```

Please note that this is only working with OmegaConf 2.0, which means this is only working with Hydra 1.0.0rc1 or newer (which is still a release candidate, most Hydra users are still using the stable version).
However, it looks like the PL support for Hydra is already assuming 1.0 so we might as well continue to assume it.
```
In[8]: from typing import List
In[9]: isinstance(trainer_params.gpus, List)
Out[9]: False
```
ok, we can replace check of `list` to `MutableSequence`
@omry we want to support this but this is now scattering a lot of code everywhere for special cases.

Unfortunately, I think this is as much effort as we can put into this at this time. Please support Lists in Omegaconf and i would also suggest to supprot --args

in the meantime @sudarshan85, i'm sorry but it looks like this is incompatible with lightning at the moment. Please use a regular argparse or maybe pass in a string for the GPUs instead of list.
I looked at implementing typing,List at some point while working on OmegaConf 2.0 and decided against it.
I just tested again and it's causing issues around copy and deepcopy , and around pickling.
I can revisit it for the next major version but I can't guarantee that I will change it.
Even if I do end up making this change - it may be as long as 6 months before it's available to Hydra users.
@williamFalcon @omry 
No worries. The only problem is if I pass in an `int` enclosed quotes to force a string such as '1' on the command line using Hydra, it automatically converts it to an int and does not read it as a string.  Is there way to force hydra to get a string out of it?
@sudarshan85 
Join the [Hydra chat](https://hydra-framework.zulipchat.com/#) if you have questions about Hydra.
@omry @sudarshan85 I have changed the checking `list` to `MutableSequence` in #2273 which shall resolve this issue, pls have look...

Commit message: test

Patch to review:
diff --git a/pytorch_lightning/core/saving.py b/pytorch_lightning/core/saving.py
index e6c7b0e222274..4ff019a20c6ae 100644
--- a/pytorch_lightning/core/saving.py
+++ b/pytorch_lightning/core/saving.py
@@ -6,20 +6,18 @@
 import torch
 import yaml
 from argparse import Namespace
-from typing import Union, Dict, Any, Optional, Callable
+from typing import Union, Dict, Any, Optional, Callable, MutableMapping
 
 from pytorch_lightning import _logger as log
 from pytorch_lightning.utilities import rank_zero_warn, AttributeDict
 from pytorch_lightning.utilities.cloud_io import load as pl_load
 
 PRIMITIVE_TYPES = (bool, int, float, str)
-ALLOWED_CONFIG_TYPES = (AttributeDict, dict, Namespace)
+ALLOWED_CONFIG_TYPES = (AttributeDict, MutableMapping, Namespace)
 try:
     from omegaconf import Container
 except ImportError:
     Container = None
-else:
-    ALLOWED_CONFIG_TYPES = ALLOWED_CONFIG_TYPES + (Container, )
 
 # the older shall be on the top
 CHECKPOINT_PAST_HPARAMS_KEYS = (
diff --git a/pytorch_lightning/trainer/distrib_parts.py b/pytorch_lightning/trainer/distrib_parts.py
index df1c9afe43933..360358f1acce4 100644
--- a/pytorch_lightning/trainer/distrib_parts.py
+++ b/pytorch_lightning/trainer/distrib_parts.py
@@ -10,7 +10,7 @@
 import time
 import random
 import torch
-from typing import Union, Callable, Any, List, Optional, Tuple
+from typing import Union, Callable, Any, List, Optional, Tuple, MutableSequence
 
 from pytorch_lightning.core.lightning import LightningModule
 from pytorch_lightning import _logger as log
@@ -356,14 +356,14 @@ def check_gpus_data_type(gpus: Any) -> None:
     Args:
         gpus: parameter as passed to the Trainer
     """
-    if gpus is not None and (not isinstance(gpus, (int, str, list)) or isinstance(gpus, bool)):
-        raise MisconfigurationException("GPUs must be int, string or list of ints or None.")
+    if gpus is not None and (not isinstance(gpus, (int, str, MutableSequence)) or isinstance(gpus, bool)):
+        raise MisconfigurationException("GPUs must be int, string or sequence of ints or None.")
 
 
 def normalize_parse_gpu_input_to_list(gpus: Union[int, List[int]]) -> Optional[List[int]]:
     assert gpus is not None
-    if isinstance(gpus, list):
-        return gpus
+    if isinstance(gpus, MutableSequence):
+        return list(gpus)
 
     # must be an int
     if not gpus:  # gpus==0
diff --git a/tests/trainer/test_trainer.py b/tests/trainer/test_trainer.py
index 6574e5094011b..42a91865d4597 100644
--- a/tests/trainer/test_trainer.py
+++ b/tests/trainer/test_trainer.py
@@ -10,6 +10,7 @@
 import cloudpickle
 import pytest
 import torch
+from omegaconf import OmegaConf
 
 import tests.base.utils as tutils
 from pytorch_lightning import Callback, LightningModule, Trainer
@@ -905,6 +906,15 @@ def __init__(self, **kwargs):
         TrainerSubclass(abcdefg='unknown_arg')
 
 
+@pytest.mark.parametrize('trainer_params', [
+    OmegaConf.create({'max_epochs': 1, 'gpus': 1}),
+    OmegaConf.create({'max_epochs': 1, 'gpus': [0]}),
+])
+@pytest.mark.skipif(not torch.cuda.is_available(), reason="test requires GPU machine")
+def test_trainer_omegaconf(trainer_params):
+    _ = Trainer(**trainer_params)
+
+
 def test_trainer_pickle(tmpdir):
     trainer = Trainer(
         max_epochs=1,


Code review comments:
diff_hunk: @@ -905,6 +906,15 @@ def __init__(self, **kwargs):
         TrainerSubclass(abcdefg='unknown_arg')
 
 
+@pytest.mark.parametrize('trainer_params', [
+    OmegaConf.create({'max_epochs': 1, 'gpus': 1}),
+    OmegaConf.create({'max_epochs': 1, 'gpus': [0]}),
+])
+@pytest.mark.skipif(not torch.cuda.is_available(), reason="test requires GPU machine")
+def test_trainer_omegaconf(trainer_params):
+    _ = Trainer(**trainer_params)
path: tests/trainer/test_trainer.py
text: @user1:
I personally prefer to just call the constructor in such cases (your call of course).
```suggestion
    Trainer(**trainer_params)
```