Fix Config.to_args for list values and False booleans by MohammadHijjawi97 · Pull Request #1806 · modelscope/modelscope · GitHub
Skip to content

Fix Config.to_args for list values and False booleans - #1806

Open
MohammadHijjawi97 wants to merge 1 commit into
modelscope:masterfrom
MohammadHijjawi97:fix/config-to-args-list-and-false
Open

MohammadHijjawi97 wants to merge 1 commit into
modelscope:masterfrom
MohammadHijjawi97:fix/config-to-args-list-and-false

Conversation

@MohammadHijjawi97

Copy link
Copy Markdown

Config.to_args documents that config values may be bool, int, str, float or a list of them, but two of these cases are broken:

  • Lists always fail. The element-type assertion is applied to the list itself (isinstance(v, (int, str, float, bool))), so any list value raises AssertionError: Element type in list is expected to be either int,str,float, but got type 0.
  • False booleans produce invalid args. bool is a subclass of int, so False falls through to the int branch and emits --flag False. argparse rejects that for a store_true flag (error: unrecognized arguments: False).

Reproduction:

import argparse
from modelscope.utils.config import Config

def parse_fn(a):
    p = argparse.ArgumentParser()
    p.add_argument('--gpus', type=int, nargs='+')
    p.add_argument('--fp16', action='store_true')
    return p.parse_args(a)

Config(dict(gpus=[0, 1])).to_args(parse_fn)   # AssertionError
Config(dict(fp16=False)).to_args(parse_fn)    # SystemExit: unrecognized arguments: False

Fix:

  • check the type of each list element, and pass the elements as separate arguments so they work with nargs;
  • omit the flag when a bool is False.

Test: added test_to_args_with_list_and_bool to tests/utils/test_config.py; it fails on master and passes with this change. flake8, isort and yapf are clean on the changed files.

`Config.to_args` documents support for bool, int, str, float and lists
of them, but:

- any list value always failed, because the element-type assertion was
  applied to the list itself instead of its items;
- a `False` bool fell through to the int branch (bool is a subclass of
  int) and produced `--flag False`, which argparse rejects for a
  store_true flag.

Check the type of each list element and pass the elements as separate
arguments so they work with `nargs`, and omit the flag for `False`.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant