-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-24152][R][TESTS] Disable check-cran from run-tests.sh #26375
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
|
@shivaram , @felixcheung , @HyukjinKwon , @viirya , @srowen . |
|
Test build #113149 has finished for PR 26375 at commit
|
|
Do we know what the reason for the failure is? Also what is the work around in terms of detecting CRAN problems in a timely fashion? Can we say run this just for PRs that touch R code? |
|
|
||
| NO_TESTS=1 NO_MANUAL=1 $FWDIR/check-cran.sh 2>&1 | tee -a $CRAN_CHECK_LOG_FILE | ||
| FAILED=$((PIPESTATUS[0]||$FAILED)) | ||
| # SPARK-24152 We will add this test back as a separate Jenkins job |
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.
Do we plan to upgrade R version? I remember that a higher R version can avoid this kind of failure. cc @HyukjinKwon
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.
I guess the higher R version is going to hit this issue again after one year, but that sounds a good solution for now. How do you think about that, @shivaram and @felixcheung ?
|
Test build #113150 has finished for PR 26375 at commit
|
|
Thanks, @shivaram .
It's always happening their side changes. We don't know why this fails suddenly. As you see on SPARK-24152. This happens frequently in this year time to time . And, everytime, @viirya or @felixcheung contacted CRAN maintainers .
We can have separate jobs for
That also can reduce this kind of massive outage. But, it's still be a blocker for all R PRs. CRAN check is only required only a few time when we do the release. |
|
I'm okay for both (1) invoking CRAN check for R PR only and (2) upgrading Jenkins R and raised the minimum R versions. However, let's unblock the PRBuilder first. Both (1) and (2) also needs some time to develop and validate. |
|
I'm not sure running it just on releases is enough as that makes it hard to go back and figure out what change broke the CRAN test. So running it on R PRs seems necessary to me. I'm fine with unblocking the build for now if you can take care of opening a new JIRA / PR to track (1) and (2) above |
|
It sounds fine to me as this actually blocks all PR dev. And because of weekend, CRAN sysadmins might not response quickly. |
|
Thank you all. Sure! |
|
I created SPARK-29732 first. For the minimum version of R, could you raise an email to dev? If there is the best person to set it, I believe it is you, @shivaram , in the community. :) |
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.
As a kind of HOTFIX, I'm merging this PR to unblock all the other PRs. Thank you for the swift reviews! Merged to master/2.4.
### What changes were proposed in this pull request? This PR aims to remove `check-cran` from `run-tests.sh`. We had better add an independent Jenkins job to run `check-cran`. ### Why are the changes needed? CRAN instability has been a blocker for our daily dev process. The following simple check causes consecutive failures in 4 of 9 Jenkins jobs + PR builder. ``` * checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : dims [product 24] do not match the length of object [0] ``` - spark-branch-2.4-test-sbt-hadoop-2.6 - spark-branch-2.4-test-sbt-hadoop-2.7 - spark-master-test-sbt-hadoop-2.7 - spark-master-test-sbt-hadoop-3.2 - PRBuilder ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Currently, PR builder is failing due to the above issue. This PR should pass the Jenkins. Closes #26375 from dongjoon-hyun/SPARK-24152. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 91d9901) Signed-off-by: Dongjoon Hyun <[email protected]>
|
Hm, once we drop R 3.4 (deprecated from 3.0), we can deal with this issue permanently. I think we dont have to disable this but upgrade R version in Jenkins to 3.4. |
|
+1 on what Hyukjin said instead of disabling tests
…________________________________
From: Hyukjin Kwon <[email protected]>
Sent: Sunday, November 3, 2019 1:37:53 AM
To: apache/spark <[email protected]>
Cc: Felix Cheung <[email protected]>; Mention <[email protected]>
Subject: Re: [apache/spark] [SPARK-24152][R][TESTS] Disable check-cran from run-tests.sh (#26375)
@shaneknapp<https://github.com/shaneknapp>, can we upgrade R version from 3.1 to 3.4 in Jenkins (since we deprecated it at #23012<#23012>)? I think we should re-enable it as soon as possible. The specific CRAN failure will be permanently fixed
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#26375?email_source=notifications&email_token=ACENZ67WU3Y7EMG4TFNYNXTQR2EWDA5CNFSM4JIJORXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC5NO5I#issuecomment-549115765>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACENZ67RGB3MMTDFLGXV3C3QR2EWDANCNFSM4JIJORXA>.
|
|
Also CRAN check is pretty important for R package summision. It also checks misusages like |
|
For instance, see the CRAN check failures at R vectorization - #23760. https://github.com/apache/spark/pull/23760/files#diff-508641a8bd6c6b59f3e77c80cdcfa6a9R1221 Unlike pip packaging test, CRAN includes a set of linter-ish rules related to packages. It also verifies documentation as well. |
|
@shaneknapp, can we upgrade R version from 3.1 to 3.4 in Jenkins (since we deprecated it at #23012)? I think we should re-enable it as soon as possible. The specific CRAN failure will be permanently fixed |
|
I think it's OK to disable temporarily to fix the PR builders. If updating R fixes it, then it can come back. Is this step important for PR builders vs the release process? only the release process pushes to CRAN. Would we not find a lot of errors until release time if we did not? |
|
Yeah, it has a lint-like feature unlike pip packaging test. I myself faced a lot when I worked on R vectoriation with Arrow optimization. |
|
Maybe we can upgrade and bring it back quick instead of reverting .. ? I dont know how hard it is to upgrade R to 3.4 and how much it takes in Jenkins cc @shaneknapp |
|
Of course, we also can revert this if everything goes green. I'm +1 for all efforts if that is able to protect our PR builder from CRAN instability. |
|
Looks like it was fixed at CRAN side. |
|
Thank you for checking. Then, if you want, you can make a PR for reverting this~ @viirya . |
|
Thanks all! |
What changes were proposed in this pull request?
This PR aims to remove
check-cranfromrun-tests.sh.We had better add an independent Jenkins job to run
check-cran.Why are the changes needed?
CRAN instability has been a blocker for our daily dev process.
The following simple check causes consecutive failures in 4 of 9 Jenkins
jobs + PR builder.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Currently, PR builder is failing due to the above issue. This PR should pass the Jenkins.