Remove bad graph logic in BlockwiseIO by rjzamora · Pull Request #6933 · dask/dask

rjzamora · 2020-12-04T05:41:12Z

Fixes incorrect logic being used to build dsk in the BlockwiseIO constructor.

Currently, the number of dimensions is used to determine the number of dummy arguments. However, the number of dummy argument should always be one dummy argument for IO.

Thanks to @ian-r-rose for pointing out this problem in #6931

Tests added / passed
Passes black dask / flake8 dask

jrbourbeau

Thanks @rjzamora! Just for my own understanding this, we only need blockwise_token(0) here because PackedFunctionCall gets a tuple of arguments at run time?

rjzamora · 2020-12-04T16:42:46Z

Thanks @rjzamora! Just for my own understanding this, we only need blockwise_token(0) here because PackedFunctionCall gets a tuple of arguments at run time?

Right - This is my understanding. The number of dummy arguments depends on the number of inputs to the underlying function, and BlockwiseIO should always have a single input (the tuple that you mentioned).

jrbourbeau

Gotcha, thanks. And, so far, ninds has never been >1 since BlockwiseIO was only utilized for DataFrames.

Also, @ian-r-rose is trying out this fix over in #6931

rjzamora · 2020-12-04T19:05:18Z

Also, @ian-r-rose is trying out this fix over in #6931

Sounds good! Note that I locally added this change to that PR and seem to get correct behavior with a series of subsequent blockwise operations:

dask/blockwise.py

mrocklin · 2020-12-08T19:16:07Z

toolz.curry unfortunately hides errors in some situations. We might consider not using it, and instead just defining a few extra functions. I'm guessing that this is coming from the wrap.py module. The curried functions are only used a couple of times, so the magic here probably isn't necessary. I suspect that after a brief removal of toolz.curry a more informative error message will arise.

…

On Tue, Dec 8, 2020 at 10:21 AM Ian Rose ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dask/blockwise.py <#6933 (comment)>: > + # Leave out `new_axes` in key + # TODO: Check that this makes sense + sz = len([i for i in self.output_indices if i not in self.new_axes]) + io_key = (self.io_name,) + tuple([k[i + 1] for i in range(sz)]) Thanks for looking into it @rjzamora <https://github.com/rjzamora>. This seems to help with the original new-axes issue: def f(x): return x[:, None] * np.ones((1, 7)) x = da.ones(5, chunks=2)y = da.blockwise( f, "aq", x, "a", new_axes={"q": 7}, concatenate=True, dtype=x.dtype )y.compute() However, it fails again with the same operation transposed (again derived from the test suite): def f(x): return x[None, :] * np.ones((7, 1)) x = da.ones(5, chunks=2)y = da.blockwise( f, "qa", x, "a", new_axes={"q": 7}, concatenate=True, dtype=x.dtype )y.compute() <ipython-input-3-988e4cf91f2a> in f(x) 1 def f(x): ----> 2 return x[None, :] * np.ones((7, 1)) 3 4 x = da.ones(5, chunks=2) 5 y = da.blockwise( TypeError: 'cytoolz.functoolz.curry' object is not subscriptable I'm puzzled as to what is going on such that the blockwise f(x) is getting fed a curry object rather than an array object, and how it relates to your logic in 4a90128 <4a90128>. Could you elaborate a bit? For what it's worth, these issues seem to disappear when skipping the blockwise fusion <https://github.com/dask/dask/blob/3a08bb32e736d51a758281a3abc0fc0dbec04efc/dask/array/optimization.py#L46> step while optimization. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6933 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACKZTGFW2AQQJMJDDNKOXLSTZVAXANCNFSM4UM7DGDQ> .

ian-r-rose · 2020-12-08T19:45:37Z

dask/blockwise.py

