-
-
Notifications
You must be signed in to change notification settings - Fork 718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ensure_bytes
from dask.utils
#6295
Conversation
Also drop `ensure_bytes` implementation & tests from Distributed.
cadb0bf
to
3cf1b23
Compare
This will likely fail until PR ( dask/dask#9050 ) is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jakirkham. I left one comment about going through a quick deprecation cycle, but overall this looks good to me
Keep `ensure_bytes` around and have it call the `dask.utils` implementation. Though have it raise a `DeprecationWarning` so users know this will be going away in the future and should update their code.
warnings.warn( | ||
"`distributed.utils.ensure_bytes` is deprecated. " | ||
"Please switch to `dask.utils.ensure_bytes`. " | ||
"This will be removed in `2022.6.0`.", | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if this looks alright.
Also picked 2022.6.0
as the point when we would remove this. Though that is somewhat arbitrary. Happy to change if needed.
Thanks James! 😄 Should be ready for another look 🙂 |
Unit Test Results 16 files ± 0 1 errors 15 suites ±0 7h 16m 9s ⏱️ + 5m 42s For more details on these parsing errors and failures, see this check. Results for commit 69d1f44. ± Comparison against base commit 52b6920. |
@jrbourbeau any more thoughts on this? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jakirkham
Thanks James! 😄 |
Depends on PR ( dask/dask#9050 )
Drops internal
ensure_bytes
implementation in favor Dask's implementation. Thus getting rid of some code duplication that we've been carrying around.pre-commit run --all-files