BUG: AttributeError raised by map inside groupby reduction
State: closed

Gets the last AttributeError following #29100.

Labels:
name: Bug
name: Groupby

Comments:
> If you make the groupby change to stop catching AttributeError but don't change the index code, there is a test (dont know which off the top of my head) that raises an AttributeError in DatetimeIndex.map because index._data is an ndarray instead of a DTA.

Thanks! It is `pandas/tests/groupby/test_groupby.py::test_groupby_reindex_inside_function`. 

That applies a function in groupby that uses `index.map(..)`, and it is the `DatetimeIndex.map` function that calls `astype`. So that is the reason you need to fix `astype` to get the test passing in this PR. 
But not sure that this is a proper fix then, as the fact that it is astype you need to fix is thus only accidental because the test was using that (the test could also use something else as astype, and it might then fail in another place?)

> the test could also use something else as astype, and it might then fail in another place?

Yes, this is a side-effect of the _index_data workaround and we're going to surface and fix this bugs as we suppress fewer exceptions in the groupby code.
>  we're going to surface and fix this bugs

Hmm, you could also formulate this as "we're going to give bugs to the users"? 
In my mind, it is currently not a "bug" if the function that you pass as a user to `agg` raises for some reason an AttributeError in the fast cython path, but works in the fallback slow path. Right now, we ensure that we at least try to slow path if the fast path does not work. If that is needed for certain functions, I don't think that is a bug.

---

I looked for a moment into the idea of "properly" setting the data (still a hack of course), seems to pass the groupby tests:

```patch
diff --git a/pandas/_libs/reduction.pyx b/pandas/_libs/reduction.pyx
index 7ed131e1c..10c113edc 100644
--- a/pandas/_libs/reduction.pyx
+++ b/pandas/_libs/reduction.pyx
@@ -409,7 +409,7 @@ cdef class SeriesGrouper:
                         cached_typ = self.typ(vslider.buf, index=cached_ityp,
                                               name=name)
                     else:
-                        object.__setattr__(cached_ityp, '_data', islider.buf)
+                        object.__setattr__(cached_ityp, '_index_data', islider.buf)
                         cached_ityp._engine.clear_mapping()
                         object.__setattr__(
                             cached_typ._data._block, 'values', vslider.buf)
diff --git a/pandas/core/groupby/generic.py b/pandas/core/groupby/generic.py
index c766fcaa4..023ace965 100644
--- a/pandas/core/groupby/generic.py
+++ b/pandas/core/groupby/generic.py
@@ -262,7 +262,7 @@ class SeriesGroupBy(GroupBy):
 
             try:
                 return self._python_agg_general(func, *args, **kwargs)
-            except (AssertionError, TypeError):
+            except (AssertionError, TypeError, AttributeError):
                 raise
             except Exception:
                 result = self._aggregate_named(func, *args, **kwargs)
diff --git a/pandas/core/indexes/base.py b/pandas/core/indexes/base.py
index 4c15e4b26..81a0b0cef 100644
--- a/pandas/core/indexes/base.py
+++ b/pandas/core/indexes/base.py
@@ -533,6 +533,16 @@ class Index(IndexOpsMixin, PandasObject):
             setattr(result, k, v)
         return result._reset_identity()
 
+    @property
+    def _index_data(self):
+        return self._data
+    
+    @_index_data.setter
+    def _index_data(self, data):
+        self._data = data
+
     @cache_readonly
     def _constructor(self):
         return type(self)
diff --git a/pandas/core/indexes/datetimelike.py b/pandas/core/indexes/datetimelike.py
index bf89bbbdf..a03702da0 100644
--- a/pandas/core/indexes/datetimelike.py
+++ b/pandas/core/indexes/datetimelike.py
@@ -98,6 +98,17 @@ class DatetimeIndexOpsMixin(ExtensionOpsMixin):
     __iter__ = ea_passthrough(DatetimeLikeArrayMixin.__iter__)
     mean = ea_passthrough(DatetimeLikeArrayMixin.mean)
 
+    @property
+    def _index_data(self):
+        return self._data._data
+    
+    @_index_data.setter
+    def _index_data(self, data):
+        self._data._data = data
+
     @property
     def freq(self):
         """
diff --git a/pandas/core/indexes/period.py b/pandas/core/indexes/period.py
index a20290e77..8988307a4 100644
--- a/pandas/core/indexes/period.py
+++ b/pandas/core/indexes/period.py
@@ -297,7 +297,7 @@ class PeriodIndex(DatetimeIndexOpsMixin, Int64Index, PeriodDelegateMixin):
         result = object.__new__(cls)
         result._data = values
         # For groupby perf. See note in indexes/base about _index_data
-        result._index_data = values._data
         result.name = name
         result._reset_identity()
         return result
diff --git a/pandas/core/indexes/timedeltas.py b/pandas/core/indexes/timedeltas.py
index 2324b8cf7..b10a9c63f 100644
--- a/pandas/core/indexes/timedeltas.py
+++ b/pandas/core/indexes/timedeltas.py
@@ -272,7 +272,7 @@ class TimedeltaIndex(
         result._data = tdarr
         result.name = name
         # For groupby perf. See note in indexes/base about _index_data
-        result._index_data = tdarr._data
 
         result._reset_identity()
         return result
```



> Hmm, you could also formulate this as "we're going to give bugs to the users"?

I could, but don't.  I formulate it as "there are bugs present, and this makes it easier to find and fix them"

> I looked for a moment into the idea of "properly" setting the data (still a hack of course), seems to pass the groupby tests:

Comments around this hack suggest it is there for performance reasons.  If the new hack you're suggesting doesn't involve a huge performance hit, then great.  But getting rid of all hack-ness in/around _index_data is a bigger task that is outside the scope of this PR.
> I could, but don't. I formulate it as "there are bugs present, and this makes it easier to find and fix them"

It's not necessarily a *user* bug. 
We know we have a hack with the `_index_data`, we know that a valid groupby operation can sometimes fail because of that hack (but in the fallback it is handled properly). Surfacing *our* failure to the user is not a good fix IMO. 

---

Trying to see where is our disagreement: 
Do you agree that this PR can cause a regression for a user? (of course every PR can give regression in *unforeseen* ways, but here we know it)

Assume we didn't have the `test_groupby_reindex_inside_function`. In that case, you wouldn't have thought about "fixing" the `DatetimeIndex.astype` method. So if we would start raising the AttributeError as you do in this PR (as it would seem there is no reason to catch it), that example would fail. 
We have now a test that used `astype` (so you fixed astype), but there are many other operations that a user could do inside the applied function with the index that does not use astype but something else that relies on the fact that `_data` is an EA.

You say: "by raising the error, it makes it easier to find bugs". 
I agree that this change might turn up more cases where an AttributeError is incorrectly raised that should be fixed. But we know such a case that might happen: because of our `_index_data` hack, we will see AttributeErrors. But the bug here is out `_index_data` hack, which we already know.

So maybe to summarize my reasoning: as long as we have this `_index_data` hack, I think we should continue catching the AttributeError to *prevent* users from seeing this (as it is something they cannot do anything about, it's something we need to fix in pandas).