Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

collision between different cl arg definitions in examples #6310

Closed
stas00 opened this issue Aug 6, 2020 · 2 comments
Closed

collision between different cl arg definitions in examples #6310

stas00 opened this issue Aug 6, 2020 · 2 comments
Labels

Comments

@stas00
Copy link
Contributor

stas00 commented Aug 6, 2020

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: #6027 and #6307

@stas00 stas00 changed the title collision between different arg definitions in examples collision between different cl arg definitions in examples Aug 7, 2020
@stas00
Copy link
Contributor Author

stas00 commented Aug 9, 2020

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.

@stale
Copy link

stale bot commented Oct 10, 2020

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.

@stale stale bot added the wontfix label Oct 10, 2020
@stale stale bot closed this as completed Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant