Skip to content

(torchx/runopt) Allow runopt type to be builtin list[str] and dict[str,str] #1093

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 1 commit into from
Jul 25, 2025

Conversation

ethanbwaite
Copy link
Contributor

Summary:
As title, supports list[str] and dict[str, str] types in runopt.

Updated testcases to better cover potential type casting issues.

Differential Revision: D78767495

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 23, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78767495

@@ -789,6 +789,48 @@ class runopt:
is_required: bool
help: str

@property
def is_type_list(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

should the property be renamed to 'is_type_list_of_str' since the check ensure the elements are of type str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, updated

return self.opt_type in (List[str], list[str])

@property
def is_type_dict(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to is_type_list, should this be more explicitly named as type_dict_of_str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, updated

…r,str] (#1093)

Summary:

As title, supports `list[str]` and `dict[str, str]` types in runopt.

Updated testcases to better cover potential type casting issues.

Differential Revision: D78767495
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78767495

Copy link
Contributor

@tonykao8080 tonykao8080 left a comment

Choose a reason for hiding this comment

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

looks good

@facebook-github-bot facebook-github-bot merged commit dc70d90 into main Jul 25, 2025
23 of 24 checks passed
@facebook-github-bot facebook-github-bot deleted the export-D78767495 branch July 25, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants