Use Taskspec fuse implementation #1162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fjetter
commented
Nov 11, 2024
dask_expr/_expr.py
Outdated
Comment on lines
3778
to
3790
| internal_tasks[t.key] = t | ||
| # The above is ambiguous and we have to cull to get an unambiguous graph | ||
| outkey = (self.exprs[0]._name, index) | ||
| work = [outkey] | ||
| internal_tasks_culled = [] | ||
| while work: | ||
| tkey = work.pop() | ||
| if tkey not in internal_tasks: | ||
| # External dependency | ||
| continue | ||
| t = internal_tasks[tkey] | ||
| internal_tasks_culled.append(t) | ||
| work.extend(t.dependencies) |
There was a problem hiding this comment.
All of this shouldn't be necessary if there weren't dead nodes
There was a problem hiding this comment.
Do you have an example with dead nodes? I am curious to see what's going on there
Note: The same is happening in dask/dask when trying to replace the SubgraphCallables :( |
|
I'ver removed the culling, things should be good to go now |
phofl
approved these changes
Nov 13, 2024
|
thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is using the
Task.fuse(see dask/dask#11509) method for the Fused task. In theory this should've reduced complexity quite a bit but I encountered problems since the subgraphs are including dead nodes, i.e. there are expressions in_exprthat are no longer reachable if we walk the tasks. TheTask.fuseraises in such a case since it requires a subgraph that is uniquely reducing to a single task (I put that condition in to catch stuff like this).Adding an explicit culling step in advance fixes this so everything is OK. However, where is this redundant node coming from and can we get rid of this??
cc @phofl