Repository: dask/dask
Pull request: Added compute method to raise error on use
- [ ] Closes #8695
- [ ] Tests added / passed
- [ ] Passes `pre-commit run --all-files`

First (very) naive implementation of compute method

Problem statement:
Nicer handling of incorrect usage of `DataFrameGroupBy`
<!-- Please do a quick search of existing issues to make sure that this has not been asked before. -->
I was trying to debug a `DataFrameGroupBy` operation and naively tried to `.compute()` it. This raised a `KeyError` that the `compute` column was missing.

```python
import dask

df = dask.datasets.timeseries()
df.groupby("name").compute()
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
...
-> 2003     raise AttributeError(e) from e

AttributeError: 'Column not found: compute'
```

In hindsight, this is clearly not the right thing to do. On the other hand, as a naive Pandas-familiar user, I was trying to "compute" the Pandas `GroupBy` object so that I could inspect it. I think it would be nice to have a more useful error in this case. However, since `dask.dataframe.DataFrameGroupBy` re-raises the `KeyError` as an `AttributeError`, I'm not sure what the appropriate resolution should be 😄 

<details><summary>Full traceback 👇 </summary>

```python
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File ~/GitHub/dask/dask/dataframe/groupby.py:2001, in DataFrameGroupBy.__getattr__(self, key)
   2000 try:
-> 2001     return self[key]
   2002 except KeyError as e:

File ~/GitHub/dask/dask/dataframe/groupby.py:1987, in DataFrameGroupBy.__getitem__(self, key)
   1986 # error is raised from pandas
-> 1987 g._meta = g._meta[key]
   1988 return g

File ~/mambaforge/envs/test-environment/lib/python3.9/site-packages/pandas/core/groupby/generic.py:1538, in DataFrameGroupBy.__getitem__(self, key)
   1532     warnings.warn(
   1533         "Indexing with multiple keys (implicitly converted to a tuple "
   1534         "of keys) will be deprecated, use a list instead.",
   1535         FutureWarning,
   1536         stacklevel=2,
   1537     )
-> 1538 return super().__getitem__(key)

File ~/mambaforge/envs/test-environment/lib/python3.9/site-packages/pandas/core/base.py:232, in SelectionMixin.__getitem__(self, key)
    231 if key not in self.obj:
--> 232     raise KeyError(f"Column not found: {key}")
    233 subset = self.obj[key]

KeyError: 'Column not found: compute'

The above exception was the direct cause of the following exception:

AttributeError                            Traceback (most recent call last)
Input In [3], in <module>
----> 1 df.groupby("name").compute()

File ~/GitHub/dask/dask/dataframe/groupby.py:2003, in DataFrameGroupBy.__getattr__(self, key)
   2001     return self[key]
   2002 except KeyError as e:
-> 2003     raise AttributeError(e) from e

AttributeError: 'Column not found: compute'
```
</details>

Hints from issue discussion:
Would defining a `compute` method and raising a `NotImplementedError` (as done [here](https://github.com/dask/dask/blob/80a82008d5b02a08f6ff59d802defcc43247eb1a/dask/dataframe/reshape.py#L133-L148)) be the right approach? @jsignell do you have thoughts on this?
> Would defining a `compute` method and raising a `NotImplementedError` (as done [here](https://github.com/dask/dask/blob/80a82008d5b02a08f6ff59d802defcc43247eb1a/dask/dataframe/reshape.py#L133-L148)) be the right approach? @jsignell do you have thoughts on this?

Yes! That seems like a perfect solution! Maybe with a bit of a suggestion as well that points people to chain an aggregation method or use a getter like `get_group`.
Hi, actually looking on this issue. To clarify: we need to implement a compute method to https://github.com/dask/dask/blob/80a82008d5b02a08f6ff59d802defcc43247eb1a/dask/dataframe/groupby.py#L1014 in order to raise a NoImplementederror ?
> Hi, actually looking on this issue. To clarify: we need to implement a compute method to
> 
> https://github.com/dask/dask/blob/80a82008d5b02a08f6ff59d802defcc43247eb1a/dask/dataframe/groupby.py#L1014
> in order to raise a NoImplementederror ?

Yes that is exactly right!

Commit message: Merge branch 'handling_dataframegroupby' of github.com:Dranaxel/dask into handling_dataframegroupby

Patch to review:
diff --git a/dask/dataframe/groupby.py b/dask/dataframe/groupby.py
index f8d2defa945..44a840c7e60 100644
--- a/dask/dataframe/groupby.py
+++ b/dask/dataframe/groupby.py
@@ -1290,6 +1290,12 @@ def _cum_agg(self, token, chunk, aggregate, initial):
         graph = HighLevelGraph.from_collections(name, dask, dependencies=dependencies)
         return new_dd_object(graph, name, chunk(self._meta), self.obj.divisions)
 
+    def _compute(self):
+        raise NotImplementedError(
+            "DataFrameGroupBy does not allow compute method."
+            "Please chain it with an aggregation method or use a getter like 'get_group'"
+        )
+
     def _shuffle(self, meta):
         df = self.obj
 


Code review comments:
diff_hunk: @@ -1290,6 +1290,12 @@ def _cum_agg(self, token, chunk, aggregate, initial):
         graph = HighLevelGraph.from_collections(name, dask, dependencies=dependencies)
         return new_dd_object(graph, name, chunk(self._meta), self.obj.divisions)
 
+    def _compute(self):
path: dask/dataframe/groupby.py
text: @user1:
I think this should just be `compute` and it should accept kwarg to match:

https://github.com/dask/dask/blob/bd8e8dc3f1329e7f9ef24072479729d9effeb6ad/dask/base.py#L266-L267

```suggestion
    def compute(self, **kwargs):
```

@author:
Why is it mandatory to accept kwargs please ?

@user1:
The idea is to match the compute method on the base class so that if someone accidentally calls `gb.compute(scheduler="threads")` for instance, they will get the `NotImplementedError`

diff_hunk: @@ -1290,6 +1290,12 @@ def _cum_agg(self, token, chunk, aggregate, initial):
         graph = HighLevelGraph.from_collections(name, dask, dependencies=dependencies)
         return new_dd_object(graph, name, chunk(self._meta), self.obj.divisions)
 
+    def _compute(self):
+        raise NotImplementedError(
+            "DataFrameGroupBy does not allow compute method."
+            "Please chain it with an aggregation method or use a getter like 'get_group'"
path: dask/dataframe/groupby.py
text: @user1:
Hmm I think this could be worded a little more clearly. What about something like:

```suggestion
            "Please chain it with an aggregation method (like ``.mean()``) or get a " 
            "specific group using ``.get_group()`` before calling ``compute()``
```