spark-test commented on code in PR #60179: URL: https://github.com/apache/spark/pull/60179#discussion_r4500472395
########## mllib/src/test/scala/org/apache/spark/ml/classification/ClassifierStorageLevelSuite.scala: ########## @@ -0,0 +1,82 @@ +/* + * 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.classification + +import java.util.concurrent.ConcurrentLinkedQueue + +import scala.jdk.CollectionConverters._ + +import org.apache.spark.ml.Estimator +import org.apache.spark.ml.linalg.Vectors +import org.apache.spark.ml.param.ParamMap +import org.apache.spark.ml.param.shared.HasIntermediateStorageLevel +import org.apache.spark.ml.util.{DefaultReadWriteTest, MLWritable} +import org.apache.spark.scheduler.{SparkListener, SparkListenerBlockUpdated} +import org.apache.spark.sql.QueryTest +import org.apache.spark.sql.test.SharedSparkSession +import org.apache.spark.storage.{RDDBlockId, StorageLevel} + +class ClassifierStorageLevelSuite + extends QueryTest with SharedSparkSession with DefaultReadWriteTest { + + import testImplicits._ + + namedGridTest("intermediateStorageLevel")( + Map[String, Estimator[_] with HasIntermediateStorageLevel with MLWritable]( + "LogisticRegression" -> new LogisticRegression().setMaxIter(2), + "LinearSVC" -> new LinearSVC().setMaxIter(2), + "FMClassifier" -> new FMClassifier().setMaxIter(2).setSeed(42))) { classifier => + assert(classifier.getIntermediateStorageLevel === "MEMORY_AND_DISK") + Seq("NONE", "invalid").foreach { level => + intercept[IllegalArgumentException] { + classifier.set(classifier.intermediateStorageLevel, level) + } + } + classifier.set(classifier.intermediateStorageLevel, "DISK_ONLY") Review Comment: **Nit (P3):** This line, like the `NONE`/`invalid` checks above it, goes through the generic `set`, because the grid's element type only exposes the shared `HasIntermediateStorageLevel` trait. So the suite never exercises the `setIntermediateStorageLevel` methods this PR adds to `LogisticRegression`, `LinearSVC` and `FMClassifier`. The Python test doesn't cover them either, since classic PySpark and Connect both apply params on the JVM through generic `set`. If one of these setters were later bound to a different param or dropped, every test in this PR would still pass. For example, if `LinearSVC`'s setter wrote `weightCol` instead, which has no validator, the mistake would only surface later as a missing-column error at `fit`. `KMeansSuite` and `ALSSuite` call their own `setIntermediateStorageLevel` for this same param, including the `NONE` and unknown-name cases. The current setters are correct; the gap only matters for future edits. **Verification:** - **Regression:** For each of the three classifiers, a test calls the typed setter with a non-default level such as DISK_ONLY and asserts that the getter reports that level and that the call returns the same estimator instance, so a setter bound to a different param, or a removed setter, makes the test fail. - **Behavior:** For each of the three classifiers, NONE and an unknown storage level name passed through the typed setter raise IllegalArgumentException, while the existing checks for the default, copy, save/load, model propagation and the DISK_ONLY block level used during fit continue to pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
