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..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"; @@ -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", @@ -2198,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 beedbabd90f..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 @@ -1153,6 +1153,42 @@ 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`, + }; + // 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 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; }