Skip to content

[WIP] Add optional pass_key argument to map_tasks#6761

Closed
rjzamora wants to merge 2 commits intodask:masterfrom
rjzamora:map-tasks-keys
Closed

[WIP] Add optional pass_key argument to map_tasks#6761
rjzamora wants to merge 2 commits intodask:masterfrom
rjzamora:map-tasks-keys

Conversation

@rjzamora
Copy link
Member

@rjzamora rjzamora commented Oct 22, 2020

Adds an optional pass_key argument to map_tasks. This enables a simple fix to distributed#4177, because it allows the map_tasks utility to be used to collect proper task-future dependencies.

  • Tests added / passed
  • Passes black dask / flake8 dask

Copy link
Contributor

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

I think this is a great feature @rjzamora but I am a bit concerned with the Blockwise implementation of the key (see my comment).

But if we can get this to work, let's make the key argument mandatory. All the map_tasks() implementations need to support it anyways.

if input_indices is None: # A literal
new_indices.append((func([key]), None))
if pass_key:
new_indices.append((func([key], key), None))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is strictly correct. key here is a literal and not a graph key. See

dask/dask/blockwise.py

Lines 380 to 382 in 97c2f06

>>> make_blockwise_graph(add, 'z', 'i', 'x', 'i', 100, None, numblocks={'x': (2,)}) # doctest: +SKIP
{('z', 0): (add, ('x', 0), 100),
('z', 1): (add, ('x', 1), 100)}

Co-authored-by: Mads R. B. Kristensen <madsbk@gmail.com>
@jrbourbeau
Copy link
Member

Is it safe to close this PR since we ended up going another direction in dask/distributed#4178?

@rjzamora
Copy link
Member Author

Yeah - It is unlikely we will need this change.

@rjzamora rjzamora closed this Oct 23, 2020
@rjzamora rjzamora deleted the map-tasks-keys branch May 21, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants