Repository: Lightning-AI/pytorch-lightning
Pull request: Custom argparser extension with Trainer arguments (argument types added) 
# Before submitting

- [x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
- [x] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), Pull Request section?
- [ ] Did you make sure to update the docs?   
- [x] Did you write any new necessary tests?  
- [x] If you made a notable change (that affects users), did you update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CHANGELOG.md)?

## What does this PR do?
Fixes [#1139 ](https://github.com/PyTorchLightning/pytorch-lightning/issues/1139)

## 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:
Can't cast Trainer automatically generated args to their required types
## ❓ Questions and Help

#### What is your question?
I'm not sure, that this is a bug, so I put it like a question.
The problem is: if I want to add the Trainer arguments to my custom `ArgumentParser` object, I call the `add_argparse_args` Trainer classmethod. But this method doesn't cast the Trainer arguments to their required types.
It forces me to cast the arguments by myself. Like so:

```python
trainer_args.update(
        {
            'accumulate_grad_batches': int(trainer_args['accumulate_grad_batches']),
            'train_percent_check': float(trainer_args['train_percent_check']),
            'val_percent_check': float(trainer_args['val_percent_check']),
            'val_check_interval': int(trainer_args['val_check_interval']),
            'track_grad_norm': int(trainer_args['track_grad_norm']),
            'max_epochs': int(trainer_args['max_epochs']),
            'precision': int(trainer_args['precision']),
            'gradient_clip_val': float(trainer_args['gradient_clip_val']),
        }
)
```
And after that, I can pass updated arguments to the Trainer:

```python
trainer = pytorch_lightning.Trainer(
        **trainer_args
)
```

And I can't find a central place, where the Trainer handles an automatically generated arguments (their types).

#### What have you tried?
I've tried to pass arguments to the trainer without handling their types. For instance, if i'll not cast `accumulate_grad_batches` to the integer type, the exception will be raised:

```
TypeError: Gradient accumulation supports only int and dict types
```

#### What's your environment?

 - OS: [Linux]
 - Packaging [pip]
 - Version [0.7.1]

Hints from issue discussion:
@alexeykarnachev could you pls give a complete example, I am missing which is the `ArgumentParser`... here it seems like you want to update the `trainer_args` by itself
```
trainer_args.update({'accumulate_grad_batches': int(trainer_args['accumulate_grad_batches']), ...)
```
if you need the default values, pls use `default_attributes` 
Yes, sorry for the insufficient details from my side.

Here is a gist:
https://gist.github.com/alexeykarnachev/fd010b797d92873905780c856e9334d5

It's not runnable script, because I've simplified it a lot. But it contains the problem i've described.


If I'll execute this script with any Trainer argument, for example:

`python script.py --my_custom_arg_1=1 --my_custom_arg_n=2 --accumulate_grad_batches=1`

It will fail without the types casting, because `accumulate_grad_batches` will be interpreted as a string, and Trainer will fail.
if do understand your use-case you want to have some parameters to be passed from CMD and/but some Trainer arguments to be fixed - forced to have a certain value, right?
```
fixed_args = {
            'logger': tb_logger_callback,
            'checkpoint_callback': model_checkpoint_callback,
            'show_progress_bar': True,
            'progress_bar_refresh_rate': 1,
            'row_log_interval': 1,
}
trainer_args = vars(args)
trainer_args.update(fixed_args)
```
but in the case of `accumulate_grad_batches` (and some others) we shall do some casting eg with `eval`
Woudl you mind sending a PR to fix `Trainer.from_argparse_args(...)` and/or `Trainer.add_argparse_args(...)` containing also type?
Yes, I'll do it.

But do you have any suggestions of how can it be performed ?
I see it like this:
in the `Trainer.add_argparse_args` classmethod there is a loop:
```
        for arg in trainer_default_params:
            parser.add_argument(
                f'--{arg}',
                default=trainer_default_params[arg],
                dest=arg,
                help='autogenerated by pl.Trainer'
            )
```
And it's possible to pass a `type` argument to the `parser.add_argument`, but this "type" first needs to be obtained somehow.
I suppose, that it can be done via the signature inspection in the `Trainer.default_attributes` classmethod.
Please, consider a PR https://github.com/PyTorchLightning/pytorch-lightning/pull/1147

Commit message: typo fixed

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4e17728f9424d..b0563977f14d6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,7 +25,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 
--
+- `Trainer.add_argparse_args` classmethod fixed. Now it adds a type for the arguments ([#1147](https://github.com/PyTorchLightning/pytorch-lightning/pull/1147)).
 
 ## [0.7.1] - 2020-03-07
 
diff --git a/pl_examples/full_examples/semantic_segmentation/models/unet/model.py b/pl_examples/full_examples/semantic_segmentation/models/unet/model.py
index 6f8df926c17d0..36890aa95c9c0 100644
--- a/pl_examples/full_examples/semantic_segmentation/models/unet/model.py
+++ b/pl_examples/full_examples/semantic_segmentation/models/unet/model.py
@@ -13,6 +13,7 @@ class UNet(nn.Module):
     bilinear (bool) - Whether to use bilinear interpolation or transposed
     convolutions for upsampling.
     '''
+
     def __init__(self, num_classes=19, bilinear=False):
         super().__init__()
         self.layer1 = DoubleConv(3, 64)
diff --git a/pl_examples/full_examples/semantic_segmentation/models/unet/parts.py b/pl_examples/full_examples/semantic_segmentation/models/unet/parts.py
index 03937846adda8..6828c3f788d70 100644
--- a/pl_examples/full_examples/semantic_segmentation/models/unet/parts.py
+++ b/pl_examples/full_examples/semantic_segmentation/models/unet/parts.py
@@ -8,6 +8,7 @@ class DoubleConv(nn.Module):
     Double Convolution and BN and ReLU
     (3x3 conv -> BN -> ReLU) ** 2
     '''
+
     def __init__(self, in_ch, out_ch):
         super().__init__()
         self.net = nn.Sequential(
@@ -27,6 +28,7 @@ class Down(nn.Module):
     '''
     Combination of MaxPool2d and DoubleConv in series
     '''
+
     def __init__(self, in_ch, out_ch):
         super().__init__()
         self.net = nn.Sequential(
@@ -44,6 +46,7 @@ class Up(nn.Module):
     followed by concatenation of feature map from contracting path,
     followed by double 3x3 convolution.
     '''
+
     def __init__(self, in_ch, out_ch, bilinear=False):
         super().__init__()
         self.upsample = None
diff --git a/pl_examples/full_examples/semantic_segmentation/semseg.py b/pl_examples/full_examples/semantic_segmentation/semseg.py
index 7e512554fd6d5..8f25243cffb96 100644
--- a/pl_examples/full_examples/semantic_segmentation/semseg.py
+++ b/pl_examples/full_examples/semantic_segmentation/semseg.py
@@ -34,6 +34,7 @@ class KITTI(Dataset):
     encoded using `encode_segmap`, and given `transform` (if any) are applied to the image only
     (mask does not usually require transforms, but they can be implemented in a similar way).
     '''
+
     def __init__(
         self,
         root_path,
@@ -120,6 +121,7 @@ class SegModel(pl.LightningModule):
 
     Adam optimizer is used along with Cosine Annealing learning rate scheduler.
     '''
+
     def __init__(self, hparams):
         super(SegModel, self).__init__()
         self.root_path = hparams.root
diff --git a/pytorch_lightning/trainer/trainer.py b/pytorch_lightning/trainer/trainer.py
index 7668a1f6de408..a1891c775d76a 100644
--- a/pytorch_lightning/trainer/trainer.py
+++ b/pytorch_lightning/trainer/trainer.py
@@ -3,33 +3,33 @@
 import sys
 import warnings
 from argparse import ArgumentParser
-from typing import Union, Optional, List, Dict, Tuple, Iterable
+from typing import Union, Optional, List, Dict, Tuple, Iterable, Generator, Any
 
 import torch
-from torch import optim
 import torch.distributed as dist
 import torch.multiprocessing as mp
+from torch import optim
 from torch.optim.optimizer import Optimizer
 from torch.utils.data import DataLoader
 from tqdm.auto import tqdm
 
 from pytorch_lightning.callbacks import Callback
 from pytorch_lightning.callbacks import ModelCheckpoint, EarlyStopping
+from pytorch_lightning.core.lightning import LightningModule
 from pytorch_lightning.loggers import LightningLoggerBase
 from pytorch_lightning.profiler import Profiler, PassThroughProfiler
 from pytorch_lightning.profiler.profiler import BaseProfiler
 from pytorch_lightning.trainer.auto_mix_precision import TrainerAMPMixin
 from pytorch_lightning.trainer.callback_config import TrainerCallbackConfigMixin
+from pytorch_lightning.trainer.callback_hook import TrainerCallbackHookMixin
 from pytorch_lightning.trainer.data_loading import TrainerDataLoadingMixin
+from pytorch_lightning.trainer.deprecated_api import TrainerDeprecatedAPITillVer0_8
 from pytorch_lightning.trainer.distrib_data_parallel import TrainerDDPMixin
 from pytorch_lightning.trainer.distrib_parts import (
     TrainerDPMixin,
     parse_gpu_ids,
     determine_root_gpu_device
 )
-from pytorch_lightning.core.lightning import LightningModule
-from pytorch_lightning.trainer.callback_hook import TrainerCallbackHookMixin
-from pytorch_lightning.trainer.deprecated_api import TrainerDeprecatedAPITillVer0_8
 from pytorch_lightning.trainer.evaluation_loop import TrainerEvaluationLoopMixin
 from pytorch_lightning.trainer.logging import TrainerLoggingMixin
 from pytorch_lightning.trainer.model_hooks import TrainerModelHooksMixin
@@ -449,17 +449,52 @@ def default_attributes(cls):
         return args
 
     @classmethod
-    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
-        """Extend existing argparse by default `Trainer` attributes."""
-        parser = ArgumentParser(parents=[parent_parser], add_help=False)
+    def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
+        r"""Generates arguments, which could extend custom `ArgumentParser` object.
 
-        trainer_default_params = Trainer.default_attributes()
+        Yields:
+            Tuple with 3 values: argument name, argument type and argument default value.
+        """
+        import inspect
+        # Only arguments of these types can extend the `parent_parser`.
+        allowed_types = (str, float, int, bool)
+        trainer_default_params = inspect.signature(cls).parameters
 
-        # TODO: get "help" from docstring :)
         for arg in trainer_default_params:
+            arg_type = trainer_default_params[arg].annotation
+            arg_default = trainer_default_params[arg].default
+            try:
+                possible_arg_types = arg_type.__args__
+            except AttributeError:
+                possible_arg_types = (arg_type,)
+
+            allowed_arg_types = set(possible_arg_types).intersection(allowed_types)
+
+            arg_type = None
+            for allowed_type in allowed_types:
+                if allowed_type in allowed_arg_types:
+                    arg_type = allowed_type
+                    break
+
+            if arg_type is not None:
+                yield arg, arg_type, arg_default
+            else:
+                log.debug(
+                    'Argument %s has no allowed default type hint and will not be added '
+                    'to the arguments parser. Allowed types: %s', (arg, allowed_types)
+                )
+
+    @classmethod
+    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
+        r"""Extend existing argparse by default `Trainer` attributes."""
+        parser = ArgumentParser(parents=[parent_parser], add_help=False, )
+
+        # TODO: get "help" from docstring :)
+        for arg, arg_type, arg_default in cls._get_argparse_args_and_types():
             parser.add_argument(
                 f'--{arg}',
-                default=trainer_default_params[arg],
+                default=arg_default,
+                type=arg_type,
                 dest=arg,
                 help='autogenerated by pl.Trainer'
             )
@@ -879,6 +914,7 @@ class _PatchDataLoader(object):
     Args:
         dataloader: Dataloader object to return when called.
     '''
+
     def __init__(self, dataloader: Union[List[DataLoader], DataLoader]):
         self.dataloader = dataloader
 
diff --git a/tests/trainer/test_trainer.py b/tests/trainer/test_trainer.py
index 38112e1b6be23..9f8c85a4250c6 100644
--- a/tests/trainer/test_trainer.py
+++ b/tests/trainer/test_trainer.py
@@ -251,6 +251,7 @@ def test_dp_output_reduce():
 
 def test_model_checkpoint_options(tmpdir):
     """Test ModelCheckpoint options."""
+
     def mock_save_function(filepath):
         open(filepath, 'a').close()
 
@@ -644,3 +645,49 @@ def test_default_args(tmpdir):
 
     assert isinstance(trainer, Trainer)
     assert trainer.max_epochs == 5
+
+
+@pytest.mark.parametrize(
+    'cli_args',
+    [
+        ['--accumulate_grad_batches=22'],
+        ['--print_nan_grads=1', '--weights_save_path=./'],
+        []
+    ]
+)
+def test_add_argparse_args_redefined(cli_args):
+    """Redefines some default Trainer arguments via the cli and
+    tests the Trainer initialization correctness.
+    """
+    parser = ArgumentParser(add_help=False)
+    parser = Trainer.add_argparse_args(parent_parser=parser)
+
+    args = parser.parse_args(cli_args)
+
+    trainer = Trainer.from_argparse_args(args=args)
+    assert isinstance(trainer, Trainer)
+
+
+@pytest.mark.parametrize(
+    'cli_args',
+    [
+        ['--callbacks=1', '--logger'],
+        ['--foo', '--bar=1']
+    ]
+)
+def test_add_argparse_args_redefined_error(cli_args, monkeypatch):
+    """Asserts thar an error raised in case of passing not default cli arguments."""
+
+    class _UnkArgError(Exception):
+        pass
+
+    def _raise():
+        raise _UnkArgError
+
+    parser = ArgumentParser(add_help=False)
+    parser = Trainer.add_argparse_args(parent_parser=parser)
+
+    monkeypatch.setattr(parser, 'exit', lambda *args: _raise(), raising=True)
+
+    with pytest.raises(_UnkArgError):
+        parser.parse_args(cli_args)


Code review comments:
diff_hunk: @@ -449,17 +449,52 @@ def default_attributes(cls):
         return args
 
     @classmethod
-    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
-        """Extend existing argparse by default `Trainer` attributes."""
-        parser = ArgumentParser(parents=[parent_parser], add_help=False)
+    def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
path: pytorch_lightning/trainer/trainer.py
text: @user1:
is there an advantage to have it as a generator instead of list/tuple?

@author:
Yes, you are right. Fixed

diff_hunk: @@ -449,17 +449,52 @@ def default_attributes(cls):
         return args
 
     @classmethod
-    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
-        """Extend existing argparse by default `Trainer` attributes."""
-        parser = ArgumentParser(parents=[parent_parser], add_help=False)
+    def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
+        r"""Generates arguments, which could extend custom `ArgumentParser` object.
 
-        trainer_default_params = Trainer.default_attributes()
+        Yields:
+            Tuple with 3 values: argument name, argument type and argument default value.
+        """
+        import inspect
path: pytorch_lightning/trainer/trainer.py
text: @user1:
I would move the `inspect` to the beginning of the file as it is standard unless there is special reason

diff_hunk: @@ -449,17 +449,52 @@ def default_attributes(cls):
         return args
 
     @classmethod
-    def add_argparse_args(cls, parent_parser: ArgumentParser) -> ArgumentParser:
-        """Extend existing argparse by default `Trainer` attributes."""
-        parser = ArgumentParser(parents=[parent_parser], add_help=False)
+    def _get_argparse_args_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
path: pytorch_lightning/trainer/trainer.py
text: @user1:
```suggestion
    def _get_init_arguments_and_types(cls) -> Generator[Tuple[str, Any, Any], None, None]:
```
sounds a bit better as it can be used elsewhere not only for `argparser`

@author:
Maybe in this case it’s worth to make this method public?

@author:
And also move the `allowed_types` to the method's input arguments?

@user1:
yes, both are good suggestions, lest doing it...

@author:
I decided to make this method more general. Let's a client decides what arguments he needs. For instance, `add_argparse_args` is a "client" of this method, and it restricts the allowed types by itself. 
What are your thoughts on this?

@user1:
that would be nice, just thinking if is better to defien wanted args/types or ignored ones

diff_hunk: @@ -644,3 +645,49 @@ def test_default_args(tmpdir):
 
     assert isinstance(trainer, Trainer)
     assert trainer.max_epochs == 5
+
+
+@pytest.mark.parametrize(
path: tests/trainer/test_trainer.py
text: @user1:
it starts to be quite long, may consider to move it to a new file like `trainer_cli` along with the existing args testing

@author:
done