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

@lepennec lepennec commented Oct 12, 2017

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 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 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

@lepennec lepennec commented Oct 13, 2017

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

@brnleehng
Copy link

@brnleehng brnleehng commented Oct 19, 2017

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

@lepennec lepennec commented Oct 19, 2017

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

@brnleehng brnleehng commented Oct 19, 2017

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 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
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 87c1ac5...8a1fc3e
Details
codecov/project 16.97% remains the same compared to 87c1ac5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@topepo
Copy link
Owner

@topepo 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

@lepennec lepennec commented Nov 12, 2017

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.