Repository: dask/dask
Pull request: Catch and raise type errors on meta reductions
Attempt to fix #2452 

Not really sure about the approach, it feels like there just isn't an elegant way to solve this properly.

Problem statement:
Failing to specify numeric columns fails with "could not convert foofoo to numeric"
A minor suggestion to improve error messages: The following code fails, because the meta data does not specify that column `A` is numeric. However, the error message is somewhat non-intuitive:

```python
import pandas as pd
import dask.dataframe as dd
from dask import delayed

df = pd.DataFrame({"A": [1, 2, 3]})
ddf = dd.from_delayed(delayed(lambda: df), meta=pd.DataFrame(columns=["A"]))
ddf["A"].mean()
# TypeError: Could not convert foofoo to numeric
```

This is probably a consequence of the data in `_simple_fake_mapping` in `dataframe/utils.py`. It would be good to turn the error into something more explicit mentioning the column name + dtype, so that users don't start to search for foofoo.

Hints from issue discussion:
I agree that this would be a good change.  Can I interest you in submitting a small PR?  ("no" is a fine answer)
In general yes, but I don't know when I will get to it.

On first glance: Solving it for `df.mean()` only is trivial, but identifying + solving the issue for all affected operations requires a bit more thought I guess.
I suspect that you could catch an error somewhere in `map_partitions` or `_get_numeric_data` and have it applied more widely.
For reference, this is what you in pandas get when you call `mean` on a non-numeric series.

```python
In [52]: x = pd.Series(['a', 'b', 'c'])

In [53]: x.mean()
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/Users/jcrist/anaconda/envs/dask/lib/python3.5/site-packages/pandas/core/nanops.py in _ensure_numeric(x)
    767         try:
--> 768             x = float(x)
    769         except Exception:

ValueError: could not convert string to float: 'abc'
```

For this case, we could provide a better error than pandas (check relevant series reductions for object dtype and fail early), but we can't catch the error for all operations. In general if you provide the wrong metadata, you're going to have a bad time.

> I suspect that you could catch an error somewhere in map_partitions

