Skip to content

Add types and improve descriptions to ArgumentParser parameters #4724

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

Merged
merged 6 commits into from
Oct 24, 2021

Conversation

puhuk
Copy link
Contributor

@puhuk puhuk commented Oct 22, 2021

To resolve issue #4694, add the missing ArgumentParser types

cc @datumbox

@datumbox
Copy link
Contributor

@puhuk Note that there are multiple missing types. I just provide a handful of examples on the issue but it would be good to check all of them. Moreover, it might be good to update the descriptions to some parameters that don't have enough info. Have a look and let me know.

@puhuk
Copy link
Contributor Author

puhuk commented Oct 22, 2021

@datumbox, Ah, ok got it :) Let me resend after looking them all.

Add missing types on argument parser
@puhuk
Copy link
Contributor Author

puhuk commented Oct 22, 2021

I resend PR but some checks failed. (build, ci/circle: python_lint, nightly)
Is there anything I should do before sending PR?

@datumbox
Copy link
Contributor

datumbox commented Oct 22, 2021

@puhuk You should run ufmt format references from the root of your repo to format the folder.

@NicolasHug I wonder if there is a way we can make the ufmt test show were the failures occurred. Is this possible?

formatted with ufmt
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@puhuk Thanks for the PR. I've added a few indicative comments.

I was hoping that we could look holistically the parameters and the descriptions across all scripts (including similarity and video_classification) and ensure that a) the parameters are properly validated by correct types, b) the descriptions are consistent across scripts c) they are intuitive and short. Naming is NP hard, so definitely not an easy task...

Let me know your thoughts.

parser.add_argument("--device", default="cuda", help="device")
parser.add_argument("--data-path", default="/datasets01/imagenet_full_size/061417/", type=str, help="dataset path")
parser.add_argument("--model", default="resnet18", type=str, help="model name")
parser.add_argument("--device", default="cuda", type=str, help="device (cuda for GPU use, cpu for not using GPU")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the following is clearer:

Suggested change
parser.add_argument("--device", default="cuda", type=str, help="device (cuda for GPU use, cpu for not using GPU")
parser.add_argument("--device", default="cuda", type=str, help="device (Use cuda or cpu Default: cuda")

parser.add_argument("--output-dir", default=".", help="path where to save")
parser.add_argument("--resume", default="", help="resume from checkpoint")
parser.add_argument("--output-dir", default=".", type=str, help="path to save outputs")
parser.add_argument("--resume", default="", type=str, help="path for resume checkpoint from")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

Suggested change
parser.add_argument("--resume", default="", type=str, help="path for resume checkpoint from")
parser.add_argument("--resume", default="", type=str, help="path of checkpoint")

parser.add_argument("--data-path", default="/datasets01/COCO/022719/", type=str, help="dataset path")
parser.add_argument("--dataset", default="coco", type=str, help="dataset name")
parser.add_argument("--model", default="maskrcnn_resnet50_fpn", type=str, help="model name")
parser.add_argument("--device", default="cuda", type=str, help="device (cuda for GPU use, cpu for not using GPU")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

parser.add_argument("--output-dir", default=".", help="path where to save")
parser.add_argument("--resume", default="", help="resume from checkpoint")
parser.add_argument("--output-dir", default=".", type=str, help="path where to save")
parser.add_argument("--resume", default="", type=str, help="path for resume checkpoint from")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -92,15 +94,17 @@ def get_args_parser(add_help=True):
"--lr-gamma", default=0.1, type=float, help="decrease lr by a factor of lr-gamma (multisteplr scheduler only)"
)
parser.add_argument("--print-freq", default=20, type=int, help="print frequency")
parser.add_argument("--output-dir", default=".", help="path where to save")
parser.add_argument("--resume", default="", help="resume from checkpoint")
parser.add_argument("--output-dir", default=".", type=str, help="path where to save")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed your earlier proposal for different help message here.

parser.add_argument("--aux-loss", action="store_true", help="auxiliar loss")
parser.add_argument("--device", default="cuda", help="device")
parser.add_argument("-b", "--batch-size", default=8, type=int)
parser.add_argument("--device", default="cuda", type=str, help="device (cuda for GPU use, cpu for not using GPU")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

parser.add_argument("--device", default="cuda", help="device")
parser.add_argument("-b", "--batch-size", default=8, type=int)
parser.add_argument("--device", default="cuda", type=str, help="device (cuda for GPU use, cpu for not using GPU")
parser.add_argument("-b", "--batch-size", default=8, type=int, help="size of batch")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we align this with the values of the other scripts? See detection for instance.

parser.add_argument("--output-dir", default=".", help="path where to save")
parser.add_argument("--resume", default="", help="resume from checkpoint")
parser.add_argument("--output-dir", default=".", type=str, help="path where to save")
parser.add_argument("--resume", default="", type=str, help="path for resume checkpoint from")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -227,8 +227,8 @@ def get_args_parser(add_help=True):
parser.add_argument("--lr-warmup-method", default="linear", type=str, help="the warmup method (default: linear)")
parser.add_argument("--lr-warmup-decay", default=0.01, type=float, help="the decay for lr")
parser.add_argument("--print-freq", default=10, type=int, help="print frequency")
parser.add_argument("--output-dir", default=".", help="path where to save")
parser.add_argument("--resume", default="", help="resume from checkpoint")
parser.add_argument("--output-dir", default=".", type=str, help="path where to save")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Updated with review
@puhuk
Copy link
Contributor Author

puhuk commented Oct 23, 2021

I see :)
I send again with a) other files including arguments and b) whether descriptions are consistent between files.
If there is anything I missed, please just let me know.
Looking forward to getting review :)

Add train.py from video_classification, similarity and train_quantization.py
@datumbox datumbox changed the title Add type to default argument (To resolve issue #4694) Add types and improve descriptions to ArgumentParser parameters Oct 24, 2021
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@datumbox datumbox merged commit 9ae833a into pytorch:main Oct 24, 2021
facebook-github-bot pushed a commit that referenced this pull request Oct 26, 2021
…ers (#4724)

Summary:
* Add type to default argument

To resolve issue #4694

* Resolve issue #4694

Add missing types on argument parser

* Update with ufmt

formatted with ufmt

* Updated with review

Updated with review

* Update type of arguments

Add train.py from video_classification, similarity and train_quantization.py

Reviewed By: NicolasHug

Differential Revision: D31916335

fbshipit-source-id: a717a3cac868b567db57b84a545ad9363820179b

Co-authored-by: Vasilis Vryniotis <[email protected]>
@puhuk puhuk deleted the issue_4694 branch November 3, 2021 23:33
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
…rch#4724)

* Add type to default argument

To resolve issue pytorch#4694

* Resolve issue pytorch#4694

Add missing types on argument parser

* Update with ufmt

formatted with ufmt

* Updated with review

Updated with review

* Update type of arguments

Add train.py from video_classification, similarity and train_quantization.py

Co-authored-by: Vasilis Vryniotis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants