FIX-#4564: Register custom serializer to avoid Ray race condition #4568

devin-petersohn · 2022-06-10T14:24:47Z

Signed-off-by: Devin Petersohn devin.petersohn@gmail.com

What do these changes do?

commit message follows format outlined here
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves BUG: ray 1.13 breaks CI, including test_binary and test_pickle, with circular import #4564
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date
added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

codecov · 2022-06-10T14:42:17Z

Codecov Report

Merging #4568 (13f964b) into master (d5ff94e) will increase coverage by 19.30%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #4568       +/-   ##
===========================================
+ Coverage   70.08%   89.38%   +19.30%     
===========================================
  Files         228      229        +1     
  Lines       18438    18714      +276     
===========================================
+ Hits        12922    16728     +3806     
+ Misses       5516     1986     -3530

Impacted Files	Coverage Δ
modin/core/execution/ray/common/utils.py	`96.87% <100.00%> (+6.25%)`	⬆️
modin/experimental/batch/test/test_pipeline.py	`100.00% <0.00%> (ø)`
modin/pandas/series.py	`94.00% <0.00%> (+0.23%)`	⬆️
modin/pandas/dataframe.py	`91.47% <0.00%> (+0.64%)`	⬆️
modin/pandas/groupby.py	`93.68% <0.00%> (+0.72%)`	⬆️
modin/pandas/base.py	`94.81% <0.00%> (+0.87%)`	⬆️
...mentations/pandas_on_ray/partitioning/partition.py	`93.57% <0.00%> (+1.83%)`	⬆️
modin/config/envvars.py	`86.63% <0.00%> (+3.46%)`	⬆️
modin/core/execution/dask/common/engine_wrapper.py	`100.00% <0.00%> (+5.55%)`	⬆️
... and 71 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

RehanSD

LGTM! Pleased to release notes!

lgtm-com · 2022-06-10T17:09:42Z

This pull request introduces 1 alert when merging f2031ed00a1f5b660a185cd1d972b5253a0cb9ab into d5ff94e - view on LGTM.com

new alerts:

1 for Module is imported more than once

YarShev

Please fix the PR title, link an issue to be resolved and add a release note with your GH nickname.

modin/core/execution/ray/common/utils.py

