Skip to content
New issue

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

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

Already on GitHub? # to your account

[Fix] use hyphen for command line args in demo & tools #808

Merged
merged 5 commits into from
Mar 28, 2022

Conversation

nijkah
Copy link
Contributor

@nijkah nijkah commented Mar 21, 2022

closes #804

@mm-assistant mm-assistant bot added the size/XS label Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #808 (264ada9) into master (50cb254) will increase coverage by 0.06%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
+ Coverage   83.04%   83.10%   +0.06%     
==========================================
  Files         216      217       +1     
  Lines       12239    12265      +26     
  Branches     1975     1981       +6     
==========================================
+ Hits        10164    10193      +29     
+ Misses       1767     1763       -4     
- Partials      308      309       +1     
Flag Coverage Δ
unittests 83.06% <92.30%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmedit/utils/cli.py 90.90% <90.90%> (ø)
mmedit/utils/__init__.py 100.00% <100.00%> (ø)
mmedit/datasets/builder.py 93.22% <0.00%> (-1.61%) ⬇️
mmedit/datasets/pipelines/augmentation.py 98.49% <0.00%> (ø)
mmedit/datasets/pipelines/blur_kernels.py 84.55% <0.00%> (+1.47%) ⬆️
mmedit/apis/train.py 30.08% <0.00%> (+12.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50cb254...264ada9. Read the comment docs.

@Yshuo-Li
Copy link
Collaborator

Yshuo-Li commented Mar 21, 2022

Good job.

Maybe markdown files such as docs/en/demo.md also need to be fixed.

@Yshuo-Li
Copy link
Collaborator

We can find all --xx_x by the following command

grep -rn -E -- '--[[:alnum:]]+_' docs

@nijkah
Copy link
Contributor Author

nijkah commented Mar 22, 2022

We can find all --xx_x by the following command

grep -rn -E -- '--[[:alnum:]]+_' docs

I ran

grep -e "--\w*_" -rn demo/*.py 
grep -e "--\w*_" -rn tools
grep -e "--\w*_" -rn docs

and added code to support previous arguments.
And Lint

@wangruohui
Copy link
Member

wangruohui commented Mar 22, 2022

Would the following implementation be better? We can use only hyphens in argparse, and add an additional function to check and modify underscore _ in-place in sys.argv. By doing so, the code would be kept clean.

We can put this function to mmedit/utils/cli.py and import them in demo scripts.

The following code is a simple test case, save to arg.py and test with the command line.

import warnings
import sys
import re


def parse_args():
    parser = argparse.ArgumentParser(description="Generation demo")
    parser.add_argument("--a-b", help="test config file path")
    parser.add_argument("--c-d-e", help="test config file path")
    parser.add_argument("--f", help="test config file path")
    args = parser.parse_args()
    return args


def main():
    print(sys.argv)
    modify_args()
    print(sys.argv)
    args = parse_args()
    print(args)


def modify_args():
    for i, v in enumerate(sys.argv):
        if i == 0:
            assert v.endswith(".py")
        elif re.match(r"--\w+_.*", v):
            new_arg = v.replace("_", "-")
            warnings.warn(
                f"command line argument {v} is deprecated, please use {new_arg} instead.",
                category=DeprecationWarning,
            )
            sys.argv[i] = new_arg


if __name__ == "__main__":
    main()

@wangruohui wangruohui changed the title Fix demo args to use hyphen [Fix] use hyphen in demo command line args Mar 22, 2022
@nijkah
Copy link
Contributor Author

nijkah commented Mar 22, 2022

@wangruohui
I updated it.
Note that local_rank in tools/train.py and tools/test.py is not modified because it is from pytorch's convention.

@wangruohui
Copy link
Member

@nijkah Thank you very much!

@wangruohui wangruohui changed the title [Fix] use hyphen in demo command line args [Fix] use hyphen for command line args in demo & tools Mar 22, 2022
@nijkah
Copy link
Contributor Author

nijkah commented Mar 23, 2022

Should I add a test code for modify_args?

@Yshuo-Li
Copy link
Collaborator

Should I add a test code for modify_args?

Yeah, that will be better.

@wangruohui wangruohui merged commit f0bb84d into open-mmlab:master Mar 28, 2022
Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
* Fix demo args to use hyphen

* Update more files and support depracated name

* Lint

* Add deprecated args util

* add pytest for modify_args
Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
* Fix demo args to use hyphen

* Update more files and support depracated name

* Lint

* Add deprecated args util

* add pytest for modify_args
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[demo args] Fix demo args as to use '-' instead of '_'?
3 participants