-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-26014][R] Deprecate R prior to version 3.4 in SparkR #23012
Conversation
Tests probably will fail since it produces warnings now. Once we upgrade R to 3.4, it wouldn't fail. cc @felixcheung. @shaneknapp, @viirya, @shivaram, @falaki, @mengxr, @yanboliang FYI. This PR is made per http://apache-spark-developers-list.1001551.n3.nabble.com/discuss-SparkR-CRAN-feasibility-check-server-problem-td25605.html |
adding @srowen too. |
Test build #98718 has finished for PR 23012 at commit
|
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'll put a note in the Docs Text field in the JIRA for release notes.
I think this should say unsupported (ie could still work) instead of deprecated
Also the compareVersion should check both major and minor ie 3.4.0
|
Also I think the warning should be in .First in general.R
|
Yea will take a look to address. But about documenting unsupported, if we explicitly are going to say it's unsupported and dropped, for instance, we should remove the compatibility change (https://github.com/apache/spark/blob/master/R/pkg/src-native/string_hash_code.c) and I assume previous versions don't work. Deprecation step might be more concervative and consistent with dropping steps of other language APIs. |
Test build #98736 has finished for PR 23012 at commit
|
FYI
This is unused code I’m going to remove it
https://github.com/apache/spark/blob/master/R/pkg/src-native/string_hash_code.c
|
Nice. Thanks! - I just saw SPARKR-7839. BTW Felix, are you maybe worrying about that we happen to upgrade R version in Jenkins to 3.4 and .. we could break lower deprecated R version support in Spark 3.0 I guess? If so, let me put the version check into both places |
In this way, we could postpone R upgrade after Spark 3.0.0 release in Jenkins, and could still test the deprecated R version 3.1. |
I think it's easier to say unsupported if we are not testing it in jenkins or appveyer. I don't know if we any coverage at release for older R version anyway, so it's better to unsupported then deprecate. but agree maybe the way to do this is deprecate without updating R in jenkins |
howdy howdy! unless we dockerize spark builds (someday!), we're going to be stuck w/testing against one version of R on the jenkins workers... i've been looking in to packrat to help manage packages, but having more than one version of R will require me manually building and disting it out. and i really, truly, don't want to do that. let me know how you think i should proceed. |
Hey shane I don’t think we are saying to test multiple R version at all. In fact quite the opposite, just the new(er) version at some point in the future.
(We don’t have a better solution for packages though. There’s another PR for R arrow package for example)
|
@shaneknapp, do you roughly know how difficult it is (and do you have some time shortly) to upgrade R from 3.1 to 3.4? I am asking this because I had some difficulties when I tried to manually upgrade from a certain low version to another non-latest version. If it's expected to take a while, let's go deprecation step. Does this sound okay to you @felixcheung? |
TL;DR: let's go w/deprecation. still TL;DR: if i never have to install or manage R again, i will be a happy person! @HyukjinKwon upgrading R is easy. getting the right mix of R and all of the associated packages working "as expected" is a nightmare. the biggest problem i foresee is if we upgrade R (and all other packages) on the workers, every version of spark will be tested against this... and there will be bugs, test failures, and other time consuming (and obtuse) problems to debug. multiply this by every branch, and you can see the rabbit hole you've just entered. for example, a month ago when i finally had time to dive back in to the ubuntu port, after finally figuring out how to install R+friends on ubuntu in an identical way to the centos workers, i STILL was finding problems w/lintr (see: #22896). anyways: i'm more than happy to upgrade R and all the packages to something much more recent, but i will definitely appreciate some help in the game of test-failure whack-a-mole. |
Ah .. right makes sense to me. Thanks @shaneknapp. +1 |
Yea there are some problem with some packages we depend on that are not installable from CRAN (eg too old) so it will be hard to a new version of R and new installation.
So to clarify, depreciation as is we still test on R 3.1?
|
yes: deprecation in this case means we test against R-3.1.1 |
Yup, will address the other comments and update the PR accordingly. |
Test build #98848 has finished for PR 23012 at commit
|
Merged to master. Thanks @felixcheung. |
## What changes were proposed in this pull request? This PR proposes to bump up the minimum versions of R from 3.1 to 3.4. R version. 3.1.x is too old. It's released 4.5 years ago. R 3.4.0 is released 1.5 years ago. Considering the timing for Spark 3.0, deprecating lower versions, bumping up R to 3.4 might be reasonable option. It should be good to deprecate and drop < R 3.4 support. ## How was this patch tested? Jenkins tests. Closes apache#23012 from HyukjinKwon/SPARK-26014. Authored-by: hyukjinkwon <gurwls223@apache.org> Signed-off-by: hyukjinkwon <gurwls223@apache.org>
What changes were proposed in this pull request?
This PR proposes to bump up the minimum versions of R from 3.1 to 3.4.
R version. 3.1.x is too old. It's released 4.5 years ago. R 3.4.0 is released 1.5 years ago. Considering the timing for Spark 3.0, deprecating lower versions, bumping up R to 3.4 might be reasonable option.
It should be good to deprecate and drop < R 3.4 support.
How was this patch tested?
Jenkins tests.