Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[params] Fix parser in python 3.8 #3598

Merged
merged 5 commits into from
Apr 17, 2021
Merged

[params] Fix parser in python 3.8 #3598

merged 5 commits into from
Apr 17, 2021

Conversation

stephenroller
Copy link
Contributor

Patch description
ParlAI likes to use short options like -hs 1024. Since Python 3.8, this has been broken, and gets parsed as -h s 1024.

Although we've patched a few places previously (example), there continue to be little bits and places left. This patch should fix all of them by changing everything to double dashes under the hood.

Testing steps
CI, manual testing.

# handle the single dash stuff. See _handle_single_dash_addarg for info
actions = set()
for action in self._actions:
actions = actions | set(action.option_strings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just add(action)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could use set.update(action.option_strings) but not set.add. option_strings is a list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick timer check shows that's much faster, so I'll update

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I didn't know it is list. It makes sense like this then. Thanks for clarifying.

return args

out_long = []
out_short = []
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why can't they both use the same list? You returned their aggregated version only anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bc i'm being cute and swapping the ordering

Copy link
Contributor Author

@stephenroller stephenroller Apr 17, 2021

Choose a reason for hiding this comment

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

we tend to define options like add_argument('-hs', '--hiddensize') but we need it to add_argument('--hiddensize', '--hs') or otherwise it will try to store the value to opt['hs']

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. That makes sense.

@stephenroller stephenroller merged commit 97e9593 into master Apr 17, 2021
@stephenroller stephenroller deleted the stupidpy3 branch April 17, 2021 03:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants