From b0e2e599d272511ae8cdbf55c553ff0bb2334870 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Wed, 12 Aug 2026 14:06:28 -0700 Subject: [PATCH] feat(operator): give the KNN trainers' metric and metric_params a working converter SklearnAdvancedKNNParameters pairs each hyperparameter with the Python callable that converts what the user typed. Two of the seven named one that cannot produce what scikit-learn accepts, so picking either failed the run whatever was entered, from the same dropdown as the five that work. Both trainers share the enum, so both were affected. metric was declared int. Its accepted values are words, so int("minkowski") raises before scikit-learn sees anything, and a number that does convert is rejected as not one of the accepted names. It is now str, which weights and algorithm beside it already are. metric_params takes a mapping of extra keyword arguments for the metric, and none of int, float or str returns one, so a well-formed {"p": 2} arrived as that same text. It now names json.loads. The type has never been limited to builtins (SVC and SVR name an inline lambda for their boolean parameters), but json.loads needs the module, so the generated template imports json. Unconditionally rather than when such a parameter is present: the alternative is threading each converter's imports through every ParamClass for one parameter of one operator. Checked against scikit-learn both ways. Before: int("minkowski") raises ValueError, and metric=int("3") reaches the estimator and is rejected as not an accepted metric name. After: minkowski with metric_params {"p": 3} fits and predicts with the mapping arriving as a dict, chebyshev fits on the regressor, and mahalanobis with a VI matrix fits, which is what metric_params is for. Co-Authored-By: Claude Opus 5 (1M context) --- .../SklearnAdvancedKNNParameters.java | 8 ++- .../base/SklearnAdvancedBaseDesc.scala | 4 ++ ...vancedKNNClassifierTrainerOpDescSpec.scala | 55 ++++++++++++++++++- .../base/SklearnAdvancedBaseDescSpec.scala | 3 + 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java index 7bb8c9dd9ac..9ab29dbe618 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java @@ -27,8 +27,12 @@ public enum SklearnAdvancedKNNParameters implements ParamClass { weights("weights", "str"), algorithm("algorithm", "str"), leaf_size("leaf_size", "int"), - metric("metric", "int"), - metric_params("metric_params", "str"); + // A metric is named, not measured: "minkowski" and the rest of the accepted + // set are words, so int() rejects every value scikit-learn would take. + metric("metric", "str"), + // The only one that is not a scalar. scikit-learn wants a mapping of extra + // keyword arguments for the metric, so the user's text is read as JSON. + metric_params("metric_params", "json.loads"); private final String name; private final String type; diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala index 3127fa91232..87a59bdcf25 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala @@ -98,10 +98,14 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperat val stringList = getParameter(paraList) val trainingParam = stringList(1) val paramString = stringList(0) + // A hyperparameter's declared type is emitted as the callable that converts the + // user's text, and the one taking a mapping names json.loads, so json is imported + // whether or not the operator at hand offers such a parameter. val finalCode = pyb""" |from pytexera import * | + |import json |import pandas as pd |${getImportStatements} | diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala index 7c62b0cf2c7..7d3acae13e1 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala @@ -19,11 +19,33 @@ package org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.KNNTrainer -import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.SklearnMLOperatorDescriptor +import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.{ + HyperParameters, + SklearnMLOperatorDescriptor +} import org.scalatest.flatspec.AnyFlatSpec class SklearnAdvancedKNNClassifierTrainerOpDescSpec extends AnyFlatSpec { + private def hyperParam( + parameter: SklearnAdvancedKNNParameters, + value: String + ): HyperParameters[SklearnAdvancedKNNParameters] = { + val hp = new HyperParameters[SklearnAdvancedKNNParameters] + hp.parameter = parameter + hp.parametersSource = false + hp.value = value + hp + } + + private def codeFor(paraList: HyperParameters[SklearnAdvancedKNNParameters]*): String = { + val d = new SklearnAdvancedKNNClassifierTrainerOpDesc + d.paraList = paraList.toList + d.selectedFeatures = List("f1") + d.groundTruthAttribute = "label" + d.generatePythonCode() + } + "SklearnAdvancedKNNClassifierTrainerOpDesc.getImportStatements" should "return the canonical KNeighborsClassifier import" in { val d = new SklearnAdvancedKNNClassifierTrainerOpDesc @@ -58,4 +80,35 @@ class SklearnAdvancedKNNClassifierTrainerOpDescSpec extends AnyFlatSpec { } assert(matched) } + + // The declared type is emitted as the callable that converts what the user + // typed, so a hyperparameter is only usable when that callable can return + // something scikit-learn accepts. These two could not: `metric` names a word + // from a fixed set, and `metric_params` a mapping. + "SklearnAdvancedKNNParameters.metric" should + "convert with str, the accepted metrics being words" in { + assert( + codeFor(hyperParam(SklearnAdvancedKNNParameters.metric, "minkowski")) + .contains("metric = str (") + ) + } + + "SklearnAdvancedKNNParameters.metric_params" should + "convert with json.loads, and the template must import json for it" in { + val code = codeFor(hyperParam(SklearnAdvancedKNNParameters.metric_params, """{"p": 2}""")) + assert(code.contains("metric_params = json.loads (")) + assert(code.contains("import json")) + } + + it should "apply both conversions in the model call, not only in the summary" in { + val code = codeFor( + hyperParam(SklearnAdvancedKNNParameters.metric, "minkowski"), + hyperParam(SklearnAdvancedKNNParameters.metric_params, """{"p": 2}""") + ) + // The values themselves travel base64-encoded, so what is checked here is + // which callable each one is handed to. + val modelCall = code.linesIterator.find(_.contains("model = KNeighborsClassifier(")).get + assert(modelCall.contains("metric = str (")) + assert(modelCall.contains("metric_params = json.loads (")) + } } diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala index ba620af298a..f649601875f 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala @@ -94,6 +94,9 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { val d = newOp(List(hyperParam("n_neighbors", "int", fromWorkflow = false, value = "5"))) val code = d.generatePythonCode() code should include("from pytexera import *") + // Unconditional: a hyperparameter's declared type is emitted as the callable + // that converts the user's text, and the one taking a mapping names json.loads. + code should include("import json") code should include("from sklearn.neighbors import KNeighborsClassifier") code should include("class ProcessTableOperator(UDFTableOperator):") code should include("def process_table(")