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

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

Merged

Conversation

mvashishtha
Copy link
Collaborator

Signed-off-by: mvashishtha mahesh@ponder.io

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: We need to pin ray < 1.13.0 until we fix deserialization race condition bug #4564. #4566
  • 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

…e condition.

Signed-off-by: mvashishtha <mahesh@ponder.io>
@mvashishtha mvashishtha requested a review from a team as a code owner June 10, 2022 10:33
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #4567 (f820a7d) into master (ba2b6ea) will decrease coverage by 15.15%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #4567       +/-   ##
===========================================
- Coverage   86.34%   71.18%   -15.16%     
===========================================
  Files         228      229        +1     
  Lines       18438    18715      +277     
===========================================
- Hits        15920    13323     -2597     
- Misses       2518     5392     +2874     
Impacted Files Coverage Δ
modin/config/envvars.py 78.71% <100.00%> (-4.46%) ⬇️
modin/utils.py 79.72% <100.00%> (+0.09%) ⬆️
...odin/experimental/core/storage_formats/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...din/experimental/core/execution/native/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...erimental/core/storage_formats/omnisci/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
.../core/execution/native/implementations/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...tive/implementations/omnisci_on_native/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...e/implementations/omnisci_on_native/io/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...mentations/omnisci_on_native/dataframe/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...tations/omnisci_on_native/partitioning/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
... and 49 more

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

vnlitvinov
vnlitvinov previously approved these changes Jun 10, 2022
Copy link
Collaborator

@vnlitvinov vnlitvinov left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@YarShev YarShev left a comment

Choose a reason for hiding this comment

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

Should we also add a condition on the upper ray bound here?

if version.parse(ray.__version__) < MIN_RAY_VERSION:

Signed-off-by: mvashishtha <mahesh@ponder.io>
@mvashishtha
Copy link
Collaborator Author

Should we also add a condition on the upper ray bound here?

if version.parse(ray.__version__) < MIN_RAY_VERSION:

@YarShev yes, done.

@RehanSD
Copy link
Collaborator

RehanSD commented Jun 12, 2022

Is this superseded by #4568 ?

Copy link
Collaborator

@devin-petersohn devin-petersohn left a comment

Choose a reason for hiding this comment

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

@mvashishtha doc build is failing because changes here are not compatible with docs/requirements-doc.txt. For some reason that is installing from the github.

@devin-petersohn
Copy link
Collaborator

This probably needs to change:

git+https://github.com/modin-project/modin.git@master#egg=modin[all]

@mvashishtha
Copy link
Collaborator Author

This probably needs to change:

git+https://github.com/modin-project/modin.git@master#egg=modin[all]

@devin-petersohn I saw that failure, but what should we change it to? How can we install modin using requirements from the current PR?

@devin-petersohn
Copy link
Collaborator

@mvashishtha will you delete that line and see if that fixes it?

mvashishtha added 3 commits June 13, 2022 10:13
…version of ray.

Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
@mvashishtha
Copy link
Collaborator Author

@mvashishtha will you delete that line and see if that fixes it?

@devin-petersohn that didn't work either. "Build docs" gives No module named 'ray' as here. It doesn't look like I can pip install ../.[all] either. I'm trying to pin ray within the doc requirements.

@devin-petersohn devin-petersohn merged commit d231df0 into modin-project:master Jun 13, 2022
vnlitvinov pushed a commit that referenced this pull request Jun 16, 2022
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.

BUG: We need to pin ray < 1.13.0 until we fix deserialization race condition bug #4564.
5 participants