…e condition (modin-project#4568) Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

mvashishtha · 2022-06-10T19:52:53Z

@devin-petersohn You have "customer" instead of "custom" in the PR title and first commit description.

mvashishtha

LGTM, Thanks!

RehanSD

LGTM! We should conduct a perf check to see how pickle compares to Ray's serializer.

devin-petersohn · 2022-06-10T19:54:05Z

@devin-petersohn You have "customer" instead of "custom" in the PR title and first commit description.

🤣 fixed

mvashishtha · 2022-06-10T19:55:05Z

@devin-petersohn "customer" is still in the first commit description. It's fine by me to leave it that way.

RehanSD

Please add your name to the contributors in the release notes!

docs/release_notes/release_notes-0.16.0.rst

Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

RehanSD

LGTM!

devin-petersohn · 2022-06-11T01:58:41Z

@modin-project/modin-ray is this the recommended way of fixing this? It seems like a bad idea but it is the only way we could use the latest ray release without a race condition

vnlitvinov

LGTM, though I would like to find a better way... I wish it existed 😞

vnlitvinov · 2022-06-11T08:44:03Z

@devin-petersohn a good point here:

LGTM! We should conduct a perf check to see how pickle compares to Ray's serializer.

Could someone please check some benchmarks?

NickCrews · 2022-06-13T04:27:16Z

Drive-by-ing here, IDK if you have a good way of running benchmarks, but perhaps the github-actions bot that we wrote for the dedupe library could be useful to you. Take a look at this comment to see how it is used. Makes it really easy for us to test performance changes on a PR.

devin-petersohn · 2022-06-13T14:25:20Z

Thanks @NickCrews, we do have asv running on every pulled-in commit: https://modin.org/modin-bench/#/

Having it run as a job for pull requests would be nice, with our current setup we can't do that. I will definitely take a look!

@vnlitvinov I have run some performance benchmark numbers and they don't look good at all. On small-moderate datasets we see 2x performance degradation across the board. Extra serialization + storage costs 200ms for every 50MB serialized, and the memory usage is higher across the board.

I think we need to pin Ray<1.13, as much as I hate doing that. I cannot justify the increased performance penalty.

devin-petersohn · 2022-06-13T14:36:43Z

I have converted this pull request to a draft, hoping we can get some response from @modin-project/modin-ray .

Linking to the ray issue I created here: ray-project/ray#25675

devin-petersohn · 2022-06-21T15:13:25Z

Closing because we chose to go a different direction.

devin-petersohn requested a review from a team as a code owner June 10, 2022 14:24

devin-petersohn mentioned this pull request Jun 10, 2022

BUG: ray 1.13 breaks CI, including test_binary and test_pickle, with circular import #4564

Closed

RehanSD previously approved these changes Jun 10, 2022

View reviewed changes

YarShev reviewed Jun 10, 2022

View reviewed changes

modin/core/execution/ray/common/utils.py Show resolved Hide resolved

devin-petersohn added 4 commits June 10, 2022 14:42

FIX-modin-project#4564: Register customer serializer to avoid Ray rac…

75ec035

…e condition (modin-project#4568) Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

Lint

163cdd6

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

create custom serializer

569fb0c

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

Update

f5fe1f4

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

devin-petersohn dismissed RehanSD’s stale review via f5fe1f4 June 10, 2022 19:44

devin-petersohn force-pushed the dep/ray-1.13 branch from f2031ed to f5fe1f4 Compare June 10, 2022 19:44

devin-petersohn changed the title ~~FIX-#0000: Sleep for 1 second to avoid Ray's race condition~~ FIX-#4564: Register customer serializer to avoid Ray race condition Jun 10, 2022

mvashishtha previously approved these changes Jun 10, 2022

View reviewed changes

devin-petersohn changed the title ~~FIX-#4564: Register customer serializer to avoid Ray race condition~~ FIX-#4564: Register custom serializer to avoid Ray race condition Jun 10, 2022

RehanSD approved these changes Jun 10, 2022

View reviewed changes

RehanSD requested changes Jun 10, 2022

View reviewed changes

YarShev reviewed Jun 10, 2022

View reviewed changes

docs/release_notes/release_notes-0.16.0.rst Outdated Show resolved Hide resolved

Update docs/release_notes/release_notes-0.16.0.rst

2ad9236

Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>

devin-petersohn dismissed mvashishtha’s stale review via 2ad9236 June 10, 2022 20:29

Add my name

13f964b

Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>

naren-ponder approved these changes Jun 10, 2022

View reviewed changes

RehanSD approved these changes Jun 10, 2022

View reviewed changes

YarShev approved these changes Jun 10, 2022

View reviewed changes

vnlitvinov approved these changes Jun 11, 2022

View reviewed changes

This was referenced Jun 12, 2022

FIX-#4566: Pin Ray < 1.13.0 to avoid deserialization race condition. #4567

Merged

FIX-#4570: Replace np.bool -> np.bool_ #4571

Merged

devin-petersohn marked this pull request as draft June 13, 2022 14:34

devin-petersohn closed this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX-#4564: Register custom serializer to avoid Ray race condition #4568

FIX-#4564: Register custom serializer to avoid Ray race condition #4568

devin-petersohn commented Jun 10, 2022 •

edited by YarShev

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

RehanSD left a comment

lgtm-com bot commented Jun 10, 2022

YarShev left a comment

mvashishtha commented Jun 10, 2022

mvashishtha left a comment

RehanSD left a comment

devin-petersohn commented Jun 10, 2022

mvashishtha commented Jun 10, 2022

RehanSD left a comment

RehanSD left a comment

devin-petersohn commented Jun 11, 2022

vnlitvinov left a comment

vnlitvinov commented Jun 11, 2022

NickCrews commented Jun 13, 2022

devin-petersohn commented Jun 13, 2022

devin-petersohn commented Jun 13, 2022

devin-petersohn commented Jun 21, 2022

FIX-#4564: Register custom serializer to avoid Ray race condition #4568

FIX-#4564: Register custom serializer to avoid Ray race condition #4568

Conversation

devin-petersohn commented Jun 10, 2022 • edited by YarShev Loading

What do these changes do?

codecov bot commented Jun 10, 2022 • edited Loading

Codecov Report

RehanSD left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 10, 2022

YarShev left a comment

Choose a reason for hiding this comment

mvashishtha commented Jun 10, 2022

mvashishtha left a comment

Choose a reason for hiding this comment

RehanSD left a comment

Choose a reason for hiding this comment

devin-petersohn commented Jun 10, 2022

mvashishtha commented Jun 10, 2022

RehanSD left a comment

Choose a reason for hiding this comment

RehanSD left a comment

Choose a reason for hiding this comment

devin-petersohn commented Jun 11, 2022

vnlitvinov left a comment

Choose a reason for hiding this comment

vnlitvinov commented Jun 11, 2022

NickCrews commented Jun 13, 2022

devin-petersohn commented Jun 13, 2022

devin-petersohn commented Jun 13, 2022

devin-petersohn commented Jun 21, 2022

devin-petersohn commented Jun 10, 2022 •

edited by YarShev

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading