Skip to content

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 jakirkham force-pushed the use_dask_ensure_bytes branch from cadb0bf to 3cf1b23 Compare May 6, 2022 17:54
@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

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