Skip to content

[SPARK-31918][R] Ignore S4 generic methods under SparkR namespace in closure cleaning to support R 4.0.0+ #28907

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

Closed
HyukjinKwon wants to merge 4 commits into from
Closed

[SPARK-31918][R] Ignore S4 generic methods under SparkR namespace in closure cleaning to support R 4.0.0+ #28907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe0deac
Ignore S4 generic methods in closure cleaning to support R 4.0.0+
HyukjinKwon Jun 23, 2020
405b964
Address a comment
HyukjinKwon Jun 24, 2020
29bfcdb
style
HyukjinKwon Jun 24, 2020
14886cb
Lint version differences
HyukjinKwon Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion R/pkg/R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,10 @@ processClosure <- function(node, oldEnv, defVars, checkedFuncs, newEnv) {
# Namespaces other than "SparkR" will not be searched.
if (!isNamespace(func.env) ||
(getNamespaceName(func.env) == "SparkR" &&
!(nodeChar %in% getNamespaceExports("SparkR")))) {
!(nodeChar %in% getNamespaceExports("SparkR")) &&
# Note that generic S4 methods should not be set to the environment of
# cleaned closure. It does not work with R 4.0.0+. See also SPARK-31918.
nodeChar != "" && !methods::isGeneric(nodeChar, func.env))) {
Copy link
Member Author

@HyukjinKwon HyukjinKwon Jun 23, 2020

Choose a reason for hiding this comment

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

nodeChar can be empty strings, and isGeneric rejects empty strings.

Copy link
Contributor

@shivaram shivaram Jun 23, 2020

Choose a reason for hiding this comment

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

Just to confirm this will only exclude generics inside SparkR -- is that right?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Jun 24, 2020

Choose a reason for hiding this comment

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

Yes .. so it wouldn't affect and/or protect other cases.

# Only include SparkR internals.

# Set parameter 'inherits' to FALSE since we do not need to search in
Expand Down
4 changes: 3 additions & 1 deletion R/pkg/tests/fulltests/test_context.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ test_that("Check masked functions", {
"colnames", "colnames<-", "intersect", "rank", "rbind", "sample", "subset",
"summary", "transform", "drop", "window", "as.data.frame", "union", "not")
version <- packageVersion("base")
if (as.numeric(version$major) >= 3 && as.numeric(version$minor) >= 3) {
is33Above <- as.numeric(version$major) >= 3 && as.numeric(version$minor) >= 3
is40Above <- as.numeric(version$major) >= 4
if (is33Above || is40Above) {
namesOfMasked <- c("endsWith", "startsWith", namesOfMasked)
}
masked <- conflicts(detail = TRUE)$`package:SparkR`
Expand Down
18 changes: 9 additions & 9 deletions R/pkg/tests/fulltests/test_mllib_classification.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ test_that("spark.svmLinear", {
summary <- summary(model)

# test summary coefficients return matrix type
expect_true(class(summary$coefficients) == "matrix")
expect_true(any(class(summary$coefficients) == "matrix"))
expect_true(class(summary$coefficients[, 1]) == "numeric")

coefs <- summary$coefficients[, "Estimate"]
Expand Down Expand Up @@ -130,7 +130,7 @@ test_that("spark.logit", {
summary <- summary(model)

# test summary coefficients return matrix type
expect_true(class(summary$coefficients) == "matrix")
expect_true(any(class(summary$coefficients) == "matrix"))
Copy link
Member Author

@HyukjinKwon HyukjinKwon Jun 23, 2020

Choose a reason for hiding this comment

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

matrix objects now also inherit from class "array", so e.g., class(diag(1)) is c("matrix", "array"). This invalidates code incorrectly assuming that class(matrix_obj)) has length one.

https://cran.r-project.org/doc/manuals/r-devel/NEWS.html

Copy link
Contributor

@shivaram shivaram Jun 23, 2020

Choose a reason for hiding this comment

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

Thanks! This also reminds me that it'll be great to test on r-devel (which I guess is 4.0.3 right now)

expect_true(class(summary$coefficients[, 1]) == "numeric")

versicolorCoefsR <- c(1.52, 0.03, -0.53, 0.04, 0.00)
Expand Down Expand Up @@ -242,8 +242,8 @@ test_that("spark.logit", {
# Test binomial logistic regression against two classes with upperBoundsOnCoefficients
# and upperBoundsOnIntercepts
u <- matrix(c(1.0, 0.0, 1.0, 0.0), nrow = 1, ncol = 4)
model <- spark.logit(training, Species ~ ., upperBoundsOnCoefficients = u,
upperBoundsOnIntercepts = 1.0)
model <- suppressWarnings(spark.logit(training, Species ~ ., upperBoundsOnCoefficients = u,
upperBoundsOnIntercepts = 1.0))
summary <- summary(model)
coefsR <- c(-11.13331, 1.00000, 0.00000, 1.00000, 0.00000)
coefs <- summary$coefficients[, "Estimate"]
Expand All @@ -255,8 +255,8 @@ test_that("spark.logit", {
# Test binomial logistic regression against two classes with lowerBoundsOnCoefficients
# and lowerBoundsOnIntercepts
l <- matrix(c(0.0, -1.0, 0.0, -1.0), nrow = 1, ncol = 4)
model <- spark.logit(training, Species ~ ., lowerBoundsOnCoefficients = l,
lowerBoundsOnIntercepts = 0.0)
model <- suppressWarnings(spark.logit(training, Species ~ ., lowerBoundsOnCoefficients = l,
lowerBoundsOnIntercepts = 0.0))
summary <- summary(model)
coefsR <- c(0, 0, -1, 0, 1.902192)
coefs <- summary$coefficients[, "Estimate"]
Expand All @@ -268,9 +268,9 @@ test_that("spark.logit", {
# Test multinomial logistic regression with lowerBoundsOnCoefficients
# and lowerBoundsOnIntercepts
l <- matrix(c(0.0, -1.0, 0.0, -1.0, 0.0, -1.0, 0.0, -1.0), nrow = 2, ncol = 4)
model <- spark.logit(training, Species ~ ., family = "multinomial",
lowerBoundsOnCoefficients = l,
lowerBoundsOnIntercepts = as.array(c(0.0, 0.0)))
model <- suppressWarnings(spark.logit(training, Species ~ ., family = "multinomial",
Copy link
Member Author

@HyukjinKwon HyukjinKwon Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It suppresses:

test_mllib_classification.R:258: error: spark.logit
(converted from warning) the condition has length > 1 and only the first element will be used
Backtrace:
 1. SparkR::spark.logit(...) tests/fulltests/test_mllib_classification.R:258:2
 2. SparkR::spark.logit(...)

lowerBoundsOnCoefficients = l,
lowerBoundsOnIntercepts = as.array(c(0.0, 0.0))))
summary <- summary(model)
versicolorCoefsR <- c(42.639465, 7.258104, 14.330814, 16.298243, 11.716429)
virginicaCoefsR <- c(0.0002970796, 4.79274, 7.65047, 25.72793, 30.0021)
Expand Down
2 changes: 1 addition & 1 deletion R/pkg/tests/fulltests/test_mllib_clustering.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ test_that("spark.kmeans", {
expect_equal(sort(collect(distinct(select(cluster, "prediction")))$prediction), c(0, 1))

# test summary coefficients return matrix type
expect_true(class(summary.model$coefficients) == "matrix")
expect_true(any(class(summary.model$coefficients) == "matrix"))
expect_true(class(summary.model$coefficients[1, ]) == "numeric")

# Test model save/load
Expand Down
2 changes: 1 addition & 1 deletion R/pkg/tests/fulltests/test_mllib_regression.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ test_that("spark.glm summary", {
rStats <- summary(glm(Sepal.Width ~ Sepal.Length + Species, data = dataset))

# test summary coefficients return matrix type
expect_true(class(stats$coefficients) == "matrix")
expect_true(any(class(stats$coefficients) == "matrix"))
expect_true(class(stats$coefficients[, 1]) == "numeric")

coefs <- stats$coefficients
Expand Down