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

PERF-#4713: Stop overriding the ray MacOS object store size limit. #4792

Merged

Conversation

mvashishtha
Copy link
Collaborator

What do these changes do?

PERF-#4713: Stop overriding the ray MacOS object store size limit.

  • 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 PERF: Stop overriding Ray's 2 GiB object store size limit on mac #4713
  • 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

@mvashishtha mvashishtha requested a review from a team as a code owner August 6, 2022 20:44
@codecov
Copy link

codecov bot commented Aug 6, 2022

Codecov Report

Merging #4792 (07528c7) into master (83df040) will increase coverage by 4.61%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #4792      +/-   ##
==========================================
+ Coverage   85.24%   89.86%   +4.61%     
==========================================
  Files         259      260       +1     
  Lines       19381    19690     +309     
==========================================
+ Hits        16522    17694    +1172     
+ Misses       2859     1996     -863     
Impacted Files Coverage Δ
modin/core/execution/ray/common/utils.py 88.88% <83.33%> (-6.35%) ⬇️
modin/logging/config.py 94.59% <0.00%> (-1.30%) ⬇️
modin/pandas/indexing.py 90.71% <0.00%> (-0.10%) ⬇️
modin/pandas/groupby.py 93.71% <0.00%> (ø)
modin/pandas/dataframe.py 91.77% <0.00%> (ø)
modin/core/io/text/text_file_dispatcher.py 97.42% <0.00%> (ø)
...odin/core/storage_formats/pandas/query_compiler.py 96.09% <0.00%> (ø)
...tal/core/storage_formats/omnisci/query_compiler.py 94.49% <0.00%> (ø)
modin/experimental/batch/test/test_pipeline.py 100.00% <0.00%> (ø)
modin/core/storage_formats/base/query_compiler.py 99.22% <0.00%> (+<0.01%) ⬆️
... and 43 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vnlitvinov
vnlitvinov previously approved these changes Aug 8, 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.

Overall LGTM, left a few optional questions.

modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
pyrito
pyrito previously approved these changes Aug 8, 2022
@mvashishtha mvashishtha dismissed stale reviews from pyrito and vnlitvinov via bc3c46d August 8, 2022 19:40
modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
mvashishtha and others added 10 commits August 10, 2022 04:22
…ize limit.

Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Co-authored-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
Signed-off-by: mvashishtha <mahesh@ponder.io>
pyrito
pyrito previously approved these changes Aug 10, 2022
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.

Left a couple of comments. Otherwise, LGTM.

@@ -34,6 +35,8 @@
)
from modin.error_message import ErrorMessage

_OBJECT_STORE_TO_SYSTEM_MEMORY_RATIO = 0.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this ratio equal to 0.6?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an arbitrary number that has been around for a long time. It goes back to 3542226 from 2019/01/11.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just wondering why we specified it equal to this number, i.e. is there any heuristic. If someone is aware of that, it would be good to know.

modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
modin/core/execution/ray/common/utils.py Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Igoshev <Poolliver868@mail.ru>
@YarShev YarShev merged commit 6e1849f into modin-project:master Aug 12, 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.

PERF: Stop overriding Ray's 2 GiB object store size limit on mac
5 participants