Support parallel type checking on Windows #20777
Conversation
Co-authored-by: Claude <claude@anthropic.com>
for more information, see https://pre-commit.ci
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Great, thanks! Couple optional comments. I will wait a bit in case you have any opinion on these, if not, I will just go ahead and merge it.
| assert pid == self.proc.pid, f"PID mismatch: {pid} vs {self.proc.pid}" | ||
| if sys.platform != "win32": | ||
| # TODO(emmatyping): for some reason this does not work on Windows. Probably | ||
| # because we don't fork? We should check this |
There was a problem hiding this comment.
It looks like we are not the first affected by this. Apparently virtual environments and other launchers on Windows create "wrapper" processes, so we will get a different PIDs, even if using sys.executable. Found this one https://stackoverflow.com/questions/64884701/subprocess-has-wrong-pid-on-windows (and a bunch of similar questions).
There was a problem hiding this comment.
Good find! Somehow my searching didn't find that. I'll update the comment to reflect it.
| ov, err = _winapi.ReadFile(conn.connection, 1, overlapped=True) | ||
| except OSError: | ||
| # Broken/closed pipe | ||
| continue |
There was a problem hiding this comment.
Not sure this except is needed. On Linux a closed socket will be returned as ready, so that the caller will fail with No data received when getting empty data from it. We want a similar behavior on Windows so that if one of the workers crashed, the coordinator should crash as well (showing the INTERNAL ERROR: ... thing).
So I guess we can either raise IPCError here, or append this one to ready.
2 participants
Follow-up to #933
There is a bit of weirdness with PIDs going on here that I didn't have time to fully resolve, but this does work for me locally.
Should I add test actions for Windows? It'd be good to test this regularly.