Skip to content

Replace token= keyword with name= in map_blocks#3597

Merged
mrocklin merged 3 commits intodask:masterfrom
mrocklin:map-blocks-name
Jun 13, 2018
Merged

Replace token= keyword with name= in map_blocks#3597
mrocklin merged 3 commits intodask:masterfrom
mrocklin:map-blocks-name

Conversation

@mrocklin
Copy link
Member

Behavior here was reversed from normal dask.array conventions.
This does an abrupt change of the name= semantics and issues a warning
when users use token=, pointing them to name=

See #3226 (comment)

  • Tests added / passed
  • Passes flake8 dask

Behavior here was reversed from normal dask.array conventions.
This does an abrupt change of the name= semantics and issues a warning
when users use token=, pointing them to name=

See dask#3226 (comment)
@mrocklin
Copy link
Member Author

cc @jakirkham

name = '%s-%s' % (token or funcname(func),
tokenize(token or func, args, **kwargs))
if token:
warnings.warn("The token= keyword to map_blocks has been moved to name=")
Copy link
Member

Choose a reason for hiding this comment

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

Should we note when we plan to drop token?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps? When I run into warnings like this the future version number rarely means much to me. I mostly care about "how far into the future will this work?" the answer I get is some version number, which doesn't mean much to me, so I go ahead and change my code anyway.

Happy to adhere to some convention though if it exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any further concern here @jakirkham ?

Copy link
Member

Choose a reason for hiding this comment

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

This point of including versions in deprecations came up once in a NumPy PR I submitted. So it seemed like a fair point to raise. That said, I don't have particularly strong feelings about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, in that case I'll probably pass on this.

@jakirkham
Copy link
Member

Thanks @mrocklin.

This seems fine to me. Had one question about the warning.

The CI failures seemed unrelated. So went ahead and restarted the failing builds.

Was using `name` the old way, which is no longer needed. So changed to
use `name` in the new way.
@jakirkham
Copy link
Member

Found usage of map_blocks(..., name=...) in Array.__getitem__. Did not see any other lingering usages. Have pushed a small commit to your PR to fix this. Also please feel free to tweak it as needed. Hope that is ok.

@mrocklin
Copy link
Member Author

Thanks for the find @jakirkham . I've gone ahead and remove the use of name= there altogether. We should be able to rely on map_blocks to sniff out the function name in that case (this probably wasn't around when the original line was written).

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.

2 participants