Skip to content

feat: add support for multiprocessing.resource_sharer type hints #8413

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 6 commits into from
Jul 28, 2022
Merged

feat: add support for multiprocessing.resource_sharer type hints #8413

merged 6 commits into from
Jul 28, 2022

Conversation

kkirsche
Copy link
Contributor

@kkirsche kkirsche commented Jul 27, 2022

X-Ref: #6799

This adds best-effort types to multiprocessing.sharer to continue to address issue #6799

The source file can be viewed here:
https://github.com/python/cpython/blob/main/Lib/multiprocessing/resource_sharer.py

@kkirsche
Copy link
Contributor Author

Commit history includes all types including private if it helps for review

@AlexWaygood
Copy link
Member

AlexWaygood commented Jul 27, 2022

Commit history includes all types including private if it helps for review

It doesn't really, to be honest — please delete them :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Please remove all the private things

@kkirsche
Copy link
Contributor Author

kkirsche commented Jul 27, 2022

Sorry about that, the pre-commit bot's changes prevented the push. Cleaning that up now

@AlexWaygood AlexWaygood dismissed their stale review July 27, 2022 21:43

Private things were removed

@github-actions

This comment has been minimized.

Comment on lines 28 to 36
class _ResourceSharer:
def __init__(self) -> None: ...
def register(self, send: Callable[[Connection, int], None], close: Callable[[], None]) -> tuple[_Address, int]: ...
@staticmethod
def get_connection(ident: tuple[_Address, int]) -> Connection: ...
def stop(self, timeout: float | None = ...) -> None: ...

_resource_sharer: _ResourceSharer = ...
stop = _resource_sharer.stop
Copy link
Member

Choose a reason for hiding this comment

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

This all looks like a bit too much to maintain, considering it's a private class in an undocumented module. The only public think here is the stop() callable:

Suggested change
class _ResourceSharer:
def __init__(self) -> None: ...
def register(self, send: Callable[[Connection, int], None], close: Callable[[], None]) -> tuple[_Address, int]: ...
@staticmethod
def get_connection(ident: tuple[_Address, int]) -> Connection: ...
def stop(self, timeout: float | None = ...) -> None: ...
_resource_sharer: _ResourceSharer = ...
stop = _resource_sharer.stop
def stop(timeout: float | None = ...) -> None: ...

Copy link
Contributor Author

@kkirsche kkirsche Jul 28, 2022

Choose a reason for hiding this comment

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

I understand the point, though personally I think it's valuable to provide these types of things for developers who choose to start doing development on internals in more depth, as without the private methods, for me at least, it really hinders learning the cpython stdlib codebase

Copy link
Member

Choose a reason for hiding this comment

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

I agree that typeshed is a fantastic tool for learning about the stdlib codebase, but that's ultimately not its primary purpose.

The issue with including implementation details in the stub is that there are no backwards compatibility guarantees, so they're liable to change frequently, possibly even in micro releases of CPython. That just imposes too much of a maintenance burden on us as maintainers.

There's also a question of the usefulness of these implementation details for typechecking. End users of this module shouldn't be using implementation details of this module in their applications or libraries (because they're liable to change at any time), so it's good, in a way, if a typechecker emits an error when a user tries to import one of these implementation details.

AlexWaygood added a commit that referenced this pull request Jul 28, 2022
I had to look at the CPython source for this module while reviewing #8413, so: here's two small things I noticed while studying it.
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit dd24bba into python:master Jul 28, 2022
@kkirsche kkirsche deleted the multiprocessing/resource_sharer branch July 28, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants