From 496adf4bd35730a4db749d9977e8ee1468dee58d Mon Sep 17 00:00:00 2001 From: kary zheng Date: Tue, 25 Aug 2026 19:57:23 -0700 Subject: [PATCH 1/2] feat(operator): refuse two hyperparameter rows that set one parameter An Advanced trainer emits one keyword argument per parameter row, so two rows naming one parameter emitted that keyword twice and Python rejected the operator with a repeated-keyword SyntaxError before any of it ran. The user was shown a line of generated code rather than the row they added. The form now warns on the offending row, driven by a uniqueAmongRows key the schema puts on the field rather than by an operator name in the frontend, since uniqueItems cannot say this: two rows naming one parameter differ in their other fields, so they are distinct items while still colliding on the keyword. The descriptor refuses the same thing while the workflow compiles, which names the operator and the parameter and also covers a workflow submitted through the API. Closes #7952 Co-Authored-By: Claude Opus 5 (1M context) --- .../base/HyperParameters.scala | 3 ++ .../base/SklearnAdvancedBaseDesc.scala | 16 ++++++++++ .../base/SklearnAdvancedBaseDescSpec.scala | 21 +++++++++++++ ...ator-property-edit-frame.component.spec.ts | 30 +++++++++++++++++++ .../operator-property-edit-frame.component.ts | 18 +++++++++++ .../types/custom-json-schema.interface.ts | 3 ++ 6 files changed, 91 insertions(+) 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..4fd56ea1598 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 @@ -29,6 +29,9 @@ import org.apache.texera.amber.operator.metadata.annotations.{ class HyperParameters[T] { + // Two rows naming one parameter emit its keyword argument twice, which the generated + // Python will not compile, so the form warns on the row rather than letting it be added. + @JsonSchemaInject(json = """{ "uniqueAmongRows": true }""") @JsonProperty(required = true) @JsonSchemaTitle("Parameter") @JsonPropertyDescription("Choose the name of the parameter") 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..3320fb2176b 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 @@ -61,6 +61,21 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperat @AutofillAttributeNameList var selectedFeatures: List[EncodableString] = _ + /** + * Each row emits one keyword argument, so two rows naming one parameter emit that keyword + * twice and Python rejects the operator with a repeated-keyword SyntaxError. Refused here + * instead, where the workflow fails to compile with the parameter named. + */ + private def requireDistinctParameters(paraList: List[HyperParameters[T]]): Unit = { + val repeated = paraList.map(_.parameter.getName).groupBy(identity).collect { + case (name, rows) if rows.size > 1 => name + } + require( + repeated.isEmpty, + s"Each parameter can be set at most once. Set more than once: ${repeated.toSeq.sorted.mkString(", ")}." + ) + } + private def getLoopTimes(paraList: List[HyperParameters[T]]): PythonTemplateBuilder = { for (ele <- paraList) { if (ele.parametersSource) { @@ -71,6 +86,7 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperat } def getParameter(paraList: List[HyperParameters[T]]): List[PythonTemplateBuilder] = { + requireDistinctParameters(paraList) var workflowParam = s""; var portParam = pyb""; var paramString = pyb"" 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..d2b15ad34cb 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 @@ -135,4 +135,25 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers { paramString should include("n_neighbors = int(table[") paramString should include(".values[i]") } + + it should "refuse two rows setting one parameter, whichever source they read" in { + val d = new TestSklearnMLOp + val paraList = List( + hyperParam("n_neighbors", "int", fromWorkflow = false, value = "5"), + hyperParam("n_neighbors", "int", fromWorkflow = true, attribute = "k_col") + ) + val thrown = the[IllegalArgumentException] thrownBy d.getParameter(paraList) + thrown.getMessage should include("n_neighbors") + } + + it should "accept rows setting different parameters" in { + val d = new TestSklearnMLOp + val paraList = List( + hyperParam("n_neighbors", "int", fromWorkflow = false, value = "5"), + hyperParam("leaf_size", "int", fromWorkflow = false, value = "30") + ) + val paramString = d.getParameter(paraList)(1).encode + paramString.filterNot(_.isWhitespace) should include("n_neighbors=int(") + paramString.filterNot(_.isWhitespace) should include("leaf_size=int(") + } } diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts index daea90d2d66..6f36ce9c278 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts @@ -1794,6 +1794,36 @@ describe("OperatorPropertyEditFrameComponent", () => { ); }); + it("adds a uniqueAmongRows validator that rejects a value another row already holds", () => { + component.setFormlyFormBinding({ + type: "object", + properties: { + paraList: { + type: "array", + items: { + type: "object", + properties: { parameter: { type: "string", uniqueAmongRows: true } }, + }, + }, + }, + } as CustomJSONSchema7); + // A row's own fields exist only once formly is asked to build a row. + const arrayField = getField("paraList")!; + const rowField = (arrayField.fieldArray as (root: FormlyFieldConfig) => FormlyFieldConfig)(arrayField); + const validator = rowField.fieldGroup?.find(f => f.key === "parameter")?.validators?.["uniqueAmongRows"]; + expect(validator).toBeDefined(); + + const twoRowsSettingC = { + key: "parameter", + parent: { parent: { model: [{ parameter: "C" }, { parameter: "C" }] } }, + } as any; + expect(validator!.expression({ value: "C" } as any, twoRowsSettingC)).toBe(false); + expect(validator!.expression({ value: "kernel" } as any, twoRowsSettingC)).toBe(true); + expect(validator!.message(null, { formControl: { value: "C" } } as any)).toBe( + '"C" is already set by another row' + ); + }); + it("maps datasetVersionPath to the datasetversionselector field type", () => { component.setFormlyFormBinding({ type: "object", 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..9fc3b9bad89 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 @@ -1153,6 +1153,24 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On }; } + // A field the schema marks unique holds a meaning the enclosing list cannot repeat. + // uniqueItems cannot say this: two hyperparameter rows naming one parameter differ in + // their other fields, so they are distinct items while still emitting one keyword twice. + if (mapSource.uniqueAmongRows === true) { + mappedField.validators.uniqueAmongRows = { + expression: (control: AbstractControl, field: FormlyFieldConfig) => { + const rows = field.parent?.parent?.model; + const key = field.key; + if (!isDefined(control?.value) || !Array.isArray(rows) || typeof key !== "string") { + return true; + } + return rows.filter(row => isDefined(row) && row[key] === control.value).length <= 1; + }, + message: (error: any, field: FormlyFieldConfig) => + `"${field.formControl?.value}" is already set by another row`, + }; + } + // Add custom validators for attribute type if (isDefined(mapSource.attributeTypeRules)) { mappedField.validators.checkAttributeType = { 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..eaf95b99183 100644 --- a/frontend/src/app/workspace/types/custom-json-schema.interface.ts +++ b/frontend/src/app/workspace/types/custom-json-schema.interface.ts @@ -69,4 +69,7 @@ export interface CustomJSONSchema7 extends JSONSchema7 { hideOnNull?: boolean; additionalEnumValue?: string; + + // no two rows of the enclosing list may hold the same value for this field + uniqueAmongRows?: boolean; } From e9a61cc18d9dde7c183d4ade5a7d79fb0e67e92b Mon Sep 17 00:00:00 2001 From: kary zheng Date: Fri, 28 Aug 2026 16:58:54 -0700 Subject: [PATCH 2/2] feat(frontend): recheck every row when one names a new parameter A duplicate marks each row that holds it, but Angular reruns a validator only on the control that changed. The row that resolved a duplicate left the other row showing an error that no longer described it, and a row changed onto a parameter another row already held marked only itself. Rechecking the whole column on any change answers both directions. The new tests render two real rows and type into one of them, which is what it takes to have a second row to clear. Co-Authored-By: Claude Opus 5 (1M context) --- ...ator-property-edit-frame.component.spec.ts | 62 ++++++++++++++++++- .../operator-property-edit-frame.component.ts | 18 ++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts index 6f36ce9c278..e8b547d7599 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts @@ -33,7 +33,7 @@ import { FORM_DEBOUNCE_TIME_MS } from "../../../service/execute-workflow/execute import { DatePipe } from "@angular/common"; import { By } from "@angular/platform-browser"; import { BrowserAnimationsModule } from "@angular/platform-browser/animations"; -import { FormControl, FormGroup, FormsModule, ReactiveFormsModule } from "@angular/forms"; +import { AbstractControl, FormControl, FormGroup, FormsModule, ReactiveFormsModule } from "@angular/forms"; import { FormlyFieldConfig, FormlyModule } from "@ngx-formly/core"; import { TEXERA_FORMLY_CONFIG } from "../../../../common/formly/formly-config"; import { HttpClientTestingModule } from "@angular/common/http/testing"; @@ -2228,6 +2228,66 @@ describe("OperatorPropertyEditFrameComponent", () => { setupPreview({ kind: "text", title: "T", pills: [] }); expect(realFixture.debugElement.query(By.css(".hf-task-preview-pills"))).toBeNull(); }); + + // Two rows holding one parameter mark each other, so the row that resolves the duplicate + // has to clear the row it left behind. Only a rendered form has the second row to clear. + describe("uniqueAmongRows across rendered rows", () => { + function renderTwoRows(first: string, second: string): void { + // A form the frame holds locked is disabled, and Angular does not validate a disabled control. + realComponent.interactive = true; + realComponent.setFormlyFormBinding({ + type: "object", + properties: { + paraList: { + type: "array", + items: { + type: "object", + properties: { parameter: { type: "string", uniqueAmongRows: true } }, + }, + }, + }, + } as CustomJSONSchema7); + realComponent.formData = { paraList: [{ parameter: first }, { parameter: second }] }; + realFixture.detectChanges(); + } + + function rowControl(index: number): AbstractControl { + return realComponent.formlyFormGroup!.get(["paraList", String(index), "parameter"])!; + } + + function typeIntoRow(index: number, parameter: string): void { + const input = realFixture.debugElement.queryAll(By.css("input"))[index].nativeElement as HTMLInputElement; + input.value = parameter; + input.dispatchEvent(new Event("input")); + realFixture.detectChanges(); + } + + it("marks both rows that name one parameter", () => { + renderTwoRows("C", "C"); + expect(realFixture.debugElement.queryAll(By.css("input")).length).toBe(2); + expect(rowControl(0).hasError("uniqueAmongRows")).toBe(true); + expect(rowControl(1).hasError("uniqueAmongRows")).toBe(true); + }); + + it("clears the row left behind when the other row picks a free parameter", () => { + renderTwoRows("C", "C"); + + typeIntoRow(1, "kernel"); + + expect(rowControl(0).hasError("uniqueAmongRows")).toBe(false); + expect(rowControl(1).hasError("uniqueAmongRows")).toBe(false); + }); + + it("marks the row already holding the parameter a row is changed onto", () => { + renderTwoRows("C", "kernel"); + expect(rowControl(0).hasError("uniqueAmongRows")).toBe(false); + + typeIntoRow(1, "C"); + + expect(rowControl(0).hasError("uniqueAmongRows")).toBe(true); + expect(rowControl(1).hasError("uniqueAmongRows")).toBe(true); + }); + }); }); describe("onFormChanges null handling", () => { 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 9fc3b9bad89..da273b8b12f 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 @@ -1169,6 +1169,24 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On message: (error: any, field: FormlyFieldConfig) => `"${field.formControl?.value}" is already set by another row`, }; + // Whether a row repeats another is a property of the whole column, but Angular reruns a + // validator only on the control that changed. A change is answered by rechecking every + // row, so the row that resolves a duplicate clears the one it left behind, and a row + // changed onto a parameter another row holds marks that row too. + mappedField.hooks = { + ...mappedField.hooks, + onInit: (field: FormlyFieldConfig) => { + field.formControl?.valueChanges + .pipe(untilDestroyed(this)) + .subscribe(() => + field.parent?.parent?.fieldGroup?.forEach(row => + row.fieldGroup + ?.find(sibling => sibling.key === field.key) + ?.formControl?.updateValueAndValidity({ emitEvent: false }) + ) + ); + }, + }; } // Add custom validators for attribute type