[PT-D] Multiprocessing API: standard streams: update 'to_std' utility parser to use enum membership testing #93209
Conversation
…o_std' utility parsing method
… to use enum membership testing Ref: #92319 (comment)
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/93209
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 FailuresAs of commit 09dfd2b: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
Would a Developer Certificate of Origin declaration be acceptable as an alternative to signing a Linux Foundation CLA? |
| def to_std(v: str) -> Std: | ||
| s = Std(int(v)) | ||
| if s in Std: | ||
| return s |
There was a problem hiding this comment.
can we raise a error in the else statement to silent mypy?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also: was mypy not complaining about that before these changes? That seems like a possible bug.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤦 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.
…nding comment on line 124, the method should always return a value
|
(note: still some time required before possible merge, because I haven't signed the CLA. I'd like to confirm whether a DCO is acceptable as an alternative, and have asked that question in #85559. no problem for me if someone else solves the issue until then) |
|
@jayaddison Have you tried to get the CLA directly as an individual contributor? |
|
@fduwjj Not yet, nope - I'd prefer to sign with a Developer Certificate of Origin if possible (I admit there are pluses and minuses to each approach). |
|
@pytorchbot merge |
Merge failedReason: 1 mandatory check(s) failed (Rule Dig deeper by viewing the failures on hud Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "Emergency fix confirmed valid to land and also another failure is not in trunk" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 mandatory check(s) failed (Rule Dig deeper by viewing the failures on hud Details for Dev Infra teamRaised by workflow job |
|
@jayaddison Maybe let's wait for one more week? Does that work for you? I am also asking around internally to see how to unblock you. |
|
Sounds good to me - thanks @fduwjj (I'm also totally ok with anyone else implementing a similar fix) |
Land #93209 faster. [ghstack-poisoned]
Land #93209 faster. [ghstack-poisoned]
" Land #93209 faster. [ghstack-poisoned]
|
This pull request has been merged in e98a942. |
|
For future reference, and transparency: although I wasn't acknowledged by EasyCLA as having signed the PyTorch CLA, I have previously signed a Facebook CLA (in the context of contributions to (probably sounds like I'm talking to myself, and to some extent I am trying to understand the terms of my own contributions, because I think that's good practice for engineers, but also I know from experience that during -- hopefully rare and not always particularly enjoyable -- work involved in license/copyright checks, having clear information available can help reduce workloads; not too different to good reusable software - so maybe this comment can help, if it's ever required) |
|
(and sorry @fduwjj, I didn't intend to hold this up unnecessarily - if I'd realized that my previous CLA signing was applicable at the time, I would've mentioned it. it took a while for me to figure out the details) |
May resolve #92319.