10000 [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.

Sign up for GitHub

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

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d19f8f2
Multiprocessing API: standard streams: add type hints for internal 't…
jayaddison Jan 28, 2023
e5fe23b
Multiprocessing API: standard streams: update 'to_std' utility parser…
jayaddison Jan 28, 2023
09dfd2b
mypy: filter warning on missing-return-statement; as per the correspo…
jayaddison Feb 1, 2023
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
Prev Previous commit
Next Next commit
Multiprocessing API: standard streams: update 'to_std' utility parser… 
… to use enum membership testing

Ref: #92319 (comment)
  • Loading branch information
@jayaddison
jayaddison committed Jan 28, 2023
commit e5fe23b8aff3b74db62ced5a7e127097c13af0f1
7 changes: 3 additions & 4 deletions torch/distributed/elastic/multiprocessing/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,9 @@ def from_str(cls, vm: str) -> Union["Std", Dict[int, "Std"]]:
"""

def to_std(v: str) -> Std:
v = int(v)
for s in Std:
if s == v:
return s
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