8000 [PT-D] Multiprocessing API: standard streams: update 'to_std' utility parser to use enum membership testing by jayaddison · Pull Request #93209 · pytorch/pytorch · GitHub
[go: up one dir, main page]

Skip to content

[PT-D] Multiprocessing API: standard streams: update 'to_std' utility parser to use enum membership testing #93209

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

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions torch/distributed/elastic/multiprocessing/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,10 @@ def from_str(cls, vm: str) -> Union["Std", Dict[int, "Std"]]:
Any other input raises an exception
"""

def to_std(v):
v = int(v)
for s in Std:
if s == v:
return s
def to_std(v: str) -> Std:
s = Std(int(v))
if s in Std:
return s
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we raise a error in the else statement to silent mypy?

Copy link
Author

Choose a reason for hiding this comment

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

Could do.. hmm, thinking about it. The intent here is not to change the behaviour of the code in any way.

Since the else should never be reached, then raising an error (like a ValueError) should be fine. In this case it might be preferable to use a # type: ignore[return] comment.

Copy link
Author

Choose a reason for hiding this comment

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

Also: was mypy not complaining about that before these changes? That seems like a possible bug.

Copy link
Author

Choose a reason for hiding this comment

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

Eugh, perhaps it's more complicated. At a guess: maybe mypy was able to infer from the for loop that all values are checked, and it doesn't do the same for the membership test.

Copy link
Author

Choose a reason for hiding this comment

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

🤦 Nope, a much simpler explanation :)

mypy did not complain previously because there were no type hints on the method.

d19f8f2 introduced type hints, and then the lint error appeared.

# return None -> should NEVER reach here since we regex check input

if re.match(_VALUE_REGEX, vm): # vm is a number (e.g. 0)
Expand Down
0