Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import java.util.{List => JList}
import java.util.UUID

import scala.collection.JavaConverters._
import scala.collection.parallel.ForkJoinTaskSupport
import scala.concurrent.forkjoin.ForkJoinPool
import scala.language.existentials

import org.apache.hadoop.fs.Path
Expand All @@ -33,7 +35,7 @@ import org.apache.spark.annotation.Since
import org.apache.spark.ml._
import org.apache.spark.ml.attribute._
import org.apache.spark.ml.linalg.Vector
import org.apache.spark.ml.param.{Param, ParamMap, ParamPair, Params}
import org.apache.spark.ml.param.{IntParam, Param, ParamMap, ParamPair, Params, ParamValidators}
import org.apache.spark.ml.util._
import org.apache.spark.sql.{DataFrame, Dataset, Row}
import org.apache.spark.sql.functions._
Expand Down Expand Up @@ -65,6 +67,12 @@ private[ml] trait OneVsRestParams extends PredictorParams with ClassifierTypeTra

/** @group getParam */
def getClassifier: ClassifierType = $(classifier)

val parallelism = new IntParam(this, "parallelism",
"parallelism parameter for tuning amount of parallelism", ParamValidators.gtEq(1))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not very informative. Can you please make it more explicit?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also

  • add Scala doc
  • add Since annotation
  • Mark this as an expertParam (the "group" annotation in the Scala doc); see other parts of the code for examples


/** @group getParam */
def getParallelism: Int = $(parallelism)
}

