Thanks @jsignell this looks great generally. This is a hugely breaking change that will likely break anyone using DataFrame dispatches outside of Dask (not sure if there's anyone other than dask-cudf).

I'm going to kickoff some conversations internally on our side, but if possible it would be ideal to hold off on merging this for a bit to give us a chance to get our ducks in a row.

Thanks @jsignell this looks great generally. This is a hugely breaking change that will likely break anyone using DataFrame dispatches outside of Dask (not sure if there's anyone other than dask-cudf).

We should discuss if this is something that is worth doing. To my mind, it's a good clean-up, but if we are committed to breaking people, we should make sure that it's change that we are really happy with.

I'm going to kickoff some conversations internally on our side, but if possible it would be ideal to hold off on merging this for a bit to give us a chance to get our ducks in a row.

Absolutely

We should discuss if this is something that is worth doing. To my mind, it's a good clean-up, but if we are committed to breaking people, we should make sure that it's change that we are really happy with.

I'm a big +1 to this change and it's much appreciated.

Regardless, after the meeting this morning and this RAPIDS release, we've decided as a team that we're going to start constraining Dask to its latest release version at the time of our release instead of letting it float. Breaking changes are going in too frequently and Dask releases go out in between our releases too frequently, where repeatedly fixing our previous release is too expensive for our team to do at this time.

This PR would break a lot less if the functions that are being registered were private. Is that already the implication? If it is not then I would be happy to open a smaller PR that prefixes them all with an underscore.

Ok I looked at dask_cudf and tried to preserve the imports that are used there. Would someone be able to run the cudf tests on this PR and let me know if there are any failures?

@kkraus14 and @quasiben can you test this with cudf and let me know if there are failures.

Maybe now that GTC is over :)

@charlesbluca would you have time to run these tests and report back ?

Ran through the dask-cudf tests, here are the results - lots of failures, but not sure if that's because of my env:

$ conda list | grep -e "dask\|cudf"
cudf                      0.20.0a210419   cuda_11.2_py38_g867d6eecca_166    rapidsai-nightly
dask                      2.9.0+733.g881a7474           dev_0    <develop>
dask-cudf                 0.20.0a210419   py38_g867d6eecca_166    rapidsai-nightly
libcudf                   0.20.0a210419   cuda11.2_g808262f49c_167    rapidsai-nightly

Let me know if I should change anything and rerun the tests.

test_results.txt

here is an example of an error:

________________________ test_categorical_basic[data0] _________________________

data = ['a', 'a', 'b', 'c', 'a']
Categories (3, object): ['a', 'b', 'c']

    @pytest.mark.parametrize("data", [data_cat_1()])
    def test_categorical_basic(data):
        cat = data.copy()
        pdsr = pd.Series(cat)
        sr = Series(cat)
        dsr = dgd.from_cudf(sr, npartitions=2)
        result = dsr.compute()
        np.testing.assert_array_equal(cat.codes, result.to_array())

        assert dsr.dtype.to_pandas() == pdsr.dtype
        # Test attributes
        assert pdsr.cat.ordered == dsr.cat.ordered

        assert_eq(pdsr.cat.categories, dsr.cat.categories)

        np.testing.assert_array_equal(pdsr.cat.codes.values, result.to_array())

        string = str(result)
        expect_str = """
    0 a
    1 a
    2 b
    3 c
    4 a
    """
        assert all(x == y for x, y in zip(string.split(), expect_str.split()))

        df = DataFrame()
        df["a"] = ["xyz", "abc", "def"] * 10

        pdf = df.to_pandas()
        cddf = dgd.from_cudf(df, 1)
>       cddf["b"] = cddf["a"].astype("category")

test_accessor.py:142:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../../../dask-dispatch-test/dask/dataframe/core.py:3885: in __setitem__
    df = self.assign(**{key: value})
../../../../../dask-dispatch-test/dask/dataframe/core.py:4196: in assign
    df2 = self._meta_nonempty.assign(**_extract_meta(kwargs, nonempty=True))
../../../../../dask-dispatch-test/dask/dataframe/core.py:5496: in _extract_meta
    res[k] = _extract_meta(x[k], nonempty)
../../../../../dask-dispatch-test/dask/dataframe/core.py:5488: in _extract_meta
    return x._meta_nonempty if nonempty else x._meta
../../../../../dask-dispatch-test/dask/dataframe/core.py:375: in _meta_nonempty
    return meta_nonempty(self._meta)
../../../../../dask-dispatch-test/dask/utils.py:512: in __call__
    return meth(arg, *args, **kwargs)
../backends.py:96: in _nonempty_series
    data = _get_non_empty_data(s)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

s = Series([], Name: a, dtype: category
Categories (0, dtype: category)

    def _get_non_empty_data(s):
        if isinstance(s._column, cudf.core.column.CategoricalColumn):
            categories = (
                s._column.categories
                if len(s._column.categories)
                else [UNKNOWN_CATEGORIES]
            )
>           codes = column.full(size=2, fill_value=0, dtype="int32")
E           NameError: name 'column' is not defined

@galipremsagar @kkraus14 do you have an idea of what's going on here?

      codes = column.full(size=2, fill_value=0, dtype="int32")

E NameError: name 'column' is not defined

@galipremsagar @kkraus14 do you have an idea of what's going on here?

This is a bug that has to be fixed in dask-cudf, column -> cudf.core.column

This is a bug that has to be fixed in dask-cudf, column -> cudf.core.column

@charlesbluca can you confirm if making just this change unblocks you for validating against changes in this PR?

Yes, that resolves all failures - thanks @galipremsagar! Should I open a PR with cuDF for this?

Yes, that resolves all failures - thanks @galipremsagar! Should I open a PR with cuDF for this?

Thanks for confirming, I'm on it 👍

Thanks @charlesbluca, @quasiben, and @galipremsagar! So seems like this is safe from the dask-cudf perspective?

Thanks @charlesbluca, @quasiben, and @galipremsagar! So seems like this is safe from the dask-cudf perspective?

Yes. Generally we are now constraining dask / distributed versions at release time to the latest released patch version so we should be more protected against breaking changes being introduced.

Ok @jrbourbeau should we merge this and #7505?

Can we get this PR to be merged? This will help unblock other PRs around having major changes in dispatch: #7586

cc: @jakirkham @quasiben @jrbourbeau

+1 from me

@jrbourbeau do you have any thoughts? Should we merge this and the array PR ( #7505 )?

James is fixing up #7505 to match the pattern in this PR and then we'll merge both.

I think this may have broken the use of make_meta. (I also see @GenevieveBuckley noted this).

>>> dd.utils.make_meta([('a', 'i8'), ('b', 'O')])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/croat/.local/share/virtualenvs/starmap-T47byR32/lib/python3.8/site-packages/dask/utils.py", line 511, in __call__
    meth = self.dispatch(type(arg))
  File "/home/croat/.local/share/virtualenvs/starmap-T47byR32/lib/python3.8/site-packages/dask/utils.py", line 505, in dispatch
    raise TypeError("No dispatch for {0}".format(cls))
TypeError: No dispatch for <class 'list'>

I think this may have broken the use of make_meta. (I also see @GenevieveBuckley noted this).

>>> dd.utils.make_meta([('a', 'i8'), ('b', 'O')])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/croat/.local/share/virtualenvs/starmap-T47byR32/lib/python3.8/site-packages/dask/utils.py", line 511, in __call__
    meth = self.dispatch(type(arg))
  File "/home/croat/.local/share/virtualenvs/starmap-T47byR32/lib/python3.8/site-packages/dask/utils.py", line 505, in dispatch
    raise TypeError("No dispatch for {0}".format(cls))
TypeError: No dispatch for <class 'list'>

In #7586, there was a new function introduced that has to be invoked to make meta i.e.,
make_meta_util: dd.utils.make_meta_util and not make_meta directly.

@galipremsagar make_meta_util calls make_meta for things like a list, so it fails in the same way:

$ python
Python 3.8.5 (default, Jan 27 2021, 15:41:15) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import dask
>>> dask.__version__
'2021.05.1'
>>> import dask.dataframe as dd
>>> dd.utils.make_meta_util([('a', 'i8'), ('b', 'O')])
Traceback (most recent call last):
  File "/home/croat/make_meta/venv/lib/python3.8/site-packages/dask/dataframe/dispatch.py", line 100, in make_meta_util
    return make_meta(x, index=index)
  File "/home/croat/make_meta/venv/lib/python3.8/site-packages/dask/utils.py", line 511, in __call__
    meth = self.dispatch(type(arg))
  File "/home/croat/make_meta/venv/lib/python3.8/site-packages/dask/utils.py", line 505, in dispatch
    raise TypeError("No dispatch for {0}".format(cls))
TypeError: No dispatch for <class 'list'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/croat/make_meta/venv/lib/python3.8/site-packages/dask/dataframe/dispatch.py", line 106, in make_meta_util
    func = make_meta_obj.dispatch(type(x))
  File "/home/croat/make_meta/venv/lib/python3.8/site-packages/dask/utils.py", line 505, in dispatch
    raise TypeError("No dispatch for {0}".format(cls))
TypeError: No dispatch for <class 'list'>
>>> 

@chrisroat could you please file this in an issue? This discussion is a bit hard to track in a merged PR, which seems unlikely to be related to the issue at hand

#7731