Skip to content
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

gh-107431: Make multiprocessing.managers.{DictProxy,ListProxy} generic #107433

Merged
merged 6 commits into from
Nov 10, 2023

Conversation

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.

Some small points:

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.

Thanks, LGTM! Will wait to see if Jelle has any objections before merging.

Previous additions of __class_getitem__ methods have been treated as features, so we probably can't backport this, unfortunately.

@AlexWaygood
Copy link
Member

Hmm... I suppose one reason for not making these subscriptable at runtime might be that these are undocumented classes that aren't included in __all__. But I personally still lean towards making them subscriptable, nonetheless.

@JelleZijlstra
Copy link
Member

Maybe these classes should be documented? I'm also a bit hesitant about this if these classes are undocumented.

@AlexWaygood
Copy link
Member

AlexWaygood commented Aug 28, 2023

Maybe these classes should be documented? I'm also a bit hesitant about this if these classes are undocumented.

@gpshead, any thoughts from you on this, as a multiprocessing expert? I think there's a decent case for documenting these classes, since instances of ListProxy and DictProxy are returned from multiprocessing.managers.SyncManager.list() and multiprocessing.managers.SyncManager.dict() respectively. Both of those are public, documented methods:

@JelleZijlstra JelleZijlstra removed their request for review September 7, 2023 04:06
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, but i'm not a typing expert.

In case of doubt, I prefer to not document DictProxy and ListProxy. I don't think that not documenting these types should block this enhancement.

@vstinner
Copy link
Member

@JelleZijlstra: What do you think?

@gvanrossum, @carljm (typing experts): Do you want to review this typing change?

@AlexWaygood
Copy link
Member

@vstinner, this needs input from multiprocessing experts, not typing experts. Two typing experts have already given their input here (Jelle and myself). From a typing perspective, the change is fine. But from a multiprocessing perspective: the classes are currently undocumented and not included in __all__. Adding __class_getitem__ might encourage their use in type annotations. Is that okay? Should we document these classes, or are they best left undocumented? What's our position on that?

@vstinner
Copy link
Member

@pitrou: Do you have an opinion on this change?

@gpshead
Copy link
Member

gpshead commented Nov 10, 2023

I don't think anyone wants to claim expertise for multiprocessing no matter how much we've done work on it. This change looks good to me, I made a couple edits.

@pitrou
Copy link
Member

pitrou commented Nov 10, 2023

I have no idea what a GenericAlias is, and the doc is rather cryptic. As long as this doesn't change runtime behavior I see no reason to oppose it.

@gpshead gpshead enabled auto-merge (squash) November 10, 2023 23:18
@gpshead gpshead merged commit ae8116c into python:main Nov 10, 2023
@vstinner
Copy link
Member

Thanks for making _BaseDictProxy private.

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…` generic (python#107433)

Make `multiprocessing.managers.{DictProxy,ListProxy}` generic for type annotation use.  `ListProxy[str]` for example.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…` generic (python#107433)

Make `multiprocessing.managers.{DictProxy,ListProxy}` generic for type annotation use.  `ListProxy[str]` for example.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
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.

multiprocessing manager classes DictProxy and ListProxy don't support typing in 3.11.4
7 participants