Skip to content

Fix map_blocks not using own arguments in name generation#8462

Merged
jrbourbeau merged 3 commits intodask:mainfrom
djhoese:bugfix-mblocks-unique-name
Dec 7, 2021
Merged

Fix map_blocks not using own arguments in name generation#8462
jrbourbeau merged 3 commits intodask:mainfrom
djhoese:bugfix-mblocks-unique-name

Conversation

@djhoese
Copy link
Contributor

@djhoese djhoese commented Dec 7, 2021

See #8450 for the full discussion. This PR adds (as suggested by @gjoseph92) the map_blocks argument's chunks, dtype, new_axis, and drop_axis to the call to tokenize so they are included as part of the result's name. Without this it is possible to get two results that should be treated differently by the scheduler but the scheduler won't know since they have the same name. I did verify that all of these tests fail without the fix.

@github-actions github-actions bot added the array label Dec 7, 2021
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 @djhoese! I left one small comment about the tests that were added, but overall the changes here look good

Copy link
Collaborator

@gjoseph92 gjoseph92 left a comment

Choose a reason for hiding this comment

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

LGTM. The tests are a little more than I was expecting—I thought you'd only test that x.name changed with the different arguments—but I'm not sure if the other related behavior (chunks, dtype, drop_axis, new_axis) is insufficiently tested, so these extra tests are worthwhile.

@djhoese
Copy link
Contributor Author

djhoese commented Dec 7, 2021

I thought you'd only test that x.name changed with the different arguments

That is what I'm doing here. Each of these arguments has an effect on the name so I wanted a test for each one, but also wanted to make sure that the functions were returning what was expected. I see what you're saying and I initially wanted to just add these .name checks to an existing test but couldn't think of a clean way to do it with pytest.mark.parametrize which is the only way I can think of swapping out the arguments in a pretty way without losing the original purpose of the tests.

I can simplify these tests if either of you would like me to.

@jrbourbeau jrbourbeau mentioned this pull request Dec 7, 2021
@jrbourbeau jrbourbeau changed the title Fix map_blocks not using own arguments in name generation Fix map_blocks not using own arguments in name generation Dec 7, 2021
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 @djhoese!

Note: The gpuCI failure is unrelated to the changes here and being tracked over in #8465

@jrbourbeau jrbourbeau merged commit d14d6d0 into dask:main Dec 7, 2021
@djhoese djhoese deleted the bugfix-mblocks-unique-name branch December 8, 2021 01:51
@gjoseph92 gjoseph92 mentioned this pull request Dec 16, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chunks/dtype not considered with map_blocks result name

3 participants