private[ml] object OneVsRestParams extends ClassifierTypeTrait {
Expand Down Expand Up @@ -273,6 +281,10 @@ final class OneVsRest @Since("1.4.0") (
@Since("1.4.0") override val uid: String)
extends Estimator[OneVsRestModel] with OneVsRestParams with MLWritable {

setDefault(
parallelism -> 4
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move the HasParallelism into OneVsRestParams ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It will be used by cross validator and train validation split, that's why it's shared.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we can define
trait OneVsRestParams extends PredictorParams with HasParallelism
trait TrainValidationSplitParams extends ValidatorParams with HasParallelism
and so on, like other ml algos do ?
But I am not sure whether there is other reasons.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@WeichenXu123 earlier in the discussion for this PR we had been coordinating with similar changes for parallel cross validator and train validation split at #16774 .

If trait OneVsRestParams extends PredictorParams with HasParallelism was done instead, then it would just give the parallelism param to the OneVsRestModel which isn't really useful since it's already been trained.

@Since("1.4.0")
def this() = this(Identifiable.randomUID("oneVsRest"))

Expand All @@ -282,6 +294,12 @@ final class OneVsRest @Since("1.4.0") (
set(classifier, value.asInstanceOf[ClassifierType])
}

/** @group setParam */
@Since("1.4.0")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

next release will be 2.3

def setParallelism(value: Int): this.type = {
set(parallelism, value)
}

/** @group setParam */
@Since("1.5.0")
def setLabelCol(value: String): this.type = set(labelCol, value)
Expand Down Expand Up @@ -325,8 +343,13 @@ final class OneVsRest @Since("1.4.0") (
multiclassLabeled.persist(StorageLevel.MEMORY_AND_DISK)
}

val iters = Range(0, numClasses).par

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CC @thunterdb just to double-check

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jkbradley thanks for calling this out. Indeed, the code as it stands may cause some non-deterministic issues in complex environments. I put a comment about that in that PR:
#16774
See how it is done in this file:
https://github.com/apache/spark/pull/16774/files#diff-d14539cadce0fba9d8b7d970adaf8b26

It should be a quick change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Discussed offline with @thunterdb : Setting the parallel collection tasksupport value is essentially doing the same thing as in @BryanCutler 's PR: ForkJoinTaskSupport is using the ExecutionContext created by ForkJoinPool under the hood.

iters.tasksupport = new ForkJoinTaskSupport(
new ForkJoinPool(getParallelism)
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

might be good to log a message here indicating the parallelism used

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can use instr.log?

// create k columns, one for each binary classifier.
val models = Range(0, numClasses).par.map { index =>
val models = iters.map { index =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using Futures is a simple change and imho is clearer than using parallel collections once you start messing around with the task support. It would just require changing a couple lines to something like this

  ...
  Future {classifier.fit(trainingDataset, paramMap)} (executionContext)
}.map(ThreadUtils.awaitResult(_, Duration.Inf)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do Futures add? They require more code, and the functionality seems to be the same for the purposes of OneVsRest.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They don't necessarily add anything to here, but they are a more standard way of parallelism in Spark over using TaskSupport and it's more flexible for setting an ExecutorService. I'm not sure if you can set TaskSupport to sameThreadExecutor or what really happens behind the scenes if you make a ThreadPoolTaskSupport with 1 thread.

// generate new label metadata for the binary problem.
val newLabelMeta = BinaryAttribute.defaultAttr.withName("label").toMetadata()
val labelColName = "mc2b$" + index
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,40 @@ class OneVsRestSuite extends SparkFunSuite with MLlibTestSparkContext with Defau
assert(expectedMetrics.confusionMatrix ~== ovaMetrics.confusionMatrix absTol 400)
}

test("one-vs-rest: tuning parallelism produces correct output") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"produces correct output" --> "does not affect output"

This test appears to check OVR vs. another algorithm. I think a more precise test would check that tuning parallelism still produces exactly the same models. Could you please update it to do so?

val numClasses = 3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not really sure what this is doing, it is not used?

val ova = new OneVsRest()
.setClassifier(new LogisticRegression)
.setParallelism(8)
assert(ova.getLabelCol === "label")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to check Params. That's not what this unit test is for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are several things like that below; just go through and remove items which are not part of this unit test.

assert(ova.getPredictionCol === "prediction")
val ovaModel = ova.fit(dataset)

MLTestingUtils.checkCopyAndUids(ova, ovaModel)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a generic test which only needs to be done in 1 test for each algorithm. You can remove it here.


assert(ovaModel.models.length === numClasses)
val transformedDataset = ovaModel.transform(dataset)

// check for label metadata in prediction col
val predictionColSchema = transformedDataset.schema(ovaModel.getPredictionCol)
assert(MetadataUtils.getNumClasses(predictionColSchema) === Some(3))

val ovaResults = transformedDataset.select("prediction", "label").rdd.map {
row => (row.getDouble(0), row.getDouble(1))
}

val lr = new LogisticRegressionWithLBFGS().setIntercept(true).setNumClasses(numClasses)
lr.optimizer.setRegParam(0.1).setNumIterations(100)

val model = lr.run(rdd.map(OldLabeledPoint.fromML))
val results = model.predict(rdd.map(p => OldVectors.fromML(p.features))).zip(rdd.map(_.label))
// determine the #confusion matrix in each class.
// bound how much error we allow compared to multinomial logistic regression.
val expectedMetrics = new MulticlassMetrics(results)
val ovaMetrics = new MulticlassMetrics(ovaResults)
assert(expectedMetrics.confusionMatrix ~== ovaMetrics.confusionMatrix absTol 400)
}

test("one-vs-rest: pass label metadata correctly during train") {
val numClasses = 3
val ova = new OneVsRest()
Expand Down
31 changes: 26 additions & 5 deletions python/pyspark/ml/classification.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#

import operator
from multiprocessing.pool import ThreadPool

from pyspark import since, keyword_only
from pyspark.ml import Estimator, Model
Expand Down Expand Up @@ -1510,21 +1511,26 @@ class OneVsRest(Estimator, OneVsRestParams, MLReadable, MLWritable):

.. versionadded:: 2.0.0
"""
parallelism = Param(Params._dummy(), "parallelism",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is a shared Param in Scala, do you want to make it one in Python too? You can add it to _shared_params_code_gen.py and then re-generate the shared.py file.

"Number of models to fit in parallel",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When you update the doc in Scala, you can update it here too.

typeConverter=TypeConverters.toInt)

@keyword_only
def __init__(self, featuresCol="features", labelCol="label", predictionCol="prediction",
classifier=None):
classifier=None, parallelism=4):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think should be the same default as scala

"""
__init__(self, featuresCol="features", labelCol="label", predictionCol="prediction", \
classifier=None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add parallelism=1 here too

"""
super(OneVsRest, self).__init__()
self._setDefault(parallelism=4)
kwargs = self._input_kwargs
self._set(**kwargs)

@keyword_only
@since("2.0.0")
def setParams(self, featuresCol=None, labelCol=None, predictionCol=None, classifier=None):
def setParams(self, featuresCol=None, labelCol=None, predictionCol=None,
classifier=None, parallelism=None):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Discrepency here between __init__ (default 1) and setParams (default None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, it's strange these were all None before, would you mind changing these all to match the defaults in the constructor?

"""
setParams(self, featuresCol=None, labelCol=None, predictionCol=None, classifier=None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto: add parallelism=1 to doc

Sets params for OneVsRest.
Expand Down Expand Up @@ -1560,14 +1566,27 @@ def trainSingleClass(index):
(classifier.predictionCol, predictionCol)])
return classifier.fit(trainingDataset, paramMap)

# TODO: Parallel training for all classes.
models = [trainSingleClass(i) for i in range(numClasses)]
pool = ThreadPool(processes=self.getParallelism())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One new thought: It'd be good to set processes to min(parallelism, numClasses). Same in Scala.


models = pool.map(trainSingleClass, range(numClasses))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So if the training of the models takes different times and there is a relatively large number of models we might end up blocking a worker here since the map effectively splits in advance. What do you think of providing a chunksize hint or using imap or imap_unordered instead? Since the overhead of sending the element to the worker should be relatively small compared to the time it takes to train the model?


if handlePersistence:
multiclassLabeled.unpersist()

return self._copyValues(OneVsRestModel(models=models))

def setParallelism(self, value):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add Since annotations

"""
Sets the value of :py:attr:`parallelism`.
"""
return self._set(parallelism=value)

def getParallelism(self):
"""
Gets the value of parallelism or its default value.
"""
return self.getOrDefault(self.parallelism)

@since("2.0.0")
def copy(self, extra=None):
"""
Expand Down Expand Up @@ -1611,8 +1630,9 @@ def _from_java(cls, java_stage):
labelCol = java_stage.getLabelCol()
predictionCol = java_stage.getPredictionCol()
classifier = JavaParams._from_java(java_stage.getClassifier())
parallelism = java_stage.getParallelism()
py_stage = cls(featuresCol=featuresCol, labelCol=labelCol, predictionCol=predictionCol,
classifier=classifier)
classifier=classifier, parallelism=parallelism)
py_stage._resetUid(java_stage.uid())
return py_stage

Expand All @@ -1625,6 +1645,7 @@ def _to_java(self):
_java_obj = JavaParams._new_java_obj("org.apache.spark.ml.classification.OneVsRest",
self.uid)
_java_obj.setClassifier(self.getClassifier()._to_java())
_java_obj.setParallelism(self.getParallelism())
_java_obj.setFeaturesCol(self.getFeaturesCol())
_java_obj.setLabelCol(self.getLabelCol())
_java_obj.setPredictionCol(self.getPredictionCol())
Expand Down
34 changes: 31 additions & 3 deletions python/pyspark/ml/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@ def test_onevsrest(self):
(2.0, Vectors.dense(0.5, 0.5))] * 10,
["label", "features"])
lr = LogisticRegression(maxIter=5, regParam=0.01)
ovr = OneVsRest(classifier=lr)
ovr = OneVsRest(classifier=lr, parallelism=8)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just use the default here, don't set the param

model = ovr.fit(df)
ovrPath = temp_path + "/ovr"
ovr.save(ovrPath)
Expand Down Expand Up @@ -1215,7 +1215,7 @@ def test_copy(self):
(2.0, Vectors.dense(0.5, 0.5))],
["label", "features"])
lr = LogisticRegression(maxIter=5, regParam=0.01)
ovr = OneVsRest(classifier=lr)
ovr = OneVsRest(classifier=lr, parallelism=1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if the default is 1, don't need this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not needed since default is 1

ovr1 = ovr.copy({lr.maxIter: 10})
self.assertEqual(ovr.getClassifier().getMaxIter(), 5)
self.assertEqual(ovr1.getClassifier().getMaxIter(), 10)
Expand All @@ -1229,7 +1229,35 @@ def test_output_columns(self):
(2.0, Vectors.dense(0.5, 0.5))],
["label", "features"])
lr = LogisticRegression(maxIter=5, regParam=0.01)
ovr = OneVsRest(classifier=lr)
ovr = OneVsRest(classifier=lr, parallelism=1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same as above

model = ovr.fit(df)
output = model.transform(df)
self.assertEqual(output.columns, ["label", "features", "prediction"])


class ParOneVsRestTests(SparkSessionTestCase):

def test_copy(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this testing?

df = self.spark.createDataFrame([(0.0, Vectors.dense(1.0, 0.8)),
(1.0, Vectors.sparse(2, [], [])),
(2.0, Vectors.dense(0.5, 0.5))],
["label", "features"])
lr = LogisticRegression(maxIter=5, regParam=0.01)
ovr = OneVsRest(classifier=lr, parallelism=8)
ovr1 = ovr.copy({lr.maxIter: 10})
self.assertEqual(ovr.getClassifier().getMaxIter(), 5)
self.assertEqual(ovr1.getClassifier().getMaxIter(), 10)
model = ovr.fit(df)
model1 = model.copy({model.predictionCol: "indexed"})
self.assertEqual(model1.getPredictionCol(), "indexed")

def test_output_columns(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto: is this needed?

How about adding a test like the one I proposed for Scala, which makes sure the same model is learned regardless of parallelism?

df = self.spark.createDataFrame([(0.0, Vectors.dense(1.0, 0.8)),
(1.0, Vectors.sparse(2, [], [])),
(2.0, Vectors.dense(0.5, 0.5))],
["label", "features"])
lr = LogisticRegression(maxIter=5, regParam=0.01)
ovr = OneVsRest(classifier=lr, parallelism=8)
model = ovr.fit(df)
output = model.transform(df)
self.assertEqual(output.columns, ["label", "features", "prediction"])
Expand Down