-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-31780][K8S][TESTS] Add R test tag to exclude R K8s image building and test #28594
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Kubernetes integration test starting |
|
Hi, @dbtsai . |
|
Kubernetes integration test status success |
|
LGTM. |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much, @dbtsai .
Merged to master/3.0.
…ing and test ### What changes were proposed in this pull request? This PR aims to skip R image building and one R test during integration tests by using `--exclude-tags r`. ### Why are the changes needed? We have only one R integration test case, `Run SparkR on simple dataframe.R example`, for submission test coverage. Since this is rarely changed, we can skip this and save the efforts required for building the whole R image and running the single test. ``` KubernetesSuite: ... - Run SparkR on simple dataframe.R example Run completed in 10 minutes, 20 seconds. Total number of tests run: 20 ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the K8S integration test and do the following manually. (Note that R test is skipped) ``` $ resource-managers/kubernetes/integration-tests/dev/dev-run-integration-tests.sh --deploy-mode docker-for-desktop --exclude-tags r --spark-tgz $PWD/spark-*.tgz ... KubernetesSuite: - Run SparkPi with no resources - Run SparkPi with a very long application name. - Use SparkLauncher.NO_RESOURCE - Run SparkPi with a master URL without a scheme. - Run SparkPi with an argument. - Run SparkPi with custom labels, annotations, and environment variables. - All pods have the same service account by default - Run extraJVMOptions check on driver - Run SparkRemoteFileTest using a remote data file - Run SparkPi with env and mount secrets. - Run PySpark on simple pi.py example - Run PySpark with Python2 to test a pyfiles example - Run PySpark with Python3 to test a pyfiles example - Run PySpark with memory customization - Run in client mode. - Start pod creation from template - PVs with local storage - Launcher client dependencies - Test basic decommissioning Run completed in 10 minutes, 23 seconds. Total number of tests run: 19 Suites: completed 2, aborted 0 Tests: succeeded 19, failed 0, canceled 0, ignored 0, pending 0 All tests passed. ``` Closes #28594 from dongjoon-hyun/SPARK-31780. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit a06768e) Signed-off-by: Dongjoon Hyun <[email protected]>
|
Test build #122907 has finished for PR 28594 at commit
|
|
Test build #122908 has finished for PR 28594 at commit
|
|
Is there a reason we are skipping this test? We are doing: in Jenkins, so we are already building the distribution with R support. What do we gain from skipping this test? I would have preferred to keep this test tbh, (so as to have greater coverage). |
i agree... more testing is better imo, especially since we have all the framework in place. |
|
Hi, @ifilonenko and @shaneknapp . |
|
By default (=if you don't use |
|
Sure, I totally see the need for an |
the specific bits to look at are towards the end of the build... right above
@dongjoon-hyun this is the exact command used by the SparkPullRequestBuilder-K8s job to launch the integration tests. we are not using |
|
@ifilonenko and @shaneknapp . Are you sure that is caused by this PR?
IIRC, at that time, this PR is tested correctly and I verified that R test are executed there. I guess another PR after this commit may cause the missing R testing. |
|
Anyway, since I made this option, let me take a look what is going on there~
|
|
BTW, I also have a personal Jenkins K8s machine on |
|
Also, please see the following commit log one week ago. It has |
|
I found another evidence from our Apache Spark AmpLab Jenkins farm log. The failed test case is
|
|
Since this PR was merged on May 20 and I provided multiple recent evidences of the R testing like the above, @ifilonenko 's claim is wrong.
I'll stop my investigation here. Please take a look at more recent commits to solve your problems. Thanks. |
I was sure you did :) but it seemed that this was the only code path that touched the R tests, recently and the inclusion of the RTestTag() threw me off. Thanks so much for the thorough investigation! I’ll review elsewhere, thanks! |
|
Thanks. I understand, @ifilonenko . :) |
|
@dongjoon-hyun thanks for the breakdown! |

What changes were proposed in this pull request?
This PR aims to skip R image building and one R test during integration tests by using
--exclude-tags r.Why are the changes needed?
We have only one R integration test case,
Run SparkR on simple dataframe.R example, for submission test coverage. Since this is rarely changed, we can skip this and save the efforts required for building the whole R image and running the single test.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the K8S integration test and do the following manually. (Note that R test is skipped)