-                # TODO: Check that this makes sense
-                sz = len([i for i in self.output_indices if i not in self.new_axes])
-                io_key = (self.io_name,) + tuple([k[i + 1] for i in range(sz)])
+                io_key = (self.io_name,) + tuple(


👍 Awesome, thanks, making progress. This fixes the above issue.

rjzamora · 2020-12-09T18:59:36Z

@ian-r-rose's latest find demonstrates that two or more BlockwiseIO layers cannot be fused into a single layer, because the class is only designed to handle a single io_name and single io_subgraph. We can certainly expand BlockwiseIO to handle a set of "io_dependencies", but I am a bit unsure if we always want to fuse the creation of multiple collections into the same layer like this. @mrocklin - Do you have any intuition on this?

rjzamora · 2020-12-10T17:36:34Z

@ian-r-rose - The latest change here revises the BlockwiseIO API a bit, but seems to resolve the problem you uncovered. With the latest changes, you will no longer be expected to pass separate io_name and io_subgraph arguments to the constructor. Instead you should pass a single dictionary argument: {<io_name>: <io_subgraph>}

rjzamora · 2020-12-11T15:56:02Z

@jrbourbeau - It may be best to avoid adding any more "fixes" to this PR. Do you have thoughts on getting this in and opening follow-up PRs if working through #6791 and #6931 exposes other problems?

jrbourbeau

Happy to merge this in and continue updating as needed

dask/blockwise.py

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

ian-r-rose

@jrbourbeau - It may be best to avoid adding any more "fixes" to this PR. Do you have thoughts on getting this in and opening follow-up PRs if working through #6791 and #6931 exposes other problems?

I'm happy to move future work on this into #6931 (there are still some cases we will need to track down, I'll post some minimal examples to that PR). @rjzamora feel free to push commits to that branch, though be aware that I've been rebasing regularly.

ian-r-rose · 2020-12-10T23:03:04Z

dask/blockwise.py

+                # If we are not getting a `dsk` input, there must be a single
+                # element in `io_deps`
+                if len(io_deps) != 1:
+                    raise ValueError(


Right - The case where we use the BlockwiseIO constructor without an intial dsk definition could probably handle the creation of multiple subgraphs. However, I couldn't think of any reason why we would need to support.

jrbourbeau

Thanks @rjzamora! I pushed a small commit to make _inject_io_tasks return the updated graph which I think makes things a bit more clear to read -- hope that's okay. Will merge after CI passes

rjzamora · 2020-12-11T20:08:15Z

Thanks @rjzamora! I pushed a small commit to make _inject_io_tasks return the updated graph which I think makes things a bit more clear to read -- hope that's okay. Will merge after CI passes

Sounds good @jrbourbeau - Thanks!

It seems that the _inject_io_tasks will need a further revision to get #6931 working, but I think it makes sense to work the problem out in that PR (or in a follow-up PR).

remove incorrect dim logic in BlockwiseIO

75331b3

rjzamora mentioned this pull request Dec 4, 2020

Initial pass at blockwise array creation routines. #6931

Merged

2 tasks

jrbourbeau reviewed Dec 4, 2020

View reviewed changes

rjzamora and others added 3 commits December 4, 2020 12:05

add missing kwargs

c32c0ec

avoiding new_axes in io-key name

4a90128

format check

d9cd247

rjzamora commented Dec 8, 2020

View reviewed changes

dask/blockwise.py Outdated Show resolved Hide resolved

handle index reordering

6d9f98d

ian-r-rose reviewed Dec 8, 2020

View reviewed changes

rjzamora added 2 commits December 10, 2020 11:24

enable multiple io subgraphs in a single blockwiseio layer

db522b7

update dataframe usage of blockwiseio

5fe4c76

jrbourbeau reviewed Dec 11, 2020

View reviewed changes

dask/blockwise.py Outdated Show resolved Hide resolved

dask/blockwise.py Outdated Show resolved Hide resolved

Update dask/blockwise.py

21a8b44

Co-authored-by: James Bourbeau <jrbourbeau@users.noreply.github.com>

ian-r-rose reviewed Dec 11, 2020

View reviewed changes

rjzamora and others added 2 commits December 11, 2020 11:00

break redundant code into _inject_io_tasks

4cdc896

Update _inject_io_tasks to return graph

8ae7fba

jrbourbeau approved these changes Dec 11, 2020

View reviewed changes

jrbourbeau merged commit 603d064 into dask:master Dec 11, 2020

rjzamora mentioned this pull request Dec 11, 2020

Improve task replacement in BlockwiseIO #6959

Closed

Uh oh!

Conversation

rjzamora commented Dec 4, 2020

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

rjzamora commented Dec 4, 2020

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

rjzamora commented Dec 4, 2020

Uh oh!

Uh oh!

mrocklin commented Dec 8, 2020 via email

Uh oh!

ian-r-rose Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

rjzamora commented Dec 9, 2020

Uh oh!

rjzamora commented Dec 10, 2020

Uh oh!

rjzamora commented Dec 11, 2020

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ian-r-rose left a comment

Choose a reason for hiding this comment

Uh oh!

ian-r-rose Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

rjzamora Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

rjzamora commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants