-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-19357][ML] Adding parallel model evaluation in ML tuning #16774
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
Changes from all commits
5650e98
36a1a68
b051afa
46fe252
1274ba4
80ac2fd
8126710
6a9b735
1c2e391
9e055cd
97ad7b4
5e8a086
864c99c
ad8a870
911af1d
658aacb
2c73b0b
7a8221b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.ml.param.shared | ||
|
|
||
| import scala.concurrent.ExecutionContext | ||
|
|
||
| import org.apache.spark.ml.param.{IntParam, Params, ParamValidators} | ||
| import org.apache.spark.util.ThreadUtils | ||
|
|
||
| /** | ||
| * Trait to define a level of parallelism for algorithms that are able to use | ||
| * multithreaded execution, and provide a thread-pool based execution context. | ||
| */ | ||
| private[ml] trait HasParallelism extends Params { | ||
|
|
||
| /** | ||
| * The number of threads to use when running parallel algorithms. | ||
| * Default is 1 for serial execution | ||
| * | ||
| * @group expertParam | ||
| */ | ||
| val parallelism = new IntParam(this, "parallelism", | ||
| "the number of threads to use when running parallel algorithms", ParamValidators.gtEq(1)) | ||
|
|
||
| setDefault(parallelism -> 1) | ||
|
|
||
| /** @group expertGetParam */ | ||
| def getParallelism: Int = $(parallelism) | ||
|
|
||
| /** | ||
| * Create a new execution context with a thread-pool that has a maximum number of threads | ||
| * set to the value of [[parallelism]]. If this param is set to 1, a same-thread executor | ||
| * will be used to run in serial. | ||
| */ | ||
| private[ml] def getExecutionContext: ExecutionContext = { | ||
| getParallelism match { | ||
| case 1 => | ||
| ThreadUtils.sameThread | ||
| case n => | ||
| ExecutionContext.fromExecutorService(ThreadUtils | ||
| .newDaemonCachedThreadPool(s"${this.getClass.getSimpleName}-thread-pool", n)) | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,33 @@ class CrossValidatorSuite | |
| } | ||
| } | ||
|
|
||
| test("cross validation with parallel evaluation") { | ||
| val lr = new LogisticRegression | ||
| val lrParamMaps = new ParamGridBuilder() | ||
| .addGrid(lr.regParam, Array(0.001, 1000.0)) | ||
| .addGrid(lr.maxIter, Array(0, 3)) | ||
| .build() | ||
| val eval = new BinaryClassificationEvaluator | ||
| val cv = new CrossValidator() | ||
| .setEstimator(lr) | ||
| .setEstimatorParamMaps(lrParamMaps) | ||
| .setEvaluator(eval) | ||
| .setNumFolds(2) | ||
| .setParallelism(1) | ||
| val cvSerialModel = cv.fit(dataset) | ||
| cv.setParallelism(2) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do we validate setParallelism is parallelizing?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little difficult to do this in a unit test without making it flaky. I have run tests manually and verified it is working by both the expected speedup in timing and that the expected number of tasks are run concurrently. I can post some results if that would help.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BryanCutler may be worth posting the result to the JIRA for posterity. |
||
| val cvParallelModel = cv.fit(dataset) | ||
|
|
||
| val serialMetrics = cvSerialModel.avgMetrics.sorted | ||
| val parallelMetrics = cvParallelModel.avgMetrics.sorted | ||
| assert(serialMetrics === parallelMetrics) | ||
|
|
||
| val parentSerial = cvSerialModel.bestModel.parent.asInstanceOf[LogisticRegression] | ||
| val parentParallel = cvParallelModel.bestModel.parent.asInstanceOf[LogisticRegression] | ||
| assert(parentSerial.getRegParam === parentParallel.getRegParam) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to also check
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should probably be done in the test that already runs |
||
| assert(parentSerial.getMaxIter === parentParallel.getMaxIter) | ||
| } | ||
|
|
||
| test("read/write: CrossValidator with simple estimator") { | ||
| val lr = new LogisticRegression().setMaxIter(3) | ||
| val evaluator = new BinaryClassificationEvaluator() | ||
|
|
||
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.
add
.setSeed(XXX)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.
So the seed param here is fixed by default and doesn't need to be set to ensure consistent results. I think that's why it's not set in the other tests in this suite. I'm not a fan of this behavior and I think it's better to explicitly set in tests, but then we should probably be consistent and set elsewhere too. What are your thoughts on this @MLnick ?
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.
OK I agree.
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.
Yeah seed defaults to a hash of the class name. There has been debate over this (see SPARK-16832). Personally I also don't like that behavior, but for now that's what it is.