Skip to content

multiprocessing.managers: fix TypeVar usage #7938

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 7 commits into from
May 26, 2022
Merged

Conversation

AlexWaygood
Copy link
Member

#7928

dict() and list() just return empty dictionaries and lists (respectively) if no arguments are supplied:

>>> from multiprocessing.managers import SyncManager
>>> with SyncManager() as s:
...     print(s.dict())
...
{}

#7928

`dict()` and `list()` just return empty dictionaries and lists (respectively) if no arguments are supplied:

```python
>>> from multiprocessing.managers import SyncManager
>>> with SyncManager() as s:
...     print(s.dict())
...
{}
```
@overload
def dict(self, sequence: Mapping[_KT, _VT]) -> _dict[_KT, _VT]: ...
@overload
def dict(self) -> _dict[Any, Any]: ...
Copy link
Member

Choose a reason for hiding this comment

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

This is another instance of the issue @erictraut found in microsoft/pyright#3501 (comment), though here it's on a function, not a constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, empty collections are tough :(

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Don't have a strong opinion on what to do here, but observations:

  • As Eric pointed out, with changes like this, we potentially make it so users silently get Any types, instead of getting warned by pyright's strict mode about "unknown" types.
  • The parameter types here are wrong. multiprocessing is a maze but as far as I can tell these should accept the same arguments as the list and dict constructors. For example, dict() allows kwargs and list() allows any iterable.
  • The return types are wrong: these methods return special proxy objects, not actual dicts or lists.

Some examples from playing with them:

>>> m.list({1, 2, 3})
<ListProxy object, typeid 'list' at 0xffff933192b0>
>>> l = _
>>> type(l)
<class 'multiprocessing.managers.ListProxy'>
>>> len(l)
3
>>> str(l)
'[1, 2, 3]'
>>> l[0]
1
>>> m.dict(x=3)
<DictProxy object, typeid 'dict' at 0xffff933cd040>
>>> d=_
>>> d["x"]
3
>>> d["x"] = 5
>>> d["x"]
5

@AlexWaygood AlexWaygood marked this pull request as draft May 25, 2022 05:37
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented May 25, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna)
+ tests/samplers_tests/tpe_tests/test_sampler.py:1044: error: Incompatible types in assignment (expression has type "DictProxy[Any, Any]", variable has type "Dict[int, List[int]]")
+ tests/samplers_tests/tpe_tests/test_sampler.py:1045: error: Incompatible types in assignment (expression has type "DictProxy[Any, Any]", variable has type "Dict[int, int]")

The code in question is doing this:

sequence_dict: Dict[int, List[int]] = manager.dict()
hash_dict: Dict[int, int] = manager.dict()

https://github.com/optuna/optuna/blob/ce2b95111cd4ce094fd81c044128fa8851c786b3/tests/samplers_tests/tpe_tests/test_sampler.py#L1044-L1045

This was the best way of annotating that code prior to this PR, given that in typeshed we didn't acknowledge the existence of these classes previously. But it will have to be updated if this PR is merged.

@AlexWaygood

This comment was marked as resolved.

@AlexWaygood AlexWaygood marked this pull request as ready for review May 25, 2022 13:11
@AlexWaygood AlexWaygood reopened this May 25, 2022
@AlexWaygood AlexWaygood requested a review from JelleZijlstra May 25, 2022 13:24
@github-actions

This comment has been minimized.

if sys.version_info < (3, 7):
def has_key(self, k: _KT) -> bool: ...

class BaseListProxy(BaseProxy, MutableSequence[_T]):
Copy link
Member

Choose a reason for hiding this comment

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

Missing __contains__ and reverse

Copy link
Member Author

Choose a reason for hiding this comment

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

Both inherited from MutableSequence

@@ -66,6 +65,62 @@ class ValueProxy(BaseProxy, Generic[_T]):
if sys.version_info >= (3, 9):
def __class_getitem__(cls, item: Any) -> GenericAlias: ...

class DictProxy(BaseProxy, MutableMapping[_KT, _VT]):
Copy link
Member

Choose a reason for hiding this comment

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

Also __contains__, clear, popitem, setdefault, update.

(Or are those inherited? In that case, why do we have __len__?)

Copy link
Member Author

Choose a reason for hiding this comment

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

All inherited from MutableMapping. MutableMapping.__len__ is an abstract method, so must be overridden here in order to make this class concrete.

def pop(self, __key: _KT) -> _VT: ...
@overload
def pop(self, __key: _KT, __default: _VT | _T) -> _VT | _T: ...
def keys(self) -> list[_KT]: ... # type: ignore[override]
Copy link
Member

Choose a reason for hiding this comment

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

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@AlexWaygood AlexWaygood requested a review from JelleZijlstra May 26, 2022 06:32
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

optuna (https://github.com/optuna/optuna)
+ tests/samplers_tests/tpe_tests/test_sampler.py:1044: error: Incompatible types in assignment (expression has type "DictProxy[Any, Any]", variable has type "Dict[int, List[int]]")
+ tests/samplers_tests/tpe_tests/test_sampler.py:1045: error: Incompatible types in assignment (expression has type "DictProxy[Any, Any]", variable has type "Dict[int, int]")

@JelleZijlstra JelleZijlstra merged commit d511312 into master May 26, 2022
@JelleZijlstra JelleZijlstra deleted the AlexWaygood-patch-1 branch May 26, 2022 14:18
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