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

Extended configuration of OOM injection mode #10013

Merged
merged 13 commits into from
Dec 15, 2023
Merged

Extended configuration of OOM injection mode #10013

merged 13 commits into from
Dec 15, 2023

Conversation

gerashegalov
Copy link
Collaborator

@gerashegalov gerashegalov commented Dec 11, 2023 •
edited
Loading

This PR enables specifying OOM injection mode with additional config

such as

  • num_ooms to inject
  • skip=N to make triger first OOM on N+1st allocation
  • type with the current default mix CPU_OR_GPU, CPU (just host allocations), GPU (just device allocations)
  • split=bool whether to inject SplitAndRetryOOM

Enables IT invocations such as :

PYSP_TEST_spark_rapids_memory_gpu_state_debug=STDERR \
TEST_PARALLEL=0 \
SPARK_HOME=~/dist/spark-3.3.0-bin-hadoop3 \
./integration_tests/run_pyspark_from_build.sh \
  -k array_exists --test_oom_injection_mode=always:type=CPU,num_ooms=1,skip=4,split=true

This PR requires is stacked on NVIDIA/spark-rapids-jni#1637

@gerashegalov
wip
f7023b3 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
marker
e61c758 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov gerashegalov added the test Only impacts tests label Dec 11, 2023
@gerashegalov gerashegalov self-assigned this Dec 11, 2023
revans2
revans2 reviewed Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

Generally it looks good.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala Outdated Show resolved Hide resolved
@gerashegalov
checkpoint
518bfd2 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
undo irrelevant rapids conf changes
58f9c89 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
moved config parsing from RmmSpark
087fb93 
reviews

Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
added split conf
69abd88 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
minor cleanup
294af26 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
two lines to delimet func
9cf4a9e 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov @jbrennan333
Apply suggestions from code review
d2a772d 
Co-authored-by: Jim Brennan <jimb@nvidia.com>
@gerashegalov gerashegalov marked this pull request as ready for review December 14, 2023 18:29
@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov
review
232bbe7 
Signed-off-by: Gera Shegalov <gera@apache.org>
jbrennan333
jbrennan333 reviewed Dec 14, 2023
View reviewed changes
Copy link
Collaborator

@jbrennan333 jbrennan333 left a comment

Choose a reason for hiding this comment

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

Looking good - just a few minor nits, and it looks like prebuild checks are failing.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala Show resolved Hide resolved
@gerashegalov
Scalastyle
6beca4a 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
typo
ce52d36 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov
review: split in testRetryOOMInjectionMode comment
1ea9e4c 
Signed-off-by: Gera Shegalov <gera@apache.org>
@gerashegalov
Copy link
Collaborator Author

@jbrennan333 sorry missed one of your comments. PTAL again.

@jbrennan333
Copy link
Collaborator

build

1 similar comment
@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov gerashegalov mentioned this pull request Dec 15, 2023
Open
@gerashegalov gerashegalov merged commit ea43d89 into NVIDIA:branch-24.02 Dec 15, 2023
37 of 38 checks passed
@gerashegalov gerashegalov deleted the gerashegalov/issue8886 branch December 15, 2023 15:19
@gerashegalov gerashegalov mentioned this pull request Dec 15, 2023
Open
@jbrennan333 jbrennan333 mentioned this pull request Dec 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrennan333 jbrennan333 jbrennan333 approved these changes

@abellina abellina Awaiting requested review from abellina

@revans2 revans2 Awaiting requested review from revans2

Assignees

@gerashegalov gerashegalov

Labels
test Only impacts tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gerashegalov @jbrennan333 @revans2