From 6e80b6270e86ccce504d3199fa0e94ef4d3fa67a Mon Sep 17 00:00:00 2001 From: kary zheng Date: Mon, 24 Aug 2026 14:58:28 -0700 Subject: [PATCH 1/7] feat(operator): give a trainer's hyperparameter value the constraint its parameter implies A hyperparameter row's value was a bare text box: no accepted values, no format, no example, and not required. The enum behind the parameter dropdown already pairs each parameter with the callable that converts its text, so the operator knew what it would accept and never said. The enum now declares that too, taken from scikit-learn itself rather than from judgement, and the descriptor writes it into its own schema. The rules sit under a Texera key rather than a JSON-Schema allOf, whose members the form builder merges into one control. The form then renders what they call for: a dropdown for a chosen-from-a-set parameter, a number input for a numeric one. Closes #7936 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../SklearnAdvancedKNNParameters.java | 32 +++-- .../SklearnAdvancedSVCParameters.java | 31 +++-- .../SklearnAdvancedSVRParameters.java | 40 ++++-- .../base/HyperParameters.scala | 16 +++ .../base/SklearnAdvancedBaseDesc.scala | 131 +++++++++++++++++- .../metadata/JsonSchemaCustomizer.scala | 37 +++++ .../metadata/OperatorMetadataGenerator.scala | 5 + .../base/SklearnAdvancedBaseDescSpec.scala | 118 +++++++++++++++- .../src/app/common/formly/formly-config.ts | 2 + .../app/common/formly/formly-utils.spec.ts | 105 ++++++++++++++ .../src/app/common/formly/formly-utils.ts | 66 ++++++++- .../constrained-value.component.ts | 95 +++++++++++++ .../operator-property-edit-frame.component.ts | 20 +++ .../types/custom-json-schema.interface.ts | 26 ++++ 14 files changed, 691 insertions(+), 33 deletions(-) create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/JsonSchemaCustomizer.scala create mode 100644 frontend/src/app/workspace/component/constrained-value/constrained-value.component.ts 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..7456e5516e3 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 @@ -22,20 +22,28 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedKNNParameters implements ParamClass { - n_neighbors("n_neighbors", "int"), - p("p", "int"), - weights("weights", "str"), - algorithm("algorithm", "str"), - leaf_size("leaf_size", "int"), - metric("metric", "int"), - metric_params("metric_params", "str"); + n_neighbors("n_neighbors", "int", "5"), + p("p", "int", "2"), + weights("weights", "str", "", "uniform", "distance"), + algorithm("algorithm", "str", "", "auto", "ball_tree", "kd_tree", "brute"), + leaf_size("leaf_size", "int", "30"), + // The last two have no example and no accepted set, because neither has a value worth + // naming under the converter it declares: the metrics are words while int() takes only + // numbers, and metric_params is a mapping that str() cannot produce. + metric("metric", "int", ""), + metric_params("metric_params", "str", ""); private final String name; private final String type; + private final String sampleValue; + private final String[] allowedValues; - SklearnAdvancedKNNParameters(String name, String type) { + SklearnAdvancedKNNParameters( + String name, String type, String sampleValue, String... allowedValues) { this.name = name; this.type = type; + this.sampleValue = sampleValue; + this.allowedValues = allowedValues; } public String getType() { @@ -45,4 +53,12 @@ public String getType() { public String getName() { return this.name; } + + public String getSampleValue() { + return this.sampleValue; + } + + public String[] getAllowedValues() { + return this.allowedValues.clone(); + } } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java index c2e6b6df3f6..2a3969b31e2 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java @@ -22,20 +22,27 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedSVCParameters implements ParamClass { - C("C", "float"), - kernel("kernel", "str"), - gamma("gamma", "float"), - degree("degree", "int"), - coef0("coef0", "float"), - tol("tol", "float"), - probability("probability", "(lambda value: value.lower() == \"true\")"); + C("C", "float", "1.0"), + kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), + // SVC's own default for gamma is "scale", which float() cannot convert, so there is no + // example to offer until the declared converter can carry one. + gamma("gamma", "float", ""), + degree("degree", "int", "3"), + coef0("coef0", "float", "0.0"), + tol("tol", "float", "0.001"), + probability("probability", "(lambda value: value.lower() == \"true\")", "", "false", "true"); private final String name; private final String type; + private final String sampleValue; + private final String[] allowedValues; - SklearnAdvancedSVCParameters(String name, String type) { + SklearnAdvancedSVCParameters( + String name, String type, String sampleValue, String... allowedValues) { this.name = name; this.type = type; + this.sampleValue = sampleValue; + this.allowedValues = allowedValues; } public String getType() { @@ -45,4 +52,12 @@ public String getType() { public String getName() { return this.name; } + + public String getSampleValue() { + return this.sampleValue; + } + + public String[] getAllowedValues() { + return this.allowedValues.clone(); + } } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java index 898db274dc8..760545579d3 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java @@ -22,24 +22,32 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedSVRParameters implements ParamClass { - C("C", "float"), - kernel("kernel", "str"), - gamma("gamma", "float"), - degree("degree", "int"), - coef0("coef0", "float"), - tol("tol", "float"), - probability("shrinking", "(lambda value: value.lower() == \"true\")"), - verbose("verbose", "(lambda value: value.lower() == \"true\")"), - epsilon("epsilon", "float"), - cache_size("cache_size", "int"), - max_iter("max_iter", "int"); + C("C", "float", "1.0"), + kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), + // SVR's own default for gamma is "scale", which float() cannot convert, so there is no + // example to offer until the declared converter can carry one. + gamma("gamma", "float", ""), + degree("degree", "int", "3"), + coef0("coef0", "float", "0.0"), + tol("tol", "float", "0.001"), + probability("shrinking", "(lambda value: value.lower() == \"true\")", "", "true", "false"), + verbose("verbose", "(lambda value: value.lower() == \"true\")", "", "false", "true"), + epsilon("epsilon", "float", "0.1"), + cache_size("cache_size", "int", "200"), + // -1 is SVR's own value for no iteration limit. + max_iter("max_iter", "int", "-1"); private final String name; private final String type; + private final String sampleValue; + private final String[] allowedValues; - SklearnAdvancedSVRParameters(String name, String type) { + SklearnAdvancedSVRParameters( + String name, String type, String sampleValue, String... allowedValues) { this.name = name; this.type = type; + this.sampleValue = sampleValue; + this.allowedValues = allowedValues; } public String getType() { @@ -49,4 +57,12 @@ public String getType() { public String getName() { return this.name; } + + public String getSampleValue() { + return this.sampleValue; + } + + public String[] getAllowedValues() { + return this.allowedValues.clone(); + } } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/HyperParameters.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/HyperParameters.scala index 13fdb9aa60f..d790e68f9e3 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/HyperParameters.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/HyperParameters.scala @@ -27,6 +27,22 @@ import org.apache.texera.amber.operator.metadata.annotations.{ HideAnnotation } +/** + * One row of a trainer's parameter table. `parametersSource` decides which of the two inputs + * the row uses, and the hide rules below show only that one, so exactly one of them is needed + * and neither can be required outright. + */ +@JsonSchemaInject(json = """ +{ + "allOf": [ + { + "if": { "properties": { "parametersSource": { "const": true } } }, + "then": { "required": ["attribute"] }, + "else": { "required": ["value"] } + } + ] +} +""") class HyperParameters[T] { @JsonProperty(required = true) 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..038bf83e541 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 @@ -30,15 +30,46 @@ import org.apache.texera.amber.operator.metadata.annotations.{ AutofillAttributeName, AutofillAttributeNameList } -import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorInfo} +import org.apache.texera.amber.operator.metadata.{ + JsonSchemaCustomizer, + OperatorGroupConstants, + OperatorInfo +} import org.apache.texera.amber.pybuilder.PythonTemplateBuilder +import org.apache.texera.amber.util.JSONUtils.objectMapper +import com.fasterxml.jackson.databind.node.ObjectNode +import java.lang.reflect.{ParameterizedType, Type} + +/** + * One hyperparameter a trainer offers. `getName` is the keyword argument passed to the + * estimator and `getType` the callable converting the user's text, so together they already + * decide which values the estimator can be given. The two below state that so the form can + * hold the user to it, rather than leaving it to be discovered when `fit` raises. + */ trait ParamClass { def getName: String def getType: String + + /** One value this parameter accepts, offered as an example rather than imposed as a + * default: what the estimator itself documents as its default is the obvious choice, but + * nothing here decides a hyperparameter on the user's behalf. Empty for a parameter with an + * accepted set, which already names every value worth offering, and empty where even the + * estimator's own default is a value `getType` cannot convert, since an example the + * operator would then reject is worse than none. + */ + def getSampleValue: String + + /** The values the estimator accepts, where it accepts a fixed set rather than a range, the + * estimator's own default first. Empty for a parameter taking any number, which its + * converter already constrains. + */ + def getAllowedValues: Array[String] } -abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperatorDescriptor { +abstract class SklearnMLOperatorDescriptor[T <: ParamClass] + extends PythonOperatorDescriptor + with JsonSchemaCustomizer { @JsonIgnore def getImportStatements: String @@ -61,6 +92,102 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperat @AutofillAttributeNameList var selectedFeatures: List[EncodableString] = _ + /** + * State what a `paraList` row's `value` may hold, which depends on the `parameter` chosen + * beside it and so cannot be annotated on a field shared by every parameter. + * + * The rules go under a key of Texera's own rather than as a JSON-Schema `allOf`, for the + * same reason `attributeTypeRules` does, and whose grammar they borrow: the form builder + * merges the members of an `allOf` into a single field, which would leave one control + * carrying every parameter's constraints at once. + */ + override def customizeJsonSchema(schema: ObjectNode): Unit = { + val rowSchema = hyperParameterRowSchema(schema) + if (rowSchema == null) return + val value = rowSchema.path("properties").path("value") + if (!value.isObject) return + + val branches = objectMapper.createArrayNode() + paramConstants.foreach { param => + val condition = objectMapper.createObjectNode() + condition + .putObject("parameter") + .putArray("valEnum") + .add(param.getName) + + val outcome = objectMapper.createObjectNode() + if (param.getAllowedValues.nonEmpty) { + // The accepted set says everything: it names each value a reader could pick and each + // value a sweep should try, so an example alongside it would only repeat one of them. + param.getAllowedValues.foreach(outcome.withArray("enum").add) + } else { + valueTypeOf(param).foreach(outcome.put("type", _)) + if (param.getSampleValue.nonEmpty) outcome.withArray("examples").add(param.getSampleValue) + } + + if (!outcome.isEmpty) { + val branch = objectMapper.createObjectNode() + branch.set[ObjectNode]("if", condition) + branch.set[ObjectNode]("then", outcome) + branches.add(branch) + } + } + if (!branches.isEmpty) + value + .asInstanceOf[ObjectNode] + .putObject("valueRules") + .set[ObjectNode]("allOf", branches) + } + + /** How the form should read a value with no fixed set of its own: from the callable the + * parameter names, since that is what the emitted code puts the text through. A parameter + * converted by anything else is left unconstrained rather than guessed at. + */ + private def valueTypeOf(param: ParamClass): Option[String] = + param.getType match { + case "int" => Some("integer") + case "float" | "double" => Some("number") + case _ => None + } + + /** The `HyperParameters` definition this operator's `paraList` points at. Followed by its + * `$ref` rather than by name, which carries the parameter enum and so differs per operator. + */ + private def hyperParameterRowSchema(schema: ObjectNode): ObjectNode = { + val ref = schema.path("properties").path("paraList").path("items").path("$ref").asText("") + val name = ref.stripPrefix("#/definitions/") + if (name.isEmpty) return null + schema.path("definitions").path(name) match { + case row: ObjectNode => row + case _ => null + } + } + + /** The hyperparameters this operator offers, from the enum bound to `T`. The field itself + * cannot say: erasure leaves `paraList` holding a plain `HyperParameters`, so the binding + * survives only on the generic supertype. Empty where `T` is not an enum, which is only + * ever a test stub standing in for one. + */ + private def paramConstants: Seq[ParamClass] = { + var t: Type = getClass.getGenericSuperclass + while (t != null) t match { + case p: ParameterizedType => + val raw = p.getRawType.asInstanceOf[Class[_]] + if (raw == classOf[SklearnMLOperatorDescriptor[_]]) + return p.getActualTypeArguments()(0) match { + case bound: Class[_] => + val constants = bound.getEnumConstants.asInstanceOf[Array[AnyRef]] + if (constants == null) Seq.empty + else constants.toSeq.map(_.asInstanceOf[ParamClass]) + case _ => Seq.empty + } + t = raw.getGenericSuperclass + case c: Class[_] => t = c.getGenericSuperclass + case _ => t = null + } + Seq.empty + } + private def getLoopTimes(paraList: List[HyperParameters[T]]): PythonTemplateBuilder = { for (ele <- paraList) { if (ele.parametersSource) { diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/JsonSchemaCustomizer.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/JsonSchemaCustomizer.scala new file mode 100644 index 00000000000..b44065ba964 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/JsonSchemaCustomizer.scala @@ -0,0 +1,37 @@ +/* + * 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.texera.amber.operator.metadata + +import com.fasterxml.jackson.databind.node.ObjectNode + +/** + * An operator with part of its schema that annotations cannot express, because that part + * depends on the descriptor's own type argument rather than on any one field. + * + * [[OperatorMetadataGenerator.generateOperatorJsonSchema]] calls this once the annotated + * schema is built, so an implementor edits a finished document rather than producing one. + */ +trait JsonSchemaCustomizer { + + /** Add to `schema` what the annotations could not state. Called with the operator's whole + * schema, `definitions` included. + */ + def customizeJsonSchema(schema: ObjectNode): Unit +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala index fdfcbcf27dc..3086ad851c7 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala @@ -140,6 +140,11 @@ object OperatorMetadataGenerator { jsonSchema.get("required").asInstanceOf[ArrayNode].remove(operatorTypeIndex) // remove "title" for the operator - frontend uses userFriendlyName to show operator title jsonSchema.remove("title") + // let an operator add the part of its schema that its annotations cannot state + opDescClass.getConstructor().newInstance() match { + case customizer: JsonSchemaCustomizer => customizer.customizeJsonSchema(jsonSchema) + case _ => + } jsonSchema } 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..170928de5cd 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 @@ -19,18 +19,26 @@ package org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base +import com.fasterxml.jackson.databind.JsonNode import org.apache.texera.amber.core.tuple.AttributeType -import org.apache.texera.amber.operator.metadata.OperatorGroupConstants +import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.KNNTrainer.SklearnAdvancedKNNClassifierTrainerOpDesc +import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.SVCTrainer.SklearnAdvancedSVCTrainerOpDesc +import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorMetadataGenerator} import org.apache.texera.amber.pybuilder.PyStringTypes.EncodableString import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers +import scala.jdk.CollectionConverters.IteratorHasAsScala + class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { - // A minimal ParamClass: the generated code only ever consults getName/getType. + // A minimal ParamClass: the generated code only ever consults getName/getType, the other + // two being read by the schema rather than by code generation. private class TestParam(name: String, typ: String) extends ParamClass { override def getName: String = name override def getType: String = typ + override def getSampleValue: String = "" + override def getAllowedValues: Array[String] = Array.empty } // A concrete descriptor supplying just the two abstract hooks; everything @@ -135,4 +143,110 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { paramString should include("n_neighbors = int(table[") paramString should include(".values[i]") } + + // The rules are built from the enum bound to the descriptor's type argument, so they need a + // real operator rather than the stub above, whose TestParam is a class and has no constants. + private def valueRulesOf(opClass: Class[_ <: org.apache.texera.amber.operator.LogicalOp]) = { + val schema = OperatorMetadataGenerator.generateOperatorJsonSchema(opClass) + val row = schema + .path("definitions") + .path( + schema + .path("properties") + .path("paraList") + .path("items") + .path("$ref") + .asText() + .stripPrefix("#/definitions/") + ) + row.path("properties").path("value").path("valueRules").path("allOf") + } + + private def ruleFor(rules: JsonNode, parameter: String): JsonNode = + rules + .elements() + .asScala + .find( + _.path("if") + .path("parameter") + .path("valEnum") + .elements() + .asScala + .exists(_.asText() == parameter) + ) + .map(_.path("then")) + .getOrElse(fail(s"no rule for $parameter")) + + "SklearnMLOperatorDescriptor.customizeJsonSchema" should + "offer a chosen-from-a-set parameter exactly the values scikit-learn accepts" in { + val kernel = ruleFor(valueRulesOf(classOf[SklearnAdvancedSVCTrainerOpDesc]), "kernel") + // scikit-learn's own default leads, so a reader taking the first takes that one + kernel.path("enum").elements().asScala.map(_.asText()).toSeq shouldBe + Seq("rbf", "linear", "poly", "sigmoid", "precomputed") + // an accepted set replaces a type rather than joining it: the values are the constraint + kernel.has("type") shouldBe false + // and it replaces an example too, having already named every value worth offering + kernel.has("examples") shouldBe false + } + + it should "read a parameter with no set of its own from the converter it names" in { + val rules = valueRulesOf(classOf[SklearnAdvancedSVCTrainerOpDesc]) + ruleFor(rules, "C").path("type").asText() shouldBe "number" + ruleFor(rules, "degree").path("type").asText() shouldBe "integer" + } + + it should "constrain a parameter it has no example for" in { + // SVC's own default for gamma is a word float() cannot convert, so the rule carries the + // constraint without an example. The two are separate statements and one can stand alone. + val gamma = ruleFor(valueRulesOf(classOf[SklearnAdvancedSVCTrainerOpDesc]), "gamma") + gamma.path("type").asText() shouldBe "number" + gamma.has("examples") shouldBe false + } + + it should "state a rule for every parameter whose converter says anything about it" in { + val rules = valueRulesOf(classOf[SklearnAdvancedKNNClassifierTrainerOpDesc]) + val covered = rules + .elements() + .asScala + .flatMap(_.path("if").path("parameter").path("valEnum").elements().asScala) + .map(_.asText()) + .toSet + // The rules follow the converter each parameter names, not what scikit-learn goes on to + // accept: metric is declared int and so is constrained to whole numbers, even though the + // metrics themselves are words. metric_params names str, which says nothing, so it is the + // one parameter left free. + covered shouldBe Set("n_neighbors", "p", "weights", "algorithm", "leaf_size", "metric") + } + + "HyperParameters" should "require whichever of the two inputs the row actually uses" in { + val schema = + OperatorMetadataGenerator.generateOperatorJsonSchema(classOf[SklearnAdvancedSVCTrainerOpDesc]) + val row = schema + .path("definitions") + .path( + schema + .path("properties") + .path("paraList") + .path("items") + .path("$ref") + .asText() + .stripPrefix("#/definitions/") + ) + val branch = row.path("allOf").path(0) + branch + .path("if") + .path("properties") + .path("parametersSource") + .path("const") + .asBoolean() shouldBe true + branch.path("then").path("required").path(0).asText() shouldBe "attribute" + branch.path("else").path("required").path(0).asText() shouldBe "value" + // neither may be required outright: the hide rules show only one of them at a time + row + .path("required") + .elements() + .asScala + .map(_.asText()) + .toSeq should contain noneOf ("value", "attribute") + } } diff --git a/frontend/src/app/common/formly/formly-config.ts b/frontend/src/app/common/formly/formly-config.ts index c4fc54fd77f..1ea2b679833 100644 --- a/frontend/src/app/common/formly/formly-config.ts +++ b/frontend/src/app/common/formly/formly-config.ts @@ -28,6 +28,7 @@ import { DatasetFileSelectorComponent } from "../../workspace/component/dataset- import { CollabWrapperComponent } from "./collab-wrapper/collab-wrapper/collab-wrapper.component"; import { FormlyRepeatDndComponent } from "./repeat-dnd/repeat-dnd.component"; import { UiUdfParametersComponent } from "../../workspace/component/ui-udf-parameters/ui-udf-parameters.component"; +import { ConstrainedValueComponent } from "../../workspace/component/constrained-value/constrained-value.component"; import { DatasetVersionSelectorComponent } from "../../workspace/component/dataset-version-selector/dataset-version-selector.component"; import { HuggingFaceImageUploadComponent } from "../../workspace/component/hugging-face-image-upload/hugging-face-image-upload.component"; import { HuggingFaceComponent } from "../../workspace/component/hugging-face/hugging-face.component"; @@ -88,6 +89,7 @@ export const TEXERA_FORMLY_CONFIG = { { name: "huggingface-image-upload", component: HuggingFaceImageUploadComponent, wrappers: ["form-field"] }, { name: "repeat-section-dnd", component: FormlyRepeatDndComponent }, { name: "ui-udf-parameters", component: UiUdfParametersComponent, wrappers: ["form-field"] }, + { name: "constrainedvalue", component: ConstrainedValueComponent, wrappers: ["form-field"] }, ], wrappers: [ { name: "preset-wrapper", component: PresetWrapperComponent }, diff --git a/frontend/src/app/common/formly/formly-utils.spec.ts b/frontend/src/app/common/formly/formly-utils.spec.ts index 4bec75a6692..6f37620a74f 100644 --- a/frontend/src/app/common/formly/formly-utils.spec.ts +++ b/frontend/src/app/common/formly/formly-utils.spec.ts @@ -21,10 +21,14 @@ import { FormlyFieldConfig } from "@ngx-formly/core"; import { createOutputFormChangeEventStream, createShouldHideFieldFunc, + createValueRulesValidator, getFieldByName, + matchingValueRule, setChildTypeDependency, setHideExpression, + valueRulesValidationMessage, } from "./formly-utils"; +import { ValueRuleSet } from "../../workspace/types/custom-json-schema.interface"; import { Subject } from "rxjs"; import { FORM_DEBOUNCE_TIME_MS } from "../../workspace/service/execute-workflow/execute-workflow.service"; import { PortSchema } from "../../workspace/types/workflow-compiling.interface"; @@ -206,3 +210,104 @@ describe("createOutputFormChangeEventStream", () => { expect(modelCheck).toHaveBeenCalledTimes(2); }); }); + +describe("valueRules", () => { + // the shape the sklearn trainers emit: one branch per hyperparameter, keyed on the + // `parameter` chosen beside the value in the same row + const rules: ValueRuleSet = { + allOf: [ + { if: { parameter: { valEnum: ["C"] } }, then: { type: "number", examples: ["1.0"] } }, + { if: { parameter: { valEnum: ["degree"] } }, then: { type: "integer", examples: ["3"] } }, + // an accepted set carries no example: it already names every value worth offering, and + // the estimator's own default leads + { + if: { parameter: { valEnum: ["kernel"] } }, + then: { enum: ["rbf", "linear", "poly", "sigmoid", "precomputed"] }, + }, + ], + }; + + const rowField = (row: unknown): FormlyFieldConfig => ({ parent: { model: row } }) as FormlyFieldConfig; + const control = (value: unknown) => ({ value }) as any; + const check = (parameter: string, value: unknown) => + createValueRulesValidator(rules)(control(value), rowField({ parameter })); + + describe("matchingValueRule", () => { + it("selects the branch the sibling's value names", () => { + expect(matchingValueRule(rules, { parameter: "kernel" })?.enum).toEqual([ + "rbf", + "linear", + "poly", + "sigmoid", + "precomputed", + ]); + expect(matchingValueRule(rules, { parameter: "degree" })?.type).toBe("integer"); + }); + + it("selects nothing when the sibling holds a value no branch names", () => { + expect(matchingValueRule(rules, { parameter: "metric_params" })).toBeUndefined(); + }); + + it("selects nothing before the row has a sibling value at all", () => { + expect(matchingValueRule(rules, {})).toBeUndefined(); + expect(matchingValueRule(rules, undefined)).toBeUndefined(); + expect(matchingValueRule(undefined, { parameter: "C" })).toBeUndefined(); + }); + }); + + describe("createValueRulesValidator", () => { + it("accepts a value the chosen parameter's set contains", () => { + expect(check("kernel", "rbf")).toBe(true); + }); + + it("rejects a value outside that set, including one of another parameter's", () => { + expect(check("kernel", "1")).toBe(false); + expect(check("kernel", "uniform")).toBe(false); + }); + + it("holds a numeric parameter to a number", () => { + expect(check("C", "1.0")).toBe(true); + expect(check("C", "-2.5e3")).toBe(true); + expect(check("C", "abc")).toBe(false); + }); + + it("holds a whole-number parameter to a whole number", () => { + expect(check("degree", "3")).toBe(true); + expect(check("degree", "-1")).toBe(true); + // int() raises on this, so the form should not let it reach the operator + expect(check("degree", "1.5")).toBe(false); + }); + + it("leaves emptiness to the required rule rather than answering twice", () => { + expect(check("C", "")).toBe(true); + expect(check("C", null)).toBe(true); + expect(check("kernel", undefined)).toBe(true); + }); + + it("accepts anything for a parameter no branch constrains", () => { + expect(check("metric_params", "whatever")).toBe(true); + }); + + it("re-judges the same value when the row switches parameter", () => { + // a value typed for one parameter is usually wrong for the next, and stays visible + expect(check("C", "1.0")).toBe(true); + expect(check("kernel", "1.0")).toBe(false); + }); + }); + + describe("valueRulesValidationMessage", () => { + const field = (parameter: string): FormlyFieldConfig => + ({ props: { valueRules: rules }, parent: { model: { parameter } } }) as FormlyFieldConfig; + + it("names the accepted values when there is a set", () => { + expect(valueRulesValidationMessage(null, field("kernel"))).toBe( + "must be one of rbf, linear, poly, sigmoid, precomputed" + ); + }); + + it("distinguishes a whole number from a number", () => { + expect(valueRulesValidationMessage(null, field("degree"))).toBe("must be a whole number"); + expect(valueRulesValidationMessage(null, field("C"))).toBe("must be a number"); + }); + }); +}); diff --git a/frontend/src/app/common/formly/formly-utils.ts b/frontend/src/app/common/formly/formly-utils.ts index cb80abe2bd5..9bcbc4fbc29 100644 --- a/frontend/src/app/common/formly/formly-utils.ts +++ b/frontend/src/app/common/formly/formly-utils.ts @@ -23,8 +23,9 @@ import { isDefined } from "../util/predicate"; import { Observable } from "rxjs"; import { FORM_DEBOUNCE_TIME_MS } from "../../workspace/service/execute-workflow/execute-workflow.service"; import { debounceTime, distinctUntilChanged, filter, share } from "rxjs/operators"; -import { HideType } from "../../workspace/types/custom-json-schema.interface"; +import { HideType, ValueRuleSet } from "../../workspace/types/custom-json-schema.interface"; import { PortSchema } from "../../workspace/types/workflow-compiling.interface"; +import { AbstractControl } from "@angular/forms"; export function getFieldByName(fieldName: string, fields: FormlyFieldConfig[]): FormlyFieldConfig | undefined { return fields.filter((field, _, __) => field.key === fieldName)[0]; @@ -39,6 +40,69 @@ export function setHideExpression(toggleHidden: string[], fields: FormlyFieldCon }); } +type ValueRule = ValueRuleSet["allOf"][number]["then"]; + +/** + * The one branch of `valueRules` that the row's current contents select, or undefined where + * none does. A branch names its sibling fields and the values of theirs it applies to, so the + * row model is what decides; `field.parent.model` is that row for an array item and the + * operator itself for a top-level field. + */ +export function matchingValueRule(rules: ValueRuleSet | undefined, rowModel: any): ValueRule | undefined { + if (!isDefined(rules) || !isDefined(rowModel)) { + return undefined; + } + return rules.allOf.find(branch => + Object.entries(branch.if).every(([sibling, condition]) => (condition.valEnum ?? []).includes(rowModel[sibling])) + )?.then; +} + +/** + * Validator holding a field to whichever branch of `valueRules` currently applies. + * + * An empty value passes: whether emptiness is allowed is `required`'s business, and a field + * that answers twice would report the wrong thing once. The numeric branches accept what + * JavaScript reads as a number, which is slightly narrower than the Python converters on the + * other end (they take `1_000` and `inf`); erring narrow here would be wrong for a field whose + * accepted set is open, but these two are bounded and the values it turns away are ones no one + * types into a hyperparameter. + */ +export function createValueRulesValidator(rules: ValueRuleSet) { + return (control: AbstractControl, field: FormlyFieldConfig): boolean => { + const rule = matchingValueRule(rules, field?.parent?.model); + if (!isDefined(rule)) { + return true; + } + const value = control.value; + if (value === null || value === undefined || value === "") { + return true; + } + const text = String(value).trim(); + if (isDefined(rule.enum)) { + return rule.enum.includes(text); + } + if (rule.type === "integer") { + return /^[-+]?\d+$/.test(text); + } + if (rule.type === "number") { + return text.length > 0 && Number.isFinite(Number(text)); + } + return true; + }; +} + +/** Says what the field will take, naming the branch rather than the rule that rejected it. */ +export function valueRulesValidationMessage(_err: unknown, field: FormlyFieldConfig): string { + const rule = matchingValueRule(field?.props?.valueRules, field?.parent?.model); + if (isDefined(rule?.enum)) { + return `must be one of ${rule.enum.join(", ")}`; + } + if (rule?.type === "integer") { + return "must be a whole number"; + } + return "must be a number"; +} + /* Factory function to make functions that hide expressions for a particular field */ export function createShouldHideFieldFunc( hideTarget: string, diff --git a/frontend/src/app/workspace/component/constrained-value/constrained-value.component.ts b/frontend/src/app/workspace/component/constrained-value/constrained-value.component.ts new file mode 100644 index 00000000000..87e305458e8 --- /dev/null +++ b/frontend/src/app/workspace/component/constrained-value/constrained-value.component.ts @@ -0,0 +1,95 @@ +/** + * 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. + */ + +import { ChangeDetectionStrategy, Component } from "@angular/core"; +import { CommonModule } from "@angular/common"; +import { FormsModule } from "@angular/forms"; +import { FieldType, FieldTypeConfig, FormlyModule } from "@ngx-formly/core"; +import { NzInputModule } from "ng-zorro-antd/input"; +import { NzSelectModule } from "ng-zorro-antd/select"; +import { matchingValueRule } from "../../../common/formly/formly-utils"; + +/** + * A field whose accepted values depend on what a sibling field holds: a chosen-from-a-set + * parameter renders as a dropdown, a numeric one as a number input, and anything the rules do + * not cover stays a plain text box. + * + * The value stays a string whichever control is showing. Operators that read one of these put + * the text through a converter of their own, so handing them a JSON number instead would only + * move the coercion somewhere less visible. + */ +@Component({ + selector: "texera-constrained-value", + standalone: true, + imports: [CommonModule, FormsModule, FormlyModule, NzInputModule, NzSelectModule], + changeDetection: ChangeDetectionStrategy.Default, + template: ` + + + + + + + + `, +}) +export class ConstrainedValueComponent extends FieldType { + /** The branch of the rules that the sibling's current value selects, if any. */ + private get rule() { + return matchingValueRule(this.props.valueRules, this.field?.parent?.model); + } + + get acceptedValues(): ReadonlyArray { + return this.rule?.enum ?? []; + } + + /** A number input where the rules call for a number, so a keyboard offers digits and the + * browser refuses most of what the converter would reject. + */ + get inputType(): string { + return this.rule?.type === undefined ? "text" : "number"; + } + + get current(): string { + return this.formControl.value ?? ""; + } + + /** Writes the control as a string whatever the control was. `nz-select` clears to null and a + * number input yields a number once its value parses, and both reach an operator expecting + * text. + */ + write(raw: unknown): void { + this.formControl.setValue(raw === null || raw === undefined ? "" : String(raw)); + this.formControl.markAsDirty(); + this.formControl.markAsTouched(); + } +} diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts index beedbabd90f..69fb08a283e 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts @@ -42,8 +42,10 @@ import { WorkflowCompilingService } from "../../../service/compile-workflow/work import { createOutputFormChangeEventStream, createShouldHideFieldFunc, + createValueRulesValidator, setChildTypeDependency, setHideExpression, + valueRulesValidationMessage, } from "src/app/common/formly/formly-utils"; import { TYPE_CASTING_OPERATOR_TYPE, @@ -856,6 +858,24 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On }; } + // a field whose accepted values follow a sibling's: give it the control those values + // call for, and hold it to them before the workflow can be run + if (isDefined(mapSource.valueRules)) { + const valueRules = mapSource.valueRules; + mappedField.type = "constrainedvalue"; + // written into the existing object rather than over it: `props` and `templateOptions` + // are two names for one object, and replacing it leaves them pointing at different ones + mappedField.props = mappedField.props ?? {}; + (mappedField.props as Record).valueRules = valueRules; + mappedField.validators = { + ...mappedField.validators, + valueRules: { + expression: createValueRulesValidator(valueRules), + message: valueRulesValidationMessage, + }, + }; + } + // if the title is fileName, then change it to custom autocomplete input template if (mappedField.key === "fileName") { mappedField.type = "inputautocomplete"; diff --git a/frontend/src/app/workspace/types/custom-json-schema.interface.ts b/frontend/src/app/workspace/types/custom-json-schema.interface.ts index 50edb681618..d9ca521137f 100644 --- a/frontend/src/app/workspace/types/custom-json-schema.interface.ts +++ b/frontend/src/app/workspace/types/custom-json-schema.interface.ts @@ -46,6 +46,31 @@ export type AttributeTypeRuleSchema = Readonly<{ [key: string]: AttributeTypeRuleSet; }>; +/** + * What one field may hold, given what a sibling holds. Borrows `attributeTypeRules`' grammar + * and, like it, sits under a key of Texera's own rather than as a JSON-Schema `allOf`: the + * form builder merges the members of an `allOf` into a single field, which would leave one + * control carrying every branch's constraints at once. + */ +export type ValueRuleSet = Readonly<{ + allOf: ReadonlyArray<{ + if: { + [siblingField: string]: { + valEnum?: string[]; + }; + }; + then: { + // the accepted set, where the value is chosen from one + enum?: ReadonlyArray; + // otherwise how the value is read, in JSON Schema's names + type?: "number" | "integer"; + // a value that is accepted, for a reader that has to supply one; the form does not + // render it, the same as everywhere else `examples` is declared + examples?: ReadonlyArray; + }; + }>; +}>; + export interface CustomJSONSchema7 extends JSONSchema7 { propertyOrder?: number; properties?: { @@ -57,6 +82,7 @@ export interface CustomJSONSchema7 extends JSONSchema7 { autofill?: "attributeName" | "attributeNameList"; autofillAttributeOnPort?: number; attributeTypeRules?: AttributeTypeRuleSchema; + valueRules?: ValueRuleSet; "enable-presets"?: boolean; // include property in schema of preset From d3b579b015e93b36fa74a82d4a23d9ff0e475320 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Mon, 24 Aug 2026 15:32:49 -0700 Subject: [PATCH 2/7] feat(operator): let a hyperparameter accept a choice between a word and a number gamma takes either of the words scale and auto or a number, and scale is what the estimator defaults to. Declared as float it could take neither word, so the mode most users want was unreachable; declared as str it would lose every number instead. No converter of a name covers it. It now names a lambda that hands the two words through and puts everything else past float(), and its rule carries a pattern in place of a type, no type being able to describe a choice between a set and a number. The pattern is written from what that converter takes, with digits spelled [0-9] rather than \d so Python, the browser and the JVM read it alike. Also adds the component spec that should have come with the control itself. Closes #7945 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../SklearnAdvancedSVCParameters.java | 23 ++- .../SklearnAdvancedSVRParameters.java | 14 +- .../base/SklearnAdvancedBaseDesc.scala | 11 +- .../base/SklearnAdvancedBaseDescSpec.scala | 13 +- .../app/common/formly/formly-utils.spec.ts | 21 +++ .../src/app/common/formly/formly-utils.ts | 12 ++ .../constrained-value.component.spec.ts | 136 ++++++++++++++++++ .../types/custom-json-schema.interface.ts | 3 + 8 files changed, 221 insertions(+), 12 deletions(-) create mode 100644 frontend/src/app/workspace/component/constrained-value/constrained-value.component.spec.ts diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java index 2a3969b31e2..b24b979dfac 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java @@ -24,9 +24,26 @@ public enum SklearnAdvancedSVCParameters implements ParamClass { C("C", "float", "1.0"), kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), - // SVC's own default for gamma is "scale", which float() cannot convert, so there is no - // example to offer until the declared converter can carry one. - gamma("gamma", "float", ""), + // gamma takes either of two words or a number, so no converter of a name covers it. This + // one hands the words through and puts everything else past float(), which is also what + // decides that a value is not a number at all. + // + // The pattern below is what that converter takes. Digits are [0-9] rather than \d so the + // three engines it runs through read it alike: Python's float() also takes non-ASCII + // decimal digits, but JavaScript's \d does not match them either, so the browser turns + // them away whichever spelling is used. It is loose in one direction, letting a negative + // through for the estimator to refuse, because excluding the sign would also exclude -0.0, + // which the estimator takes, and turning away a value that works is the worse mistake. + gamma( + "gamma", + "(lambda value: value.strip() if value.strip() in (\"scale\", \"auto\") else float(value))", + "scale") { + @Override + public String getPattern() { + return "^\\s*(?:scale|auto|[-+]?(?:(?:[0-9]+(?:_[0-9]+)*)?\\.(?:[0-9]+(?:_[0-9]+)*)" + + "|(?:[0-9]+(?:_[0-9]+)*)\\.?)(?:[eE][-+]?[0-9]+(?:_[0-9]+)*)?)\\s*$"; + } + }, degree("degree", "int", "3"), coef0("coef0", "float", "0.0"), tol("tol", "float", "0.001"), diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java index 760545579d3..757c38f1b69 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java @@ -24,9 +24,17 @@ public enum SklearnAdvancedSVRParameters implements ParamClass { C("C", "float", "1.0"), kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), - // SVR's own default for gamma is "scale", which float() cannot convert, so there is no - // example to offer until the declared converter can carry one. - gamma("gamma", "float", ""), + // Same converter and shape as SVC's gamma -- see there for why each is spelled this way. + gamma( + "gamma", + "(lambda value: value.strip() if value.strip() in (\"scale\", \"auto\") else float(value))", + "scale") { + @Override + public String getPattern() { + return "^\\s*(?:scale|auto|[-+]?(?:(?:[0-9]+(?:_[0-9]+)*)?\\.(?:[0-9]+(?:_[0-9]+)*)" + + "|(?:[0-9]+(?:_[0-9]+)*)\\.?)(?:[eE][-+]?[0-9]+(?:_[0-9]+)*)?)\\s*$"; + } + }, degree("degree", "int", "3"), coef0("coef0", "float", "0.0"), tol("tol", "float", "0.001"), 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 038bf83e541..f346e6b42f7 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 @@ -65,6 +65,12 @@ trait ParamClass { * converter already constrains. */ def getAllowedValues: Array[String] + + /** The shape the value takes, for a parameter that accepts neither a fixed set nor a plain + * number but a choice between them. Empty for every parameter one of the other two + * describes, which is most of them. + */ + def getPattern: String = "" } abstract class SklearnMLOperatorDescriptor[T <: ParamClass] @@ -121,7 +127,10 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] // value a sweep should try, so an example alongside it would only repeat one of them. param.getAllowedValues.foreach(outcome.withArray("enum").add) } else { - valueTypeOf(param).foreach(outcome.put("type", _)) + // A pattern is what a parameter offering a choice between a set and a number has + // instead, so it stands in for the type rather than joining it. + if (param.getPattern.nonEmpty) outcome.put("pattern", param.getPattern) + else valueTypeOf(param).foreach(outcome.put("type", _)) if (param.getSampleValue.nonEmpty) outcome.withArray("examples").add(param.getSampleValue) } 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 170928de5cd..6f1496e4724 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 @@ -195,12 +195,15 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { ruleFor(rules, "degree").path("type").asText() shouldBe "integer" } - it should "constrain a parameter it has no example for" in { - // SVC's own default for gamma is a word float() cannot convert, so the rule carries the - // constraint without an example. The two are separate statements and one can stand alone. + it should "describe a parameter choosing between a set and a number with a pattern" in { + // gamma takes either of two words or a number, which no type names, so the rule carries a + // pattern in place of one. Its example is the estimator's own default, a word. val gamma = ruleFor(valueRulesOf(classOf[SklearnAdvancedSVCTrainerOpDesc]), "gamma") - gamma.path("type").asText() shouldBe "number" - gamma.has("examples") shouldBe false + gamma.has("type") shouldBe false + gamma.path("examples").path(0).asText() shouldBe "scale" + val pattern = gamma.path("pattern").asText() + Seq("scale", "auto", "0.1", "1e-3", " 1 ").foreach(v => v.matches(pattern) shouldBe true) + Seq("abc", "", "scaleauto", "1.2.3").foreach(v => v.matches(pattern) shouldBe false) } it should "state a rule for every parameter whose converter says anything about it" in { diff --git a/frontend/src/app/common/formly/formly-utils.spec.ts b/frontend/src/app/common/formly/formly-utils.spec.ts index 6f37620a74f..c6a8936392b 100644 --- a/frontend/src/app/common/formly/formly-utils.spec.ts +++ b/frontend/src/app/common/formly/formly-utils.spec.ts @@ -218,6 +218,11 @@ describe("valueRules", () => { allOf: [ { if: { parameter: { valEnum: ["C"] } }, then: { type: "number", examples: ["1.0"] } }, { if: { parameter: { valEnum: ["degree"] } }, then: { type: "integer", examples: ["3"] } }, + // gamma takes either of two words or a number, which no type names + { + if: { parameter: { valEnum: ["gamma"] } }, + then: { pattern: "^\\s*(?:scale|auto|[-+]?[0-9]*\\.?[0-9]+)\\s*$", examples: ["scale"] }, + }, // an accepted set carries no example: it already names every value worth offering, and // the estimator's own default leads { @@ -288,6 +293,16 @@ describe("valueRules", () => { expect(check("metric_params", "whatever")).toBe(true); }); + it("holds a parameter with a pattern to the shape it declares", () => { + // both halves of the union it describes + expect(check("gamma", "scale")).toBe(true); + expect(check("gamma", "auto")).toBe(true); + expect(check("gamma", "0.1")).toBe(true); + expect(check("gamma", " 1 ")).toBe(true); + expect(check("gamma", "abc")).toBe(false); + expect(check("gamma", "scaleauto")).toBe(false); + }); + it("re-judges the same value when the row switches parameter", () => { // a value typed for one parameter is usually wrong for the next, and stays visible expect(check("C", "1.0")).toBe(true); @@ -309,5 +324,11 @@ describe("valueRules", () => { expect(valueRulesValidationMessage(null, field("degree"))).toBe("must be a whole number"); expect(valueRulesValidationMessage(null, field("C"))).toBe("must be a number"); }); + + it("points at a working value where a pattern is what the branch declares", () => { + expect(valueRulesValidationMessage(null, field("gamma"))).toBe( + "is not a value this parameter takes, such as scale" + ); + }); }); }); diff --git a/frontend/src/app/common/formly/formly-utils.ts b/frontend/src/app/common/formly/formly-utils.ts index 9bcbc4fbc29..2a47df651fc 100644 --- a/frontend/src/app/common/formly/formly-utils.ts +++ b/frontend/src/app/common/formly/formly-utils.ts @@ -81,6 +81,11 @@ export function createValueRulesValidator(rules: ValueRuleSet) { if (isDefined(rule.enum)) { return rule.enum.includes(text); } + if (isDefined(rule.pattern)) { + // anchored the way the declaration writes it, so the same expression judges the value + // here, in the operator's own tests and in the generated Python + return new RegExp(rule.pattern).test(String(value)); + } if (rule.type === "integer") { return /^[-+]?\d+$/.test(text); } @@ -97,6 +102,13 @@ export function valueRulesValidationMessage(_err: unknown, field: FormlyFieldCon if (isDefined(rule?.enum)) { return `must be one of ${rule.enum.join(", ")}`; } + if (isDefined(rule?.pattern)) { + // a pattern covers shapes no short phrase names, so point at a value that works instead + const example = rule.examples?.[0]; + return isDefined(example) + ? `is not a value this parameter takes, such as ${example}` + : "is not a value this parameter takes"; + } if (rule?.type === "integer") { return "must be a whole number"; } diff --git a/frontend/src/app/workspace/component/constrained-value/constrained-value.component.spec.ts b/frontend/src/app/workspace/component/constrained-value/constrained-value.component.spec.ts new file mode 100644 index 00000000000..ea5221a0734 --- /dev/null +++ b/frontend/src/app/workspace/component/constrained-value/constrained-value.component.spec.ts @@ -0,0 +1,136 @@ +/** + * 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. + */ + +import { FormControl } from "@angular/forms"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; +import { FormlyFieldConfig } from "@ngx-formly/core"; +import { NoopAnimationsModule } from "@angular/platform-browser/animations"; +import { ValueRuleSet } from "../../types/custom-json-schema.interface"; +import { ConstrainedValueComponent } from "./constrained-value.component"; + +describe("ConstrainedValueComponent", () => { + // one branch of each shape a rule can take, keyed on the sibling `parameter` + const rules: ValueRuleSet = { + allOf: [ + { + if: { parameter: { valEnum: ["kernel"] } }, + then: { enum: ["rbf", "linear", "poly", "sigmoid", "precomputed"] }, + }, + { if: { parameter: { valEnum: ["C"] } }, then: { type: "number", examples: ["1.0"] } }, + { if: { parameter: { valEnum: ["degree"] } }, then: { type: "integer", examples: ["3"] } }, + { + if: { parameter: { valEnum: ["gamma"] } }, + then: { pattern: "^\\s*(?:scale|auto|[-+]?[0-9]*\\.?[0-9]+)\\s*$", examples: ["scale"] }, + }, + ], + }; + + let fixture: ComponentFixture; + let component: ConstrainedValueComponent; + + /** Puts the component in the row a real `paraList` item would give it. */ + const showFor = (parameter: string, value: string = ""): FormControl => { + const formControl = new FormControl(value); + (component as any).field = { + key: "value", + formControl, + props: { valueRules: rules }, + parent: { model: { parameter } }, + } as FormlyFieldConfig; + fixture.detectChanges(); + return formControl; + }; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ConstrainedValueComponent, NoopAnimationsModule], + }).compileComponents(); + + fixture = TestBed.createComponent(ConstrainedValueComponent); + component = fixture.componentInstance; + }); + + it("offers the accepted values as a dropdown when the parameter is chosen from a set", () => { + showFor("kernel"); + expect(component.acceptedValues).toEqual(["rbf", "linear", "poly", "sigmoid", "precomputed"]); + expect(fixture.debugElement.query(By.css("nz-select"))).not.toBeNull(); + // nz-select carries a hidden input of its own, so look for ours rather than for any + expect(fixture.debugElement.query(By.css("input[nz-input]"))).toBeNull(); + }); + + it("gives a numeric parameter a number input instead", () => { + showFor("C"); + expect(component.acceptedValues).toEqual([]); + expect(component.inputType).toBe("number"); + expect(fixture.debugElement.query(By.css("nz-select"))).toBeNull(); + expect(fixture.debugElement.query(By.css("input[nz-input]")).nativeElement.type).toBe("number"); + }); + + it("keeps a parameter described by a pattern on a text input, since it may hold a word", () => { + showFor("gamma"); + expect(component.inputType).toBe("text"); + expect(fixture.debugElement.query(By.css("input[nz-input]")).nativeElement.type).toBe("text"); + }); + + it("leaves a parameter no branch names as a plain text box", () => { + showFor("metric_params"); + expect(component.acceptedValues).toEqual([]); + expect(component.inputType).toBe("text"); + }); + + it("follows the row when the parameter beside it changes", () => { + showFor("kernel"); + expect(fixture.debugElement.query(By.css("nz-select"))).not.toBeNull(); + + (component as any).field.parent.model.parameter = "C"; + fixture.detectChanges(); + + expect(fixture.debugElement.query(By.css("nz-select"))).toBeNull(); + expect(fixture.debugElement.query(By.css("input[nz-input]")).nativeElement.type).toBe("number"); + }); + + it("writes the control as a string whichever control produced the value", () => { + const control = showFor("C"); + // a number input yields a number once its text parses + component.write(0.1); + expect(control.value).toBe("0.1"); + expect(typeof control.value).toBe("string"); + }); + + it("writes an empty string when a dropdown is cleared", () => { + const control = showFor("kernel", "rbf"); + component.write(null); + expect(control.value).toBe(""); + }); + + it("marks the control touched so the error shows on the first bad value", () => { + const control = showFor("C"); + expect(control.touched).toBe(false); + component.write("abc"); + expect(control.dirty).toBe(true); + expect(control.touched).toBe(true); + }); + + it("reads an unset control as an empty string rather than null", () => { + const control = showFor("C"); + control.setValue(null); + expect(component.current).toBe(""); + }); +}); diff --git a/frontend/src/app/workspace/types/custom-json-schema.interface.ts b/frontend/src/app/workspace/types/custom-json-schema.interface.ts index d9ca521137f..0a784610c0f 100644 --- a/frontend/src/app/workspace/types/custom-json-schema.interface.ts +++ b/frontend/src/app/workspace/types/custom-json-schema.interface.ts @@ -64,6 +64,9 @@ export type ValueRuleSet = Readonly<{ enum?: ReadonlyArray; // otherwise how the value is read, in JSON Schema's names type?: "number" | "integer"; + // or, where the value is a choice between a set and a number and no type names it, + // the shape it takes + pattern?: string; // a value that is accepted, for a reader that has to supply one; the form does not // render it, the same as everywhere else `examples` is declared examples?: ReadonlyArray; From 194e817d474ebe2e8b963d19714b6bbcf19bf6ed Mon Sep 17 00:00:00 2001 From: kary zheng Date: Mon, 24 Aug 2026 16:33:02 -0700 Subject: [PATCH 3/7] test(operator): cover the paths a malformed schema and a stub descriptor take customizeJsonSchema passes over a schema it cannot find the row in, and writes nothing for a descriptor whose type argument is not an enum. Neither arises from a schema the generator produced, so every existing case went down the one path that works and left the guards unexercised. Also covers the two message branches a pattern rule reaches: one with no example to point at, and one where the row moved on and no branch applies any more. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../base/SklearnAdvancedBaseDescSpec.scala | 33 +++++++++++++++++++ .../app/common/formly/formly-utils.spec.ts | 14 ++++++++ 2 files changed, 47 insertions(+) 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 6f1496e4724..8e44b3b3e1c 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 @@ -20,7 +20,9 @@ package org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base import com.fasterxml.jackson.databind.JsonNode +import com.fasterxml.jackson.databind.node.ObjectNode import org.apache.texera.amber.core.tuple.AttributeType +import org.apache.texera.amber.util.JSONUtils.objectMapper import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.KNNTrainer.SklearnAdvancedKNNClassifierTrainerOpDesc import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.SVCTrainer.SklearnAdvancedSVCTrainerOpDesc import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorMetadataGenerator} @@ -221,6 +223,37 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { covered shouldBe Set("n_neighbors", "p", "weights", "algorithm", "leaf_size", "metric") } + it should "leave a schema it cannot find the row in alone" in { + // The three shapes that make the rules unwritable. None arises from a schema the generator + // produced, so the point is that each is passed over rather than thrown on. + val svc = new SklearnAdvancedSVCTrainerOpDesc + Seq( + objectMapper.createObjectNode(), + objectMapper.readTree("""{"properties": {"paraList": {"items": {}}}}"""), + objectMapper.readTree( + """{"properties": {"paraList": {"items": {"$ref": "#/definitions/Row"}}}, + "definitions": {"Row": {"properties": {"value": "not an object"}}}}""" + ) + ).foreach { schema => + val node = schema.asInstanceOf[ObjectNode] + noException should be thrownBy svc.customizeJsonSchema(node) + node.findValue("valueRules") shouldBe null + } + } + + it should "write no rules for a descriptor whose type argument is not an enum" in { + // TestParam is a class standing in for one, so there are no constants to read. A real + // operator always binds an enum; this is the path a test stub takes. + val schema = objectMapper + .readTree( + """{"properties": {"paraList": {"items": {"$ref": "#/definitions/Row"}}}, + "definitions": {"Row": {"properties": {"value": {"type": "string"}}}}}""" + ) + .asInstanceOf[ObjectNode] + new TestSklearnMLOp().customizeJsonSchema(schema) + schema.findValue("valueRules") shouldBe null + } + "HyperParameters" should "require whichever of the two inputs the row actually uses" in { val schema = OperatorMetadataGenerator.generateOperatorJsonSchema(classOf[SklearnAdvancedSVCTrainerOpDesc]) diff --git a/frontend/src/app/common/formly/formly-utils.spec.ts b/frontend/src/app/common/formly/formly-utils.spec.ts index c6a8936392b..445f8fcca59 100644 --- a/frontend/src/app/common/formly/formly-utils.spec.ts +++ b/frontend/src/app/common/formly/formly-utils.spec.ts @@ -330,5 +330,19 @@ describe("valueRules", () => { "is not a value this parameter takes, such as scale" ); }); + + it("says only what it knows when a pattern branch offers no example", () => { + const noExample: ValueRuleSet = { + allOf: [{ if: { parameter: { valEnum: ["gamma"] } }, then: { pattern: "^scale$" } }], + }; + const bare = { props: { valueRules: noExample }, parent: { model: { parameter: "gamma" } } }; + expect(valueRulesValidationMessage(null, bare as FormlyFieldConfig)).toBe("is not a value this parameter takes"); + }); + + it("falls back to the numeric wording when no branch applies at all", () => { + // reached when the row's parameter changes between the check failing and the message + // being read, so the message must still say something rather than throw + expect(valueRulesValidationMessage(null, field("metric_params"))).toBe("must be a number"); + }); }); }); From b2f775d25e0db7e9bc0ac60146fa6b986ab92eef Mon Sep 17 00:00:00 2001 From: kary zheng Date: Mon, 24 Aug 2026 17:13:14 -0700 Subject: [PATCH 4/7] feat(operator): hold a numeric hyperparameter to the range its estimator accepts Saying a value is read as a number leaves out the half of the constraint that actually bites: C is refused at zero, n_neighbors starts at one, and a negative anything is refused nearly everywhere. Each was accepted by the editor and raised from inside scikit-learn once the run started. Eleven parameters now declare the low end of their range as the estimator states it, open or closed, read from the same Interval the accepted sets came from. coef0 declares none, being the one bounded by nothing at either end, and max_iter's is -1 rather than zero because that is its own value for no limit. Checked against the operator over the boundary values rather than assumed: the form's verdict and the converter-then-sklearn verdict agree on every one, so nothing that runs is turned away and nothing turned away would have run. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../SklearnAdvancedKNNParameters.java | 8 +++-- .../SklearnAdvancedSVCParameters.java | 9 +++-- .../SklearnAdvancedSVRParameters.java | 16 +++++---- .../base/SklearnAdvancedBaseDesc.scala | 27 ++++++++++++++- .../base/SklearnAdvancedBaseDescSpec.scala | 19 +++++++++++ .../app/common/formly/formly-utils.spec.ts | 33 +++++++++++++++---- .../src/app/common/formly/formly-utils.ts | 28 ++++++++++++---- .../types/custom-json-schema.interface.ts | 5 ++- 8 files changed, 116 insertions(+), 29 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 7456e5516e3..4fb30c9cd8e 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 @@ -22,11 +22,13 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedKNNParameters implements ParamClass { - n_neighbors("n_neighbors", "int", "5"), - p("p", "int", "2"), + // Bounds are scikit-learn's own: a neighbour count and a leaf size start at one, and the + // Minkowski power is open at zero. + n_neighbors("n_neighbors", "int", "5") { @Override public String getMinimum() { return ">=1"; } }, + p("p", "int", "2") { @Override public String getMinimum() { return ">0"; } }, weights("weights", "str", "", "uniform", "distance"), algorithm("algorithm", "str", "", "auto", "ball_tree", "kd_tree", "brute"), - leaf_size("leaf_size", "int", "30"), + leaf_size("leaf_size", "int", "30") { @Override public String getMinimum() { return ">=1"; } }, // The last two have no example and no accepted set, because neither has a value worth // naming under the converter it declares: the metrics are words while int() takes only // numbers, and metric_params is a mapping that str() cannot produce. diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java index b24b979dfac..5bda1cf0ecb 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVCTrainer/SklearnAdvancedSVCParameters.java @@ -22,7 +22,9 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedSVCParameters implements ParamClass { - C("C", "float", "1.0"), + // Bounds are scikit-learn's own, whose ranges are open at zero for the two below and + // closed for degree. + C("C", "float", "1.0") { @Override public String getMinimum() { return ">0"; } }, kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), // gamma takes either of two words or a number, so no converter of a name covers it. This // one hands the words through and puts everything else past float(), which is also what @@ -44,9 +46,10 @@ public String getPattern() { + "|(?:[0-9]+(?:_[0-9]+)*)\\.?)(?:[eE][-+]?[0-9]+(?:_[0-9]+)*)?)\\s*$"; } }, - degree("degree", "int", "3"), + degree("degree", "int", "3") { @Override public String getMinimum() { return ">=0"; } }, + // coef0 is the one parameter here with no bound at either end. coef0("coef0", "float", "0.0"), - tol("tol", "float", "0.001"), + tol("tol", "float", "0.001") { @Override public String getMinimum() { return ">0"; } }, probability("probability", "(lambda value: value.lower() == \"true\")", "", "false", "true"); private final String name; diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java index 757c38f1b69..c204f757453 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/SVRTrainer/SklearnAdvancedSVRParameters.java @@ -22,7 +22,8 @@ import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.ParamClass; public enum SklearnAdvancedSVRParameters implements ParamClass { - C("C", "float", "1.0"), + // Bounds are scikit-learn's own; see SVC for the shared ones. + C("C", "float", "1.0") { @Override public String getMinimum() { return ">0"; } }, kernel("kernel", "str", "", "rbf", "linear", "poly", "sigmoid", "precomputed"), // Same converter and shape as SVC's gamma -- see there for why each is spelled this way. gamma( @@ -35,15 +36,16 @@ public String getPattern() { + "|(?:[0-9]+(?:_[0-9]+)*)\\.?)(?:[eE][-+]?[0-9]+(?:_[0-9]+)*)?)\\s*$"; } }, - degree("degree", "int", "3"), + degree("degree", "int", "3") { @Override public String getMinimum() { return ">=0"; } }, coef0("coef0", "float", "0.0"), - tol("tol", "float", "0.001"), + tol("tol", "float", "0.001") { @Override public String getMinimum() { return ">0"; } }, probability("shrinking", "(lambda value: value.lower() == \"true\")", "", "true", "false"), verbose("verbose", "(lambda value: value.lower() == \"true\")", "", "false", "true"), - epsilon("epsilon", "float", "0.1"), - cache_size("cache_size", "int", "200"), - // -1 is SVR's own value for no iteration limit. - max_iter("max_iter", "int", "-1"); + epsilon("epsilon", "float", "0.1") { @Override public String getMinimum() { return ">=0"; } }, + cache_size("cache_size", "int", "200") { @Override public String getMinimum() { return ">0"; } }, + // -1 is SVR's own value for no iteration limit, which is also why the bound is -1 and not + // zero: the sentinel has to remain reachable. + max_iter("max_iter", "int", "-1") { @Override public String getMinimum() { return ">=-1"; } }; 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 f346e6b42f7..592ee04f451 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 @@ -39,6 +39,7 @@ import org.apache.texera.amber.pybuilder.PythonTemplateBuilder import org.apache.texera.amber.util.JSONUtils.objectMapper import com.fasterxml.jackson.databind.node.ObjectNode import java.lang.reflect.{ParameterizedType, Type} +import scala.util.Try /** * One hyperparameter a trainer offers. `getName` is the keyword argument passed to the @@ -71,6 +72,13 @@ trait ParamClass { * describes, which is most of them. */ def getPattern: String = "" + + /** How low the value may go, written the way the estimator's own range reads: `">0"` where + * zero itself is refused and `">=1"` where the bound is included. Empty for a parameter + * bounded by nothing, which a number alone cannot be told from one whose bound nobody + * looked up. + */ + def getMinimum: String = "" } abstract class SklearnMLOperatorDescriptor[T <: ParamClass] @@ -130,7 +138,13 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] // A pattern is what a parameter offering a choice between a set and a number has // instead, so it stands in for the type rather than joining it. if (param.getPattern.nonEmpty) outcome.put("pattern", param.getPattern) - else valueTypeOf(param).foreach(outcome.put("type", _)) + else + valueTypeOf(param).foreach { valueType => + outcome.put("type", valueType) + // A bound belongs to a value read as a number, so it rides with the type rather + // than standing on its own. + addMinimum(param.getMinimum, outcome) + } if (param.getSampleValue.nonEmpty) outcome.withArray("examples").add(param.getSampleValue) } @@ -148,6 +162,17 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] .set[ObjectNode]("allOf", branches) } + /** Puts a declared bound under the JSON Schema name for it, `>` and `>=` being the two forms + * an estimator's range takes at the low end. A bound spelled any other way is skipped + * rather than guessed at, since a wrong one turns away values that work. + */ + private def addMinimum(bound: String, outcome: ObjectNode): Unit = + if (bound.startsWith(">=")) numberOf(bound.drop(2)).foreach(outcome.put("minimum", _)) + else if (bound.startsWith(">")) + numberOf(bound.drop(1)).foreach(outcome.put("exclusiveMinimum", _)) + + private def numberOf(text: String): Option[Double] = Try(text.trim.toDouble).toOption + /** How the form should read a value with no fixed set of its own: from the callable the * parameter names, since that is what the emitted code puts the text through. A parameter * converted by anything else is left unconstrained rather than guessed at. 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 8e44b3b3e1c..7e794c070d9 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 @@ -25,6 +25,7 @@ import org.apache.texera.amber.core.tuple.AttributeType import org.apache.texera.amber.util.JSONUtils.objectMapper import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.KNNTrainer.SklearnAdvancedKNNClassifierTrainerOpDesc import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.SVCTrainer.SklearnAdvancedSVCTrainerOpDesc +import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.SVRTrainer.SklearnAdvancedSVRTrainerOpDesc import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorMetadataGenerator} import org.apache.texera.amber.pybuilder.PyStringTypes.EncodableString import org.scalatest.flatspec.AnyFlatSpec @@ -197,6 +198,24 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { ruleFor(rules, "degree").path("type").asText() shouldBe "integer" } + it should "carry the estimator's bound under whichever name matches its range" in { + val rules = valueRulesOf(classOf[SklearnAdvancedSVCTrainerOpDesc]) + // C's range is open at zero and degree's is closed, so the two take different names + ruleFor(rules, "C").path("exclusiveMinimum").asDouble() shouldBe 0.0 + ruleFor(rules, "C").has("minimum") shouldBe false + ruleFor(rules, "degree").path("minimum").asDouble() shouldBe 0.0 + ruleFor(rules, "degree").has("exclusiveMinimum") shouldBe false + // coef0 is bounded by nothing, so it gets neither rather than a made-up zero + ruleFor(rules, "coef0").has("minimum") shouldBe false + ruleFor(rules, "coef0").has("exclusiveMinimum") shouldBe false + } + + it should "keep a sentinel value reachable when it lies below the useful range" in { + // SVR's max_iter uses -1 for no limit, so the bound has to admit it + val rules = valueRulesOf(classOf[SklearnAdvancedSVRTrainerOpDesc]) + ruleFor(rules, "max_iter").path("minimum").asDouble() shouldBe -1.0 + } + it should "describe a parameter choosing between a set and a number with a pattern" in { // gamma takes either of two words or a number, which no type names, so the rule carries a // pattern in place of one. Its example is the estimator's own default, a word. diff --git a/frontend/src/app/common/formly/formly-utils.spec.ts b/frontend/src/app/common/formly/formly-utils.spec.ts index 445f8fcca59..13cafb5aeeb 100644 --- a/frontend/src/app/common/formly/formly-utils.spec.ts +++ b/frontend/src/app/common/formly/formly-utils.spec.ts @@ -216,8 +216,15 @@ describe("valueRules", () => { // `parameter` chosen beside the value in the same row const rules: ValueRuleSet = { allOf: [ - { if: { parameter: { valEnum: ["C"] } }, then: { type: "number", examples: ["1.0"] } }, - { if: { parameter: { valEnum: ["degree"] } }, then: { type: "integer", examples: ["3"] } }, + { + if: { parameter: { valEnum: ["C"] } }, + then: { type: "number", exclusiveMinimum: 0, examples: ["1.0"] }, + }, + { + if: { parameter: { valEnum: ["degree"] } }, + then: { type: "integer", minimum: 0, examples: ["3"] }, + }, + { if: { parameter: { valEnum: ["coef0"] } }, then: { type: "number", examples: ["0.0"] } }, // gamma takes either of two words or a number, which no type names { if: { parameter: { valEnum: ["gamma"] } }, @@ -272,15 +279,16 @@ describe("valueRules", () => { it("holds a numeric parameter to a number", () => { expect(check("C", "1.0")).toBe(true); - expect(check("C", "-2.5e3")).toBe(true); + // coef0 carries no bound, so it is where number-ness alone can be checked + expect(check("coef0", "-2.5e3")).toBe(true); expect(check("C", "abc")).toBe(false); }); it("holds a whole-number parameter to a whole number", () => { expect(check("degree", "3")).toBe(true); - expect(check("degree", "-1")).toBe(true); // int() raises on this, so the form should not let it reach the operator expect(check("degree", "1.5")).toBe(false); + expect(check("coef0", "-1")).toBe(true); }); it("leaves emptiness to the required rule rather than answering twice", () => { @@ -293,6 +301,16 @@ describe("valueRules", () => { expect(check("metric_params", "whatever")).toBe(true); }); + it("holds a value to the bound the estimator puts on it", () => { + // C is open at zero, degree is closed at it, and coef0 has no bound at all + expect(check("C", "0")).toBe(false); + expect(check("C", "-1")).toBe(false); + expect(check("C", "0.0001")).toBe(true); + expect(check("degree", "0")).toBe(true); + expect(check("degree", "-1")).toBe(false); + expect(check("coef0", "-100")).toBe(true); + }); + it("holds a parameter with a pattern to the shape it declares", () => { // both halves of the union it describes expect(check("gamma", "scale")).toBe(true); @@ -320,9 +338,10 @@ describe("valueRules", () => { ); }); - it("distinguishes a whole number from a number", () => { - expect(valueRulesValidationMessage(null, field("degree"))).toBe("must be a whole number"); - expect(valueRulesValidationMessage(null, field("C"))).toBe("must be a number"); + it("distinguishes a whole number from a number, and names the bound where there is one", () => { + expect(valueRulesValidationMessage(null, field("degree"))).toBe("must be a whole number of at least 0"); + expect(valueRulesValidationMessage(null, field("C"))).toBe("must be a number greater than 0"); + expect(valueRulesValidationMessage(null, field("coef0"))).toBe("must be a number"); }); it("points at a working value where a pattern is what the branch declares", () => { diff --git a/frontend/src/app/common/formly/formly-utils.ts b/frontend/src/app/common/formly/formly-utils.ts index 2a47df651fc..506cb11ca50 100644 --- a/frontend/src/app/common/formly/formly-utils.ts +++ b/frontend/src/app/common/formly/formly-utils.ts @@ -86,11 +86,21 @@ export function createValueRulesValidator(rules: ValueRuleSet) { // here, in the operator's own tests and in the generated Python return new RegExp(rule.pattern).test(String(value)); } - if (rule.type === "integer") { - return /^[-+]?\d+$/.test(text); + if (rule.type === "integer" && !/^[-+]?\d+$/.test(text)) { + return false; + } + if (rule.type === "number" && !(text.length > 0 && Number.isFinite(Number(text)))) { + return false; } - if (rule.type === "number") { - return text.length > 0 && Number.isFinite(Number(text)); + if (isDefined(rule.type)) { + // the estimator's own bound, which it would otherwise raise on after the run started + const value = Number(text); + if (isDefined(rule.minimum) && value < rule.minimum) { + return false; + } + if (isDefined(rule.exclusiveMinimum) && value <= rule.exclusiveMinimum) { + return false; + } } return true; }; @@ -109,10 +119,14 @@ export function valueRulesValidationMessage(_err: unknown, field: FormlyFieldCon ? `is not a value this parameter takes, such as ${example}` : "is not a value this parameter takes"; } - if (rule?.type === "integer") { - return "must be a whole number"; + const kind = rule?.type === "integer" ? "a whole number" : "a number"; + if (isDefined(rule?.minimum)) { + return `must be ${kind} of at least ${rule.minimum}`; + } + if (isDefined(rule?.exclusiveMinimum)) { + return `must be ${kind} greater than ${rule.exclusiveMinimum}`; } - return "must be a number"; + return `must be ${kind}`; } /* Factory function to make functions that hide expressions for a particular field */ diff --git a/frontend/src/app/workspace/types/custom-json-schema.interface.ts b/frontend/src/app/workspace/types/custom-json-schema.interface.ts index 0a784610c0f..7ce0aa97926 100644 --- a/frontend/src/app/workspace/types/custom-json-schema.interface.ts +++ b/frontend/src/app/workspace/types/custom-json-schema.interface.ts @@ -62,8 +62,11 @@ export type ValueRuleSet = Readonly<{ then: { // the accepted set, where the value is chosen from one enum?: ReadonlyArray; - // otherwise how the value is read, in JSON Schema's names + // otherwise how the value is read, in JSON Schema's names, with the bound the estimator + // puts on it where it has one type?: "number" | "integer"; + minimum?: number; + exclusiveMinimum?: number; // or, where the value is a choice between a set and a number and no type names it, // the shape it takes pattern?: string; From e967e787319eda0da270dd02f8bdfb6598fbb89c Mon Sep 17 00:00:00 2001 From: kary zheng Date: Mon, 24 Aug 2026 17:33:24 -0700 Subject: [PATCH 5/7] fix(operator): key a hyperparameter rule on the value the config carries The condition named the keyword the estimator takes. For SVR's shrinking that differs from the constant offering it, so the branch never held and the value it constrains stayed unconstrained. Co-Authored-By: Claude Opus 5 (1M context) --- .../base/SklearnAdvancedBaseDesc.scala | 13 ++++++++++++- .../base/SklearnAdvancedBaseDescSpec.scala | 8 ++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) 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 592ee04f451..47a37c7b545 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 @@ -127,7 +127,7 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] condition .putObject("parameter") .putArray("valEnum") - .add(param.getName) + .add(chosenValueOf(param)) val outcome = objectMapper.createObjectNode() if (param.getAllowedValues.nonEmpty) { @@ -162,6 +162,17 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] .set[ObjectNode]("allOf", branches) } + /** What a chosen `parameter` holds in the config, which a rule's condition has to name to + * hold: the enum constant, since that is what Jackson writes and what the form compares + * against. Not `getName`, the keyword the emitted Python passes on, which SVR's `shrinking` + * spells differently from the constant offering it. + */ + private def chosenValueOf(param: ParamClass): String = + param match { + case constant: Enum[_] => constant.name + case _ => param.getName + } + /** Puts a declared bound under the JSON Schema name for it, `>` and `>=` being the two forms * an estimator's range takes at the low end. A bound spelled any other way is skipped * rather than guessed at, since a wrong one turns away values that work. 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 7e794c070d9..400eff2fd9a 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 @@ -227,6 +227,14 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { Seq("abc", "", "scaleauto", "1.2.3").foreach(v => v.matches(pattern) shouldBe false) } + it should "name the parameter as the config spells it, not as the estimator does" in { + // SVR's `shrinking` is offered by a constant named `probability`, and a chosen parameter + // reaches the config as the constant. A condition naming the keyword instead would hold + // for nothing, leaving the value it constrains free. + val shrinking = ruleFor(valueRulesOf(classOf[SklearnAdvancedSVRTrainerOpDesc]), "probability") + shrinking.path("enum").elements().asScala.map(_.asText()).toSeq shouldBe Seq("true", "false") + } + it should "state a rule for every parameter whose converter says anything about it" in { val rules = valueRulesOf(classOf[SklearnAdvancedKNNClassifierTrainerOpDesc]) val covered = rules From 6345c7a3efe3c166a445290d261e7a827399cca6 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Tue, 25 Aug 2026 14:31:06 -0700 Subject: [PATCH 6/7] fix(frontend): re-judge a hyperparameter value when the parameter beside it changes The value's rule comes from the parameter chosen next to it, but the validator sits on the value control, and Angular re-runs a validator only when the control carrying it changes. A value typed for one parameter therefore kept the verdict it earned there: 1.0 stayed valid once the row switched from C to kernel. The field now re-judges itself whenever a sibling a condition names changes, reading formly's own event rather than the sibling control's so that the row model the branch is chosen from is already the new one, and comparing parents so that one table row leaves the others alone. The wiring moves out of the property editor into setValueRules beside the validator it belongs with, which also lets the new tests start from what the editor really builds: a rendered form over the real field controls, where picking a parameter is what drives the assertions. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../app/common/formly/formly-utils.spec.ts | 168 +++++++++++++++++- .../src/app/common/formly/formly-utils.ts | 42 ++++- .../operator-property-edit-frame.component.ts | 17 +- 3 files changed, 208 insertions(+), 19 deletions(-) diff --git a/frontend/src/app/common/formly/formly-utils.spec.ts b/frontend/src/app/common/formly/formly-utils.spec.ts index 13cafb5aeeb..6f8feea682f 100644 --- a/frontend/src/app/common/formly/formly-utils.spec.ts +++ b/frontend/src/app/common/formly/formly-utils.spec.ts @@ -17,7 +17,7 @@ * under the License. */ -import { FormlyFieldConfig } from "@ngx-formly/core"; +import { FormlyFieldConfig, FormlyModule } from "@ngx-formly/core"; import { createOutputFormChangeEventStream, createShouldHideFieldFunc, @@ -26,9 +26,17 @@ import { matchingValueRule, setChildTypeDependency, setHideExpression, + setValueRules, valueRulesValidationMessage, } from "./formly-utils"; import { ValueRuleSet } from "../../workspace/types/custom-json-schema.interface"; +import { Component } from "@angular/core"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; +import { NoopAnimationsModule } from "@angular/platform-browser/animations"; +import { AbstractControl, FormGroup, ReactiveFormsModule } from "@angular/forms"; +import { FormlyNgZorroAntdModule } from "@ngx-formly/ng-zorro-antd"; +import { TEXERA_FORMLY_CONFIG } from "./formly-config"; import { Subject } from "rxjs"; import { FORM_DEBOUNCE_TIME_MS } from "../../workspace/service/execute-workflow/execute-workflow.service"; import { PortSchema } from "../../workspace/types/workflow-compiling.interface"; @@ -321,8 +329,9 @@ describe("valueRules", () => { expect(check("gamma", "scaleauto")).toBe(false); }); - it("re-judges the same value when the row switches parameter", () => { - // a value typed for one parameter is usually wrong for the next, and stays visible + it("judges the same value against whichever parameter the row now holds", () => { + // a value typed for one parameter is usually wrong for the next, and stays visible. That + // the form asks again when the parameter changes is the rendered form's test below expect(check("C", "1.0")).toBe(true); expect(check("kernel", "1.0")).toBe(false); }); @@ -364,4 +373,157 @@ describe("valueRules", () => { expect(valueRulesValidationMessage(null, field("metric_params"))).toBe("must be a number"); }); }); + + /** + * Through a rendered form rather than a hand-made field, because what the field carries is + * only half of it: the other half is when Angular runs a validator, which is when the control + * carrying it changes and not when the parameter beside it does. + */ + describe("setValueRules in a rendered form", () => { + /** One `paraList` row: the parameter dropdown and the value field that follows it. */ + @Component({ + standalone: true, + imports: [ReactiveFormsModule, FormlyModule], + template: `
+ +
`, + }) + class RowHost { + readonly form = new FormGroup({}); + readonly model: Record = { + paraList: [ + { parameter: "C", value: "1.0" }, + { parameter: "kernel", value: "rbf" }, + ], + }; + readonly fields: FormlyFieldConfig[] = [ + { + key: "paraList", + type: "array", + fieldArray: { + fieldGroup: [ + { + key: "parameter", + type: "enum", + props: { + options: ["C", "degree", "gamma", "kernel", "metric_params"].map(p => ({ label: p, value: p })), + }, + }, + valueField, + ], + }, + }, + ]; + } + + let valueField: FormlyFieldConfig; + let fixture: ComponentFixture; + let parameter: AbstractControl; + let value: AbstractControl; + + beforeEach(async () => { + valueField = { key: "value" }; + setValueRules(valueField, rules); + + await TestBed.configureTestingModule({ + imports: [RowHost, NoopAnimationsModule, FormlyModule.forRoot(TEXERA_FORMLY_CONFIG), FormlyNgZorroAntdModule], + }).compileComponents(); + + fixture = TestBed.createComponent(RowHost); + fixture.detectChanges(); + parameter = rowControl(0, "parameter"); + value = rowControl(0, "value"); + }); + + const rowControl = (row: number, key: string): AbstractControl => + fixture.componentInstance.form.get(`paraList.${row}.${key}`)!; + + /** What the user does: picks a parameter, leaving whatever value the row already held. */ + const choose = (parameterName: string) => { + parameter.setValue(parameterName); + fixture.detectChanges(); + }; + + it("gives the value field the control and the validator the rules call for", () => { + expect(valueField.type).toBe("constrainedvalue"); + expect(value.valid).toBe(true); + expect(fixture.debugElement.query(By.css("texera-constrained-value"))).not.toBeNull(); + }); + + it("re-judges a value the row already holds when the parameter changes under it", () => { + // 1.0 is a C, and no kernel at all + expect(value.valid).toBe(true); + + choose("kernel"); + + expect(value.valid).toBe(false); + expect(value.hasError("valueRules")).toBe(true); + }); + + it("clears the error once the parameter changes to one the value suits", () => { + choose("kernel"); + expect(value.valid).toBe(false); + + choose("gamma"); + + expect(value.valid).toBe(true); + }); + + it("re-judges when the new parameter constrains the value no rule did before", () => { + choose("metric_params"); + value.setValue("1.5"); + expect(value.valid).toBe(true); + + choose("degree"); + + expect(value.valid).toBe(false); + }); + + it("leaves an empty value to the required rule whichever parameter it sits under", () => { + value.setValue(""); + choose("kernel"); + expect(value.valid).toBe(true); + }); + + it("gives the message of the parameter now chosen, not the one judged against", () => { + choose("kernel"); + value.markAsTouched(); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain("must be one of rbf, linear, poly, sigmoid, precomputed"); + }); + + it("follows the parameter with the control the branch calls for", () => { + // scoped to the row's value field, so that neither the parameter's own dropdown nor the + // second row is what is seen + const valueControl = (selector: string) => + fixture.debugElement.queryAll(By.css("texera-constrained-value"))[0].query(By.css(selector)); + expect(valueControl("input[nz-input]").nativeElement.type).toBe("number"); + expect(valueControl("nz-select")).toBeNull(); + + choose("kernel"); + + expect(valueControl("nz-select")).not.toBeNull(); + expect(valueControl("input[nz-input]")).toBeNull(); + }); + + it("re-judges only the row whose parameter changed", () => { + const otherValue = rowControl(1, "value"); + expect(otherValue.valid).toBe(true); + + choose("kernel"); + + // rbf is still a kernel, whatever the row above holds + expect(value.valid).toBe(false); + expect(otherValue.valid).toBe(true); + + rowControl(1, "parameter").setValue("degree"); + fixture.detectChanges(); + + expect(otherValue.valid).toBe(false); + }); + }); }); diff --git a/frontend/src/app/common/formly/formly-utils.ts b/frontend/src/app/common/formly/formly-utils.ts index 506cb11ca50..05d2a33c8c1 100644 --- a/frontend/src/app/common/formly/formly-utils.ts +++ b/frontend/src/app/common/formly/formly-utils.ts @@ -22,7 +22,7 @@ import { isDefined } from "../util/predicate"; import { Observable } from "rxjs"; import { FORM_DEBOUNCE_TIME_MS } from "../../workspace/service/execute-workflow/execute-workflow.service"; -import { debounceTime, distinctUntilChanged, filter, share } from "rxjs/operators"; +import { debounceTime, distinctUntilChanged, filter, share, tap } from "rxjs/operators"; import { HideType, ValueRuleSet } from "../../workspace/types/custom-json-schema.interface"; import { PortSchema } from "../../workspace/types/workflow-compiling.interface"; import { AbstractControl } from "@angular/forms"; @@ -129,6 +129,46 @@ export function valueRulesValidationMessage(_err: unknown, field: FormlyFieldCon return `must be ${kind}`; } +/** + * Gives a field whose accepted values follow a sibling's the control those values call for and + * the validator holding it to them. + * + * The two have to be kept in step. Which branch is in force moves with the sibling, but Angular + * re-runs a validator only when the control carrying it changes, so a value typed for the + * parameter before would otherwise keep the verdict it earned there. The hook re-judges the + * field whenever a sibling named by a condition changes, reading formly's own event rather than + * the sibling control's so that the row model the branch is chosen from is already the new one. + * Returning the subscription as an observable leaves formly to end it with the field. + */ +export function setValueRules(field: FormlyFieldConfig, rules: ValueRuleSet): void { + const siblings = new Set(rules.allOf.flatMap(branch => Object.keys(branch.if))); + field.type = "constrainedvalue"; + // written into the existing object rather than over it: `props` and `templateOptions` are two + // names for one object, and replacing it leaves them pointing at different ones + field.props = field.props ?? {}; + (field.props as Record).valueRules = rules; + field.validators = { + ...field.validators, + valueRules: { + expression: createValueRulesValidator(rules), + message: valueRulesValidationMessage, + }, + }; + field.hooks = { + ...field.hooks, + onInit: valueField => + valueField.options?.fieldChanges?.pipe( + filter( + change => + change.type === "valueChanges" && + change.field.parent === valueField.parent && + siblings.has(String(change.field.key)) + ), + tap(() => valueField.formControl?.updateValueAndValidity()) + ), + }; +} + /* Factory function to make functions that hide expressions for a particular field */ export function createShouldHideFieldFunc( hideTarget: string, diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts index 69fb08a283e..be0a2d88f91 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts @@ -42,10 +42,9 @@ import { WorkflowCompilingService } from "../../../service/compile-workflow/work import { createOutputFormChangeEventStream, createShouldHideFieldFunc, - createValueRulesValidator, setChildTypeDependency, setHideExpression, - valueRulesValidationMessage, + setValueRules, } from "src/app/common/formly/formly-utils"; import { TYPE_CASTING_OPERATOR_TYPE, @@ -861,19 +860,7 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On // a field whose accepted values follow a sibling's: give it the control those values // call for, and hold it to them before the workflow can be run if (isDefined(mapSource.valueRules)) { - const valueRules = mapSource.valueRules; - mappedField.type = "constrainedvalue"; - // written into the existing object rather than over it: `props` and `templateOptions` - // are two names for one object, and replacing it leaves them pointing at different ones - mappedField.props = mappedField.props ?? {}; - (mappedField.props as Record).valueRules = valueRules; - mappedField.validators = { - ...mappedField.validators, - valueRules: { - expression: createValueRulesValidator(valueRules), - message: valueRulesValidationMessage, - }, - }; + setValueRules(mappedField, mapSource.valueRules); } // if the title is fileName, then change it to custom autocomplete input template From 006ce38edad9f230cdacd541d593ff78b60ad124 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Tue, 25 Aug 2026 14:40:44 -0700 Subject: [PATCH 7/7] docs(frontend): shorten the note on setValueRules to what is not in the code Two facts a reader cannot get from reading it: Angular re-runs a validator only for its own control, and the row model is current by the time formly emits. The rest restated the body. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/app/common/formly/formly-utils.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/frontend/src/app/common/formly/formly-utils.ts b/frontend/src/app/common/formly/formly-utils.ts index 05d2a33c8c1..15c25fb071a 100644 --- a/frontend/src/app/common/formly/formly-utils.ts +++ b/frontend/src/app/common/formly/formly-utils.ts @@ -130,15 +130,13 @@ export function valueRulesValidationMessage(_err: unknown, field: FormlyFieldCon } /** - * Gives a field whose accepted values follow a sibling's the control those values call for and - * the validator holding it to them. + * Gives a field whose accepted values follow a sibling's both the control they call for and the + * validator holding it to them. * - * The two have to be kept in step. Which branch is in force moves with the sibling, but Angular - * re-runs a validator only when the control carrying it changes, so a value typed for the - * parameter before would otherwise keep the verdict it earned there. The hook re-judges the - * field whenever a sibling named by a condition changes, reading formly's own event rather than - * the sibling control's so that the row model the branch is chosen from is already the new one. - * Returning the subscription as an observable leaves formly to end it with the field. + * Angular re-runs a validator only when the control carrying it changes, so the field has to be + * re-judged when the sibling deciding its rule changes. The hook reads formly's own event rather + * than the sibling control's because formly writes the row model before emitting, and the row + * model is what picks the branch. */ export function setValueRules(field: FormlyFieldConfig, rules: ValueRuleSet): void { const siblings = new Set(rules.allOf.flatMap(branch => Object.keys(branch.if))); @@ -156,6 +154,7 @@ export function setValueRules(field: FormlyFieldConfig, rules: ValueRuleSet): vo }; field.hooks = { ...field.hooks, + // returned rather than subscribed, so that formly ends it with the field onInit: valueField => valueField.options?.fieldChanges?.pipe( filter(