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.

Sign up for GitHub

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

Jump to bottom

FEAT-#5394: Reduce amount of remote calls for Map operator #7136

Merged
merged 20 commits into from
May 3, 2024
Merged

FEAT-#5394: Reduce amount of remote calls for Map operator #7136

merged 20 commits into from
May 3, 2024

Conversation

Retribution98
Copy link
Collaborator

@Retribution98 Retribution98 commented Mar 28, 2024 •
edited
Loading

What do these changes do?

This PR includes an implementation of the simple method proposed in the task:
Check for partitioning before every Map call and if there're too many partitions then call the function across row/column axis so the number of remote calls would equal to the number of row/column partitions (fewer than the total amount of partitions).

But this way got slow when the Dataframe has few columnar partitions and many row partitions (much more than the Cpu count), otherwise modin would only use one remote task.
To solve this problem, another strategy was implemented. If we use columnar partitions to reduce the number of remote tasks, we can try to split them to fill all processors. If possible, we use a new implementation, otherwise the simple method.
perf_5394

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • 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 Reduce amount of remote calls for square-like dataframes #5394
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

@Retribution98 Retribution98 marked this pull request as draft March 28, 2024 13:21
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 4, 2024
View reviewed changes
check_perfomance.py Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 4, 2024
View reviewed changes
check_perfomance.py Fixed Show fixed Hide fixed
@YarShev
Copy link
Collaborator

YarShev commented Apr 10, 2024

Is this PR ready for review?

@Retribution98 Retribution98 marked this pull request as ready for review April 10, 2024 11:25
@Retribution98 Retribution98 force-pushed the feat_5394 branch 2 times, most recently from 3c2f61e to 8917466 Compare April 11, 2024 08:43
YarShev
YarShev reviewed Apr 16, 2024
View reviewed changes
modin/core/dataframe/pandas/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/dataframe/pandas/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/dataframe/pandas/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/dataframe/pandas/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/dataframe/pandas/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/dataframe/pandas/partitioning/partition_manager.py Outdated Show resolved Hide resolved
@Retribution98 Retribution98 changed the base branch from master to main April 29, 2024 12:29
@anmyachev
Copy link
Collaborator

@Retribution98 I see your graphs above, but I don’t really understand what the axes mean. Please label them.

@Retribution98
Copy link
Collaborator Author

Retribution98 commented May 2, 2024 •
edited
Loading

@Retribution98 I see your graphs above, but I don’t really understand what the axes mean. Please label them.

@anmyachev Thanks, updated it.

anmyachev
anmyachev previously approved these changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@anmyachev anmyachev left a comment

Choose a reason for hiding this comment

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

LGTM!

modin/tests/core/storage_formats/pandas/test_internals.py
nrows = MinPartitionSize.get() * CpuCount.get() * 2
data = {f"col{i}": np.ones(nrows) for i in range(ncols)}
df = pd.DataFrame(data)
partitions = df._query_compiler._modin_frame._partitions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of partition_manager_class?

Suggested change
partitions = df._query_compiler._modin_frame._partitions
partitions = df._query_compiler._modin_frame._partitions
partition_mgr_cls = df._query_compiler._modin_frame._partition_mgr_cls

modin/tests/core/storage_formats/pandas/test_internals.py Outdated


def test_map_partitions_joined_by_column():
# Set the config to 'True' inside of the context-manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it mean?

modin/core/dataframe/pandas/partitioning/partition_manager.py Outdated
kw = {
"num_splits": step,
}
result = np.empty(partitions.shape, dtype=cls._partition_class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are equivalent actions, but let's make it more explicit.

Suggested change
result = np.empty(partitions.shape, dtype=cls._partition_class)
result = np.empty(partitions.shape, dtype=object)

@anmyachev
Copy link
Collaborator

@YarShev any more comments?

YarShev
YarShev approved these changes May 3, 2024
View reviewed changes
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.

@Retribution98, LGTM, thanks!

@YarShev YarShev merged commit f8bf5b4 into modin-project:main May 3, 2024
38 checks passed
@YarShev YarShev mentioned this pull request May 3, 2024
Closed
@Retribution98 Retribution98 mentioned this pull request May 8, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunjose696 arunjose696 arunjose696 left review comments

@YarShev YarShev YarShev approved these changes

@anmyachev anmyachev anmyachev approved these changes

@devin-petersohn devin-petersohn Awaiting requested review from devin-petersohn devin-petersohn is a code owner

@mvashishtha mvashishtha Awaiting requested review from mvashishtha mvashishtha is a code owner

@RehanSD RehanSD Awaiting requested review from RehanSD RehanSD is a code owner

@vnlitvinov vnlitvinov Awaiting requested review from vnlitvinov vnlitvinov is a code owner

@dchigarev dchigarev Awaiting requested review from dchigarev dchigarev is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce amount of remote calls for square-like dataframes
5 participants
@Retribution98 @YarShev @anmyachev @arunjose696 @dchigarev