Skip to content

Change the active-fusion default to False for Dask-Dataframe#7620

Merged
jrbourbeau merged 9 commits intodask:mainfrom
rjzamora:dataframe-disable-fusion
May 11, 2021
Merged

Change the active-fusion default to False for Dask-Dataframe#7620
jrbourbeau merged 9 commits intodask:mainfrom
rjzamora:dataframe-disable-fusion

Conversation

@rjzamora
Copy link
Member

Now that #7415 is merged, low-level fusion is unnecessary for most Dask-Dataframe workflows (parquet, csv, and orc leverage Blockwise fusion, and other IO mechanisms are on their way to Blockwise). This PR changes the configuration default to be None for "optimization.fuse.active". In dask.dataframe.optimize, a None configuration setting will be treated as False, while everywhere else the None setting will be treated as True.

To clarify: This PR should only change fusion behavior for Dask-Dataframe.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rjzamora! FWIW I'm running the distributed test suite against this PR over in https://github.com/jrbourbeau/distributed/tree/test-dask-7620. Are there any other tests we should run before merging this?

)

with pytest.raises(ValueError):
with pytest.raises((ValueError, RuntimeWarning)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we start raising a RuntimeWarning?

Copy link
Member Author

@rjzamora rjzamora May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - Good question. We seem to be taking a different code path with fusion disabled, so we are running into a true-division RunTimeWarning rather than a "No non-trivial arrays found" ValueError. I'll poke around a bit before taking a stance on whether or not this is a problem :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only lingering thing to address. Any insight into why we started raising a RuntimeWarning? I'm also happy to look into it

@jrbourbeau
Copy link
Member

Just a heads up that the distributed CI run against this PR turned up a couple of test failures

@rjzamora
Copy link
Member Author

rjzamora commented May 4, 2021

Thanks James! Looking into the test_futures_in_subgraphs failure now.

EDIT: I don't have a great understanding of the root cause yet, but the failure is definitely set off by a lack of low-level fusion in the compute_as_if_collection call within categorize. One possible "fix" is to set the "fuse.active" default to True when the input graph is already materialized. However, I still want to understand the root cause before I formally suggest something like this.

@rjzamora
Copy link
Member Author

rjzamora commented May 5, 2021

@jrbourbeau - I seem to have all distributed tests passing with the latest commit. However, I am still not sure why/how the key_dependencies (populated by the HLG cull) are causing problems when the input graph is already materialized.

@rjzamora
Copy link
Member Author

rjzamora commented May 6, 2021

Update: It seems that the root cause was that the output dependencies were not including the known_key_dependencies in HLG dask_distributed_pack. The fix was just added here, and now distributed tests are passing for me locally.

Comment on lines +20 to +24
else:
# Perform Blockwise optimizations for HLG input
dsk = optimize_dataframe_getitem(dsk, keys=keys)
dsk = optimize_blockwise(dsk, keys=keys)
dsk = fuse_roots(dsk, keys=keys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own understanding, we're adding the else block here because none of these optimizations will apply to a HLG which is only MaterializedLayers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - Thanks for calling this out. My understadning is that these optimizations only make sense for Blockwise Layers.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rjzamora!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants