TYP: core.computations.scope

[jbrockmendel]

mypy does not play nicely with DeepChainMap. @WillAyd @simonjayhawkins any ideas about the 4 places where i had to put # type: ignore?

Made _update private just to make it easier to reason about this thing

[simonjayhawkins]

I think this could be a typeshed issue not a mypy issue as new_child should return type of self not typing.ChainMap[_KT, _VT] see https://github.com/python/typeshed/blob/34d68ab0a2117a08fa221d3a10884f35cacf2cdc/stdlib/3/collections/__init__.pyi#L323-L338

[simonjayhawkins]

the # type: ignore could be avoided with

    def new_child(self, *args, **kwargs) -> "DeepChainMap":
        result = super().new_child(*args, **kwargs)
        assert isinstance(result, DeepChainMap)
        return result

[simonjayhawkins]

mypy does not play nicely with DeepChainMap

The design of ChainMap does not appear to be intended to be a generic container. i.e. the interface does not suggest that if a list of Dicts is given, that maps will contain a list of Dicts.

Although our implementation of DeepChainMap works, I guess that it could break in the future.

Trying to modify DeepChainMap to convince mypy that is a list of Mutuable mappings gives rise to errors such as error: Signature of "maps" incompatible with supertype "ChainMap" and error: Return type "List[MutableMapping[Any, Any]]" of "maps" incompatible with return type "List[Mapping[Any, Any]]" in supertype "ChainMap"

So maybe ChainMap shouldn't be subclassed for this intent.

here's a DeepChainMap using composition instead, passing mypy without ignores and passing tests. (probably not what we want though)

diff --git a/pandas/compat/chainmap.py b/pandas/compat/chainmap.py
index 493ba68bf..77ec507b9 100644
--- a/pandas/compat/chainmap.py
+++ b/pandas/compat/chainmap.py
@@ -1,22 +1,46 @@
 from collections import ChainMap
+from typing import List, MutableMapping
 
 
-class DeepChainMap(ChainMap):
+class DeepChainMap:
+    def __init__(self, *maps):
+        self.chainmap = ChainMap(*maps)
+
     def __setitem__(self, key, value):
-        for mapping in self.maps:
+        for mapping in self.chainmap.maps:
             if key in mapping:
                 mapping[key] = value
                 return
-        self.maps[0][key] = value
+        self.chainmap.maps[0][key] = value
 
     def __delitem__(self, key):
-        for mapping in self.maps:
+        for mapping in self.chainmap.maps:
             if key in mapping:
                 del mapping[key]
                 return
         raise KeyError(key)
 
+    @property
+    def maps(self) -> List[MutableMapping]:
+        return self.chainmap.maps
+
     def new_child(self, *args, **kwargs) -> "DeepChainMap":
-        # ChainMap.new_child returns self.__class__(...) but mypy
-        #  doesn't know that, so we annotate it explicitly here.
-        return super().new_child(*args, **kwargs)  # type: ignore
+        return DeepChainMap(self.chainmap.new_child(*args, **kwargs))
+
+    def __iter__(self):
+        return iter(self.chainmap)
+
+    def update(self, *args, **kwargs):
+        return self.chainmap.update(*args, **kwargs)
+
+    def keys(self, *args, **kwargs):
+        return self.chainmap.keys(*args, **kwargs)
+
+    def __getitem__(self, key):
+        return self.chainmap[key]
+
+    def __len__(self):
+        return len(self.chainmap)
+
+    def copy(self):
+        return DeepChainMap(*self.chainmap.copy().maps)
diff --git a/pandas/core/computation/scope.py b/pandas/core/computation/scope.py
index 9b9dde83f..f9bdf96ab 100644
--- a/pandas/core/computation/scope.py
+++ b/pandas/core/computation/scope.py
@@ -222,14 +222,10 @@ class Scope:
         new_value : object
             Value to be replaced along with the possible renaming
         """
-        maps: List[MutableMapping]
-        mapping: MutableMapping
-
-        # TODO: convince mypy that these maps are in fact mutable
         if self.has_resolvers:
-            maps = self.resolvers.maps + self.scope.maps  # type: ignore
+            maps = self.resolvers.maps + self.scope.maps
         else:
-            maps = self.scope.maps  # type: ignore
+            maps = self.scope.maps
 
         maps.append(self.temps)
 
@@ -323,6 +319,5 @@ class Scope:
         vars : DeepChainMap
             All variables in this scope.
         """
-        # TODO: convince mypy that all of the maps are mutable
-        maps = [self.temps] + self.resolvers.maps + self.scope.maps  # type: ignore
+        maps = [self.temps] + self.resolvers.maps + self.scope.maps
         return DeepChainMap(*maps)

[jbrockmendel]

@simonjayhawkins thanks for taking a look at this. You've outlined several options for how to deal with these issues; do you have a preferred one? Leaving this un-typed is also an option.

[simonjayhawkins]

I think mypy is correctly identifying an potential issue with subclassing ChainMap that we should be aware of. But not sure whether making significant changes is warranted at this time.

(also we don't check for MutableMappings when instantiating a DeepChainMap or in __setitem__)

The DeepChainMap code above was while establishing the api of ChainMap required in our usage. Again whether we do need duck type compatibility is another issue.

Maybe we don't even need to use a ChainMap at all here and just store the maps in a List within our DeepChainMap implementation.

You've outlined several options for how to deal with these issues; do you have a preferred one?

my preference would be to move any # type: ignores, casts, asserts etc. out of the main code and into DeepChainMap. You've successfully done this with new_child. I've tried unsuccessfully to do the same with maps, hence ending up with a DeepChainMap not inheriting from ChainMap.

so if you can get it to work by doing something with maps inside the DeepChainMap implementation that'll be great.

otherwise we just need to have good comments for future readers and future self. (which you have done)

@WillAyd any preference?

[simonjayhawkins]

i think we could lose the ignore here using unpacking instead.

diff --git a/pandas/core/computation/scope.py b/pandas/core/computation/scope.py
index 9b9dde83f..935b35e22 100644
--- a/pandas/core/computation/scope.py
+++ b/pandas/core/computation/scope.py
@@ -323,6 +323,4 @@ class Scope:
         vars : DeepChainMap
             All variables in this scope.
         """
-        # TODO: convince mypy that all of the maps are mutable
-        maps = [self.temps] + self.resolvers.maps + self.scope.maps  # type: ignore
-        return DeepChainMap(*maps)
+        return DeepChainMap(self.temps, *self.resolvers.maps, *self.scope.maps)

[jbrockmendel]

Closing following #29667. ill follow up if i find a nice solution for the DeepChainMap issues