Skip to content
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

Merged
merged 3 commits into from
May 9, 2022

Conversation

jakirkham
Copy link
Member

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.


  • Tests added / passed
  • Passes pre-commit run --all-files

Also drop `ensure_bytes` implementation & tests from Distributed.
@jakirkham
Copy link
Member Author

This will likely fail until PR ( dask/dask#9050 ) is merged.

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 @jakirkham. I left one comment about going through a quick deprecation cycle, but overall this looks good to me

distributed/utils.py Outdated Show resolved Hide resolved
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.
Comment on lines +1004 to +1010
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,
)
Copy link
Member Author

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.

@jakirkham jakirkham marked this pull request as ready for review May 6, 2022 20:51
@jakirkham jakirkham requested a review from jrbourbeau May 6, 2022 20:51
@jakirkham
Copy link
Member Author

Thanks James! 😄 Should be ready for another look 🙂

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2022

Unit Test Results

       16 files  ±  0           1 errors    15 suites  ±0   7h 16m 9s ⏱️ + 5m 42s
  2 764 tests  -   3    2 685 ✔️  -   1    78 💤  -   1  1  - 1 
20 506 runs   - 27  19 600 ✔️  - 14  905 💤  - 12  1  - 1 

For more details on these parsing errors and failures, see this check.

Results for commit 69d1f44. ± Comparison against base commit 52b6920.

@jakirkham
Copy link
Member Author

@jrbourbeau any more thoughts on this? 🙂

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 @jakirkham

@jrbourbeau jrbourbeau merged commit decfe7f into dask:main May 9, 2022
@jakirkham jakirkham deleted the use_dask_ensure_bytes branch May 9, 2022 23:02
@jakirkham
Copy link
Member Author

Thanks James! 😄

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