Can we lose the doctest: +SKIP?

I opted to remove the sorted() call instead.

Why separate this into two functions?

Because the memoize decorator needs hashable arguments (which lists are not).

Try @memoize(key=tuple)

Also, should we be concerned about the memoized function holding on to too many inputs?

I don't think so, unless someone has a need for a large number of locations for newaxis in their indices.

Try @memoize(key=tuple)

The signature for the key argument isn't very pretty (it takes a args, kwargs tuple). I wonder if it's not more readable to keep the two-argument variant.

Given the new simplified structure of get_dependencies is this function still useful?

Yes, because passing a bunch of tasks at once is more efficient than calling _deps() for each one.
(note _deps() can be very fast on trivial entries)

Same question as above. Do we still need this function?

Ditto.

In the case above I now see that we're sending a list of tasks. In this case though we're just using a dict comprehension. That dict-comprehension could presumably be inlined. I suppose the gains here are based on the type checks and avoiding the extra function call?

Yes, they are. We could further inline the dict comprehension, but at the price of code duplication, so I'm not sure that it's a good idea.

If it's a single line dict comprehension then I would prefer the code duplication. My opinion is that code indirection (having to follow many different functions to find the logic you want) can be as bad as code duplication in some cases.

Ah... I'll take a look.

_deps() is a private API, should we make it public? Use get_dependencies instead (at the price of a small slowdown - cull() seems to become 15% slower)?

Thoughts on removing _deps entirely and folding it into get_dependencies? We could place the burden of arg/task inputs and list/set outputs on the caller.

This can also happen later. Mostly I want to push on the principle of "there is a maintenance cost to having lots of functions that do similar things". If we can have a few of these that compose well with standard language constructs like dictionary comprehensions then I think that dask.core will remain an easy place to get started for newish developers.

Definitely agreed. I'll try to fold _deps into get_dependencies.

Why are these necessary?

I'm trying to fix the following failure:
https://travis-ci.org/dask/dask/jobs/172618031#L731
I'm not sure why the build worked previously.

Now I'm getting another similar issue with s3fs: https://travis-ci.org/dask/dask/jobs/172623691#L751
Do you have an idea what may be happening?

s3fs is included as a dependency in the distributed conda package but not in the PyPI package. We keep the PyPI packages lightweight.

Why was this added?

It's not added, it's part of the _deps() algorithm, rewritten iteratively, and folded here.

I see, thanks. I'm confused why we want to do this with dicts. This was added in 05ddd68.

In [20]: d = {'x': 1, 'y': {'a': 'x'}}

In [21]: dask.core.get_dependencies(d, 'y')
Out[21]: {'x'}

In [22]: dask.core.get(d, ['y'])
Out[22]: ({'a': 'x'},)

In [23]: dask.get(d, ['y'])
---------------------------------------------------------------------------
KeyError: 'y'

No idea from me :-)

@eriknw we do this for the distributed scheduler. It ended up being useful, though at the moment I don't particularly recall the original use case.

In [1]: from distributed import Client
In [2]: c = Client()
In [3]: d = {'x': 1, 'y': {'a': 'x'}}
In [4]: c.get(d, 'y')
Out[4]: {'a': 1}

I agree the dict solution doesn't look pretty.

Here's a simple, non-recursive helper function that makes the intent explicit and doesn't require changing the spec:

In [35]: def subdask(d):
    ...:     return (dict, (zip, tuple(d.keys()), list(d.values())))
    ...: 
    ...: d = {
    ...:     'a': 1,
    ...:     'b': 2,
    ...:     'c': subdask({'a': 'a', 'b': 'b'}),
    ...: }
    ...: 

In [36]: d
Out[36]: {'a': 1, 'b': 2, 'c': (dict, (<function zip>, ('a', 'b'), ['a', 'b']))}

In [37]: dask.core.get(d, 'c')
Out[37]: {'a': 1, 'b': 2}

By the way, I'm assuming this discussion isn't an issue for the current PR, right? :-)

Yeah. The PR lgtm. It keeps the existing behavior with dicts, which we can revisit elsewhere.

#1748