We already do this when we infer metadata for user provided functions, and prefix the error message with a brief note (see https://github.com/dask/dask/blob/master/dask/dataframe/utils.py#L126-L149).

I think the error behavior should be:

- For methods that take user functions (e.g. `map_partitions` or `reduction`), metadata inference should fail nicely (already done, but maybe message prefix could be nicer).
- For builtin methods, we should provide same/similar errors to pandas assuming the metadata is correct (already done by calling method on fake data generated from the metadata).

If the metadata is incorrect (as happened here) I'm not sure what we should do.
My idea would be to wrap all lines like [this](https://github.com/dask/dask/blob/ecdae84aa3b8ee5408876b0b7d7eade3d127e177/dask/dataframe/core.py#L1174) into: 

```python
with safe_meta_numerics():
    meta = self._meta_nonempty.mean(axis=axis, skipna=skipna)
```

with an appropriate context manager that catches only `TypeError` containing "Could not convert" and re-raises with an explicit hint that the meta data must be wrong. 

It took me quite some time to make sense of this error, probably because:
* I did explicitly specify `meta` including dtypes, just with a tiny mistake. 
* From a user perspective the `.mean()` is a lazy operation, so it was surprising to see hints of a numeric operation in the first place.
* The "foofoo" pointed me in the wrong direction, checking my code & data for "foofoo".
>  explicit hint that the meta data must be wrong.

If the column is actually an object column, then the metadata doesn't have to be wrong, the operation is just invalid. I'd rather fix series reductions to check for object dtype, and error explicitly with `ValueError("mean reduction not supported for object dtype")`. If the metadata was correct, this is a better error message than what pandas throws. If the metadata is wrong, the user will see "object dtype" and should be able to track down their prior mistake.

Commit message: switched to upfront raise

Patch to review:
diff --git a/dask/dataframe/core.py b/dask/dataframe/core.py
index 5f0be93456c..9895b94a3c1 100644
--- a/dask/dataframe/core.py
+++ b/dask/dataframe/core.py
@@ -5,6 +5,7 @@
 import operator
 from operator import getitem
 from pprint import pformat
+from contextlib import contextmanager
 import warnings
 
 from toolz import merge, first, unique, partition_all, remove
@@ -1079,6 +1080,7 @@ def _reduction_agg(self, name, axis=None, skipna=True,
 
     @derived_from(pd.DataFrame)
     def abs(self):
+        _raise_if_object_series(self, "abs")
         meta = self._meta_nonempty.abs()
         return self.map_partitions(M.abs, meta=meta)
 
@@ -1171,6 +1173,7 @@ def count(self, axis=None, split_every=False):
     @derived_from(pd.DataFrame)
     def mean(self, axis=None, skipna=True, split_every=False):
         axis = self._validate_axis(axis)
+        _raise_if_object_series(self, "mean")
         meta = self._meta_nonempty.mean(axis=axis, skipna=skipna)
         if axis == 1:
             return map_partitions(M.mean, self, meta=meta,
@@ -1190,6 +1193,7 @@ def mean(self, axis=None, skipna=True, split_every=False):
     @derived_from(pd.DataFrame)
     def var(self, axis=None, skipna=True, ddof=1, split_every=False):
         axis = self._validate_axis(axis)
+        _raise_if_object_series(self, "var")
         meta = self._meta_nonempty.var(axis=axis, skipna=skipna)
         if axis == 1:
             return map_partitions(M.var, self, meta=meta,
@@ -1210,6 +1214,7 @@ def var(self, axis=None, skipna=True, ddof=1, split_every=False):
     @derived_from(pd.DataFrame)
     def std(self, axis=None, skipna=True, ddof=1, split_every=False):
         axis = self._validate_axis(axis)
+        _raise_if_object_series(self, "std")
         meta = self._meta_nonempty.std(axis=axis, skipna=skipna)
         if axis == 1:
             return map_partitions(M.std, self, meta=meta,
@@ -1223,6 +1228,7 @@ def std(self, axis=None, skipna=True, ddof=1, split_every=False):
     @derived_from(pd.DataFrame)
     def sem(self, axis=None, skipna=None, ddof=1, split_every=False):
         axis = self._validate_axis(axis)
+        _raise_if_object_series(self, "sem")
         meta = self._meta_nonempty.sem(axis=axis, skipna=skipna, ddof=ddof)
         if axis == 1:
             return map_partitions(M.sem, self, meta=meta,
@@ -1259,6 +1265,7 @@ def quantile(self, q=0.5, axis=0):
             return map_partitions(M.quantile, self, q, axis,
                                   token=keyname, meta=(q, 'f8'))
         else:
+            _raise_if_object_series(self, "quantile")
             meta = self._meta.quantile(q, axis=axis)
             num = self._get_numeric_data()
             quantiles = tuple(quantile(self[c], q) for c in num.columns)
@@ -1542,6 +1549,15 @@ def values(self):
         return Array(merge(self.dask, dsk), name, chunks, x.dtype)
 
 
+def _raise_if_object_series(x, funcname):
+    """
+    Utility function to raise an error if an object column does not support
+    a certain operation like `mean`.
+    """
+    if isinstance(x, Series) and hasattr(x, "dtype") and x.dtype == object:
+        raise ValueError("`%s` not supported with object series" % funcname)
+
+
 normalize_token.register((Scalar, _Frame), lambda a: a._name)
 
 
diff --git a/dask/dataframe/tests/test_dataframe.py b/dask/dataframe/tests/test_dataframe.py
index 37a0b8b9bfb..a20ab26757c 100644
--- a/dask/dataframe/tests/test_dataframe.py
+++ b/dask/dataframe/tests/test_dataframe.py
@@ -1769,7 +1769,7 @@ def test_abs():
     ddf = dd.from_pandas(df, npartitions=2)
     assert_eq(ddf.A.abs(), df.A.abs())
     assert_eq(ddf[['A', 'B']].abs(), df[['A', 'B']].abs())
-    pytest.raises(TypeError, lambda: ddf.C.abs())
+    pytest.raises(ValueError, lambda: ddf.C.abs())
     pytest.raises(TypeError, lambda: ddf.abs())
 
 
@@ -2632,3 +2632,14 @@ def test_boundary_slice_same(index, left, right):
     df = pd.DataFrame({"A": range(len(index))}, index=index)
     result = boundary_slice(df, left, right)
     tm.assert_frame_equal(result, df)
+
+
+def test_meaningful_error_on_wrong_meta():
+    df = pd.DataFrame({"A": [1, 2, 3]})
+    ddf = dd.from_pandas(df, npartitions=2)
+    # simulate erroneous meta data
+    ddf._meta = pd.DataFrame(columns=["A"])
+    with pytest.raises(ValueError) as e_info:
+        ddf["A"].mean()
+    assert "foofoo" not in e_info.value.message
+    assert "not supported" in e_info.value.message


Code review comments:
diff_hunk: @@ -2632,3 +2632,14 @@ def test_boundary_slice_same(index, left, right):
     df = pd.DataFrame({"A": range(len(index))}, index=index)
     result = boundary_slice(df, left, right)
     tm.assert_frame_equal(result, df)
+
+
+def test_meaningful_error_on_wrong_meta():
path: dask/dataframe/tests/test_dataframe.py
text: @user1:
This is a test for a specific issue, but the real behavior we'd like to test is that errors for things like `mean` on object series are more informative. Perhaps this would be better:

```python
def test_better_errors_object_reductions():
    # GH2452
    s = pd.Series(['a', 'b', 'c', 'd'])
    ds = dd.from_pandas(s, npartitions=2)
    with pytest.raises(ValueError) as err:
        ds.mean()
    assert "`mean` not supported with object series" in str(err.value)
```