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.

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

Add missing exports required for some foreach backend #748

Merged
merged 2 commits into from
Oct 24, 2017

Conversation

lepennec
Copy link
Contributor

I've stumbled upon a bug for some foreach backend (doFuture with multisession for instance...). There are some missing exports in workflows.R. It seems that completing the list as proposed below fix this issue.

@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #748 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #748   +/-   ##
=======================================
  Coverage   16.97%   16.97%           
=======================================
  Files          90       90           
  Lines       13187    13187           
=======================================
  Hits         2238     2238           
  Misses      10949    10949

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87c1ac5...8a1fc3e. Read the comment docs.

@freezby
Copy link

freezby commented Oct 13, 2017

I had the same problem using doMPI on a cluster of two nodes (localhost and one remote computer)
But to get things working I had to complete the export variable in a different way :
export <- c("optimism_xy", "requireNamespaceQuietStop","subset_x","pred_failed",
"model_failed", "fail_warning", "fill_failed_pred", "fill_failed_prob",
"trim_values", "outcome_conversion", "flatTable", "ppOpts", "createModel",
"predictionFunction","probFunction"
)

@lepennec
Copy link
Contributor Author

OK. It looks like my list should be enlarged...

@brnleehng
Copy link

I had a similar issue with doAzureParallel, it seems that all of the internal functions will need to be
in the export list since it depends on what caret function you are using

#743

@lepennec
Copy link
Contributor Author

Experimenting with foreach, it seems that forcing to load caret with the .packages option helps. Can you help me checking this is indeed the case by sending a reproducible example or testing the proposed modification?

@brnleehng
Copy link

I ran one of our samples codes. Installed your branch, It works for me.

library(DAAG)
library(caret)

set.seed(998)
inTraining <- createDataPartition(spam7$yesno, p = .75, list = FALSE)
training <- spam7[ inTraining,]
testing <- spam7[-inTraining,]

fitControl <- trainControl(## 10-fold cross validation
method = "repeatedcv",
number = 10,
## repeat 10 times
repeats = 10,
classProbs = TRUE,
summaryFunction = twoClassSummary,
search = "random",
## run on the parallel backend
allowParallel = TRUE)

rf_fit <- train(
yesno ~ .,
data = training,
method = "rf",
metric = "ROC",
tuneLength = 30,
trControl = fitControl)

rf_fit

@topepo
Copy link
Owner

topepo commented Oct 24, 2017

Thanks for fixing this. I run a bunch of tests using doMC. I'll add some more for doParallel. The issue is somewhat OS dependent (and cluster type dependent) too.

@topepo topepo merged commit bdeb658 into topepo:master Oct 24, 2017
@topepo
Copy link
Owner

topepo commented Nov 12, 2017

@brnleehng have you tested these updates on doAzureParallel? I'm getting a release together and it would be good to have confirmation.

@lepennec Anything else that you think needs to happen?

@lepennec
Copy link
Contributor Author

I expect that if we cover sufficient different backends, which seems to be the case, we should not have more surprise...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants