Require additional dependencies: cloudpickle, partd, fsspec, toolz

[jsignell]

• Closes Make trivial dependencies mandatory #7244
• Tests added / passed
• Passes black dask / flake8 dask

I am planning on adding commits to remove handling of cases where these packages aren't available.

[jrbourbeau]

Nice! Thanks @jsignell -- let me know when you'd like someone to review

[jsignell]

This failure has me kind of stumped.

=================================== FAILURES ===================================
_______________________ test_works_with_highlevel_graph ________________________

    def test_works_with_highlevel_graph():
        """Previously `dask.multiprocessing.get` would accidentally forward
        `HighLevelGraph` graphs through the dask optimization/scheduling routines,
        resulting in odd errors. One way to trigger this was to have a
        non-indexable object in a task. This is just a smoketest to ensure that
        things work properly even if `HighLevelGraph` objects get passed to
        `dask.multiprocessing.get`. See https://github.com/dask/dask/issues/7190.
        """
    
        class NoIndex:
            def __init__(self, x):
                self.x = x
    
            def __getitem__(self, key):
                raise Exception("Oh no!")
    
        x = delayed(lambda x: x)(NoIndex(1))
        (res,) = get(x.dask, x.__dask_keys__())
>       assert isinstance(res, NoIndex)
E       AssertionError: assert False
E        +  where False = isinstance(<dask.tests.test_multiprocessing.NoIndex object at 0x7fd42c727490>, <class 'dask.tests.test_multiprocessing.test_works_with_highlevel_graph.<locals>.NoIndex'>)

dask/tests/test_multiprocessing.py:174: AssertionError

[jrbourbeau]

That looks to be an issue with cloudpickle which was fixed in the 1.1.1 release. We can fix things by either moving the NoIndex class definition outside of the body of test_works_with_highlevel_graph or updating our minimum cloudpickle to be >=1.1.1 (which is coming up on being two years old)

[jsignell]

Oh nice find! I say let's bump up our cloudpickle.

[jsignell]

I'm not sure if we care about this, but since it came up in #7358 I thought I'd try it out. I added a skip in conftest.py to support this.

[jsignell]

Ok @crusaderky and @jrbourbeau I think this is ready for you to take a look. I put some questions inline in comments.

[jrbourbeau]

Overall the changes here look good, thanks for working on this @jsignell. Some comments in addition to the ones below:

• There are a couple of pytest.importorskip("tlz") occurances we can remove
• Same with pytest.importorskip("dask.bag") and import_or_none("dask.bag")

[crusaderky]

please fix docstring of dask.multiprocessing.get:

    func_dumps : function
        Function to use for function serialization
        (defaults to cloudpickle.dumps if available, otherwise pickle.dumps)
    func_loads : function
        Function to use for function deserialization
        (defaults to cloudpickle.loads if available, otherwise pickle.loads)

[crusaderky]

In docs/source/bag.rst, please remove:

Because the multiprocessing scheduler requires moving functions between multiple
processes, we encourage that Dask Bag users also install the cloudpickle_ library to
enable the transfer of more complex functions.

[...]
.. _cloudpickle: https://github.com/cloudpipe/cloudpickle

[jsignell]

Ok! I have addressed all the comments. Thanks for the reviews @crusaderky and @jrbourbeau!

[jsignell]

Note that this will require changes in the conda recipes. @martindurant I don't know if we need to let the anaconda conda team know about this. I'll open a PR in the conda-forge recipe now

[martindurant]

Thanks @jsignell . I don't think so normally, but I passed it on anyway.

[jsignell]

🎉 green! 🎉 @jrbourbeau @crusaderky are you happy with the state of this?

[jakirkham]

No need to do anything in this PR, but I'm curious when do we want to bump these and what versions should we bump them to?

[jsignell]

We bumped them a few months ago to this. There was some talk in the maintenance meeting about using released > n months ago as the floor. Apparently xarray does something like this? But we didn't choose a specific policy

[crusaderky]

At xarray and pint we use a rolling policy based on NEP-29:
https://xarray.pydata.org/en/stable/installing.html#minimum-dependency-versions
The number of months listed is arbitrary and subject to negotiation. The key benefit of such a policy is that it allows developer to bump up minimum dependencies as needed, without initiating a discussion every time.

[jakirkham]

Leaving alone is fine. I think we have some workarounds for NumPy pre-1.17 that could be cleaned out once that is a minimum

cc @pentschev (in case you have thoughts here 🙂)

[pentschev]

Yeah, I also agree we could consider dropping older versions of NumPy. As @crusaderky mentioned NEP-29, it's worth noticing that NumPy is supporting Python releases for 18 months. Is there any reason for us to keep on supporting NumPy versions (or all other dependencies for that matter) that are much older than that?

[jsignell]

I opened #7378 to continue this conversation

[jakirkham]

I wonder what we think about bumping this to 1.5.0. That version added support for the pickle5 backport package. Admittedly that is only needed for Python pre-3.8, which is just Python 3.7 for Dask now

[crusaderky]

I don't think dask/dask has any need to directly acknowledge pickle protocol 5? 1.5.0 is very recent...

[jsignell]

distributed pins to 1.5.0, but since that is less than a year old it seemed like overkill for dask/dask.

[jakirkham]

Cloudpickle will use it under-the-hood if installed

Yeah I don't have strong feelings here. Just wanted to raise for discussion. This also will become irrelevant once Python 3.7 is dropped (guessing that will coincide with the Python 3.10 release)

[jrbourbeau]

Thanks @jsignell!