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

bug fix: DLModel prediction #2194

Merged

Conversation

sperlingxx
Copy link
Contributor

@sperlingxx sperlingxx commented Jan 16, 2018

What changes were proposed in this pull request?

  1. Make sure DLModel.train=False when predicting in pipeline API
  2. Broadcast transformer(SampleToMiniBatch) rather than initializing them on executors, which will cause exception in spark-cluster mode because of calling Engine.nodeNumber() on executors.

How was this patch tested?

manual test

Make sure DLModel.train=False when predicting in pipeline API
@qiuxin2012 qiuxin2012 requested a review from hhbyyh January 17, 2018 08:18
Copy link

@hhbyyh hhbyyh left a comment

Choose a reason for hiding this comment

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

Thanks for finding the issue. This is a regression issue.

@@ -391,7 +391,8 @@ class DLModel[@specialized(Float, Double) T: ClassTag](
val localBatchSize = $(batchSize)

val resultRDD = dataFrame.rdd.mapPartitions { rowIter =>
val localModel = modelBroadCast.value()
// call the evaluate method to enable DLModel.train=False during the predict process
val localModel = modelBroadCast.value().evaluate()
Copy link

Choose a reason for hiding this comment

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

Is it better if we move evaluate before model broadcasting?

.setBatchSize(nRecords)
.setOptimMethod(new LBFGS[Float]())
.setLearningRate(0.1)
.setMaxEpoch(maxEpoch)
Copy link

Choose a reason for hiding this comment

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

let's set this to 1.

@@ -91,6 +91,27 @@ class DLEstimatorSpec extends FlatSpec with Matchers with BeforeAndAfter {
assert(correct > nRecords * 0.8)
}

"An DLEstimator" should "throws exception when DLModel is predicting with DLModel.train=True" in {
Copy link

@hhbyyh hhbyyh Jan 18, 2018

Choose a reason for hiding this comment

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

Shall we use intercept exception in this unit test? The content and the title does not quite match.

@sperlingxx
Copy link
Contributor Author

Hi @hhbyyh,
Modifications are made about this PR.

@hhbyyh
Copy link

hhbyyh commented Jan 31, 2018

Thanks for the update.
Also an unit test is still nice to have. I can add that in a follow-up PR.

@qiuxin2012
Copy link
Contributor

jenkins passed

@qiuxin2012 qiuxin2012 merged commit 43f023f into intel-analytics:master Feb 7, 2018
@sperlingxx sperlingxx deleted the bugfix_DLModel_prediction branch February 7, 2018 09:31
yiheng pushed a commit to yiheng/BigDL that referenced this pull request Feb 13, 2018
* bug fix: DLModel prediction (#4)

Make sure DLModel.train=False when predicting in pipeline API

* 1. broadcast transformer in DLModel.transform ; 2. remove useless ut
Le-Zheng pushed a commit to Le-Zheng/BigDL that referenced this pull request Oct 20, 2021
dding3 pushed a commit to dding3/BigDL that referenced this pull request Nov 17, 2021
dding3 pushed a commit to dding3/BigDL that referenced this pull request Nov 17, 2021
dding3 pushed a commit to dding3/BigDL that referenced this pull request Nov 17, 2021
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.

4 participants