collision between different cl arg definitions in examples
Author: stas00
Pull request: no
Labels: name: wontfix

 The `examples` have an incosistency of how the cl args are defined and parsed. Some rely on PL's main args as `finetune.py` does: https://github.com/huggingface/transformers/blob/master/examples/seq2seq/finetune.py#L410 

```
    parser = argparse.ArgumentParser()
    parser = pl.Trainer.add_argparse_args(parser)
```

others like `run_pl_glue.py` rely on `lightening_base.py`'s main args: https://github.com/huggingface/transformers/blob/master/examples/text-classification/run_pl_glue.py#L176

```
    parser = argparse.ArgumentParser()
    add_generic_args(parser, os.getcwd())
```
now that we pushed `--gpus` into  `lightening_base.py`'s main args the scripts that run PL's main args collide and we have:

```
fail.argparse.ArgumentError: argument --gpus: conflicting option string: --gpus
```

i.e. PL already supplies `--gpus` and many other args that some of the scripts in `examples` re-define. 

So either the example scripts need to stop using `pl.Trainer.add_argparse_args(parser)` and rely exclusively on `lightning_base.add_generic_args`, or we need a different clean approach. It appears that different scripts have different needs arg-wise. But they all use `lightning_base`.

The problem got exposed in: https://github.com/huggingface/transformers/pull/6027 and https://github.com/huggingface/transformers/pull/6307

Comments:
Here is a potential idea of how to keep all the common cl arg definitions in `BaseTransformer` and then let each example subclass tell which ones it wants to support, w/o needing to duplicate the same thing everywhere.
```
import argparse

# removes an option from the parser after parser.add_argument's are all done
#https://stackoverflow.com/a/49753634/9201239
def remove_option(parser, arg):
    for action in parser._actions:
        if (vars(action)['option_strings']
            and vars(action)['option_strings'][0] == arg) \
                or vars(action)['dest'] == arg:
            parser._remove_action(action)

    for action in parser._action_groups:
        vars_action = vars(action)
        var_group_actions = vars_action['_group_actions']
        for x in var_group_actions:
            if x.dest == arg:
                var_group_actions.remove(x)
                return

# another way to remove an arg, but perhaps incomplete
#parser._handle_conflict_resolve(None, [('--bar',parser._actions[2])])

# tell the parser which args to keep (the rest will be removed)
def keep_arguments(parser, supported_args):
    for act in parser._actions:
        arg = act.dest
        if not arg in supported_args:
            remove_option(parser, arg)

parser = argparse.ArgumentParser()

# superclass can register all kinds of options
parser.add_argument('--foo', help='foo argument', required=False)
parser.add_argument('--bar', help='bar argument', required=False)
parser.add_argument('--tar', help='bar argument', required=False)

# then a subclass can choose which of them it wants/can support
supported_args = ('foo bar'.split()) # no --tar please

keep_arguments(parser, supported_args)

args = parser.parse_args()
```

Granted, there is no public API to remove args once registered. This idea uses a hack that taps into an internal API.

----

Alternatively, `BaseTransformer` could maintain a dict of all the common args with help/defaults/etc w/o registering any of them, and then the subclass can just tell it which cl args it wants to be registered. This will be just a matter of formatting the dict and then a subclass would call:

```
# a potential new function to be called by a subclass 
register_arguments(parser, 'foo bar'.split())
```
or if no abstraction is desired it could go as explicit as:
```
defs = self.args_def() # non-existing method fetching the possible args
parser.add_argument(defs['foo'])
parser.add_argument(defs['bar'])
```
but this probably defeats the purpose, just as well copy the whole thing.

---


One thing to consider in either solution is that a subclass may want to have different defaults, so the new API could provide for defaults override as well.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.