You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/09/02 01:21:05 UTC
(texera) branch main updated (22a66d325c -> fe76eec2dc)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
from 22a66d325c feat(gui): take the required marker from the schema's own condition (#7644)
new 7db229c41f feat(frontend): remove an operator property when its field is cleared (#7584)
new fe76eec2dc feat(operator): give the KNN trainers' metric and metric_params a working converter (#7598)
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../KNNTrainer/SklearnAdvancedKNNParameters.java | 8 +++-
.../base/SklearnAdvancedBaseDesc.scala | 4 ++
...arnAdvancedKNNClassifierTrainerOpDescSpec.scala | 55 +++++++++++++++++++++-
.../base/SklearnAdvancedBaseDescSpec.scala | 3 ++
.../workflow-graph/model/workflow-graph.spec.ts | 14 ++++++
.../types/shared-editing.interface.spec.ts | 19 ++++++--
.../workspace/types/shared-editing.interface.ts | 20 ++++++--
7 files changed, 112 insertions(+), 11 deletions(-)
(texera) 02/02: feat(operator): give the KNN trainers' metric and metric_params a working converter (#7598)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
commit fe76eec2dc01522ab41af1a915e87aabb2954ed8
Author: Kary Zheng <15...@users.noreply.github.com>
AuthorDate: Wed Sep 2 00:54:10 2026 +0000
feat(operator): give the KNN trainers' metric and metric_params a working converter (#7598)
### What changes were proposed in this PR?
`SklearnAdvancedKNNParameters` pairs each hyperparameter with the Python
callable that converts what the user typed. Two of the seven named one
that cannot produce what scikit-learn accepts, so picking either failed
the run whatever was entered, from the same dropdown as the five that
work. Both trainers share the enum, so both were affected.
`metric` was declared `int`. Its accepted values are words, so
`int("minkowski")` raises before scikit-learn sees anything, and a
number that does convert is rejected as not one of the accepted names.
It is now `str`, which `weights` and `algorithm` beside it already are.
`metric_params` takes a mapping of extra keyword arguments for the
metric, and none of `int`, `float` or `str` returns one, so a
well-formed `{"p": 2}` arrived as that same text. It now names
`json.loads`. The type has never been limited to builtins, since the SVC
and SVR trainers already name an inline lambda for their boolean
parameters, but `json.loads` needs the module, so the generated template
imports json. Unconditionally rather than when such a parameter is
present: the alternative is threading each converter's imports through
every `ParamClass` for one parameter of one operator.
### Any related issues, documentation, discussions?
Fixes #7593.
### How was this PR tested?
- Reproduced against scikit-learn both ways. Before: `int("minkowski")`
raises ValueError, and `metric=int("3")` reaches the estimator and is
rejected as not an accepted metric name. After: minkowski with
`metric_params` `{"p": 3}` fits and predicts with the mapping arriving
as a dict, chebyshev fits on the regressor, and mahalanobis with a VI
matrix fits, which is the case `metric_params` exists for.
- The KNN classifier spec asserts which callable each value is handed to
in the emitted model call, not only in the parameter summary beside it.
Values travel base64-encoded, so the assertion is on the converter
rather than the literal.
- The base descriptor's spec covers the template's new import.
- `WorkflowOperator/testOnly *sklearnAdvanced*`: 7 suites, 40 tests,
none failed. `scalafmtCheckAll` and both scalafix checks clean.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
Co-authored-by: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../KNNTrainer/SklearnAdvancedKNNParameters.java | 8 +++-
.../base/SklearnAdvancedBaseDesc.scala | 4 ++
...arnAdvancedKNNClassifierTrainerOpDescSpec.scala | 55 +++++++++++++++++++++-
.../base/SklearnAdvancedBaseDescSpec.scala | 3 ++
4 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java
index 7bb8c9dd9a..9ab29dbe61 100644
--- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java
+++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNParameters.java
@@ -27,8 +27,12 @@ public enum SklearnAdvancedKNNParameters implements ParamClass {
weights("weights", "str"),
algorithm("algorithm", "str"),
leaf_size("leaf_size", "int"),
- metric("metric", "int"),
- metric_params("metric_params", "str");
+ // A metric is named, not measured: "minkowski" and the rest of the accepted
+ // set are words, so int() rejects every value scikit-learn would take.
+ metric("metric", "str"),
+ // The only one that is not a scalar. scikit-learn wants a mapping of extra
+ // keyword arguments for the metric, so the user's text is read as JSON.
+ metric_params("metric_params", "json.loads");
private final String name;
private final String type;
diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala
index 8c5a34c310..ba04aa5cbf 100644
--- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala
+++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala
@@ -98,10 +98,14 @@ abstract class SklearnMLOperatorDescriptor[T <: ParamClass] extends PythonOperat
val stringList = getParameter(paraList)
val trainingParam = stringList(1)
val paramString = stringList(0)
+ // A hyperparameter's declared type is emitted as the callable that converts the
+ // user's text, and the one taking a mapping names json.loads, so json is imported
+ // whether or not the operator at hand offers such a parameter.
val finalCode =
pyb"""
|from pytexera import *
|
+ |import json
|import pandas as pd
|${getImportStatements}
|
diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala
index 7c62b0cf2c..7d3acae13e 100644
--- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala
+++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/KNNTrainer/SklearnAdvancedKNNClassifierTrainerOpDescSpec.scala
@@ -19,11 +19,33 @@
package org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.KNNTrainer
-import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.SklearnMLOperatorDescriptor
+import org.apache.texera.amber.operator.machineLearning.sklearnAdvanced.base.{
+ HyperParameters,
+ SklearnMLOperatorDescriptor
+}
import org.scalatest.flatspec.AnyFlatSpec
class SklearnAdvancedKNNClassifierTrainerOpDescSpec extends AnyFlatSpec {
+ private def hyperParam(
+ parameter: SklearnAdvancedKNNParameters,
+ value: String
+ ): HyperParameters[SklearnAdvancedKNNParameters] = {
+ val hp = new HyperParameters[SklearnAdvancedKNNParameters]
+ hp.parameter = parameter
+ hp.parametersSource = false
+ hp.value = value
+ hp
+ }
+
+ private def codeFor(paraList: HyperParameters[SklearnAdvancedKNNParameters]*): String = {
+ val d = new SklearnAdvancedKNNClassifierTrainerOpDesc
+ d.paraList = paraList.toList
+ d.selectedFeatures = List("f1")
+ d.groundTruthAttribute = "label"
+ d.generatePythonCode()
+ }
+
"SklearnAdvancedKNNClassifierTrainerOpDesc.getImportStatements" should
"return the canonical KNeighborsClassifier import" in {
val d = new SklearnAdvancedKNNClassifierTrainerOpDesc
@@ -58,4 +80,35 @@ class SklearnAdvancedKNNClassifierTrainerOpDescSpec extends AnyFlatSpec {
}
assert(matched)
}
+
+ // The declared type is emitted as the callable that converts what the user
+ // typed, so a hyperparameter is only usable when that callable can return
+ // something scikit-learn accepts. These two could not: `metric` names a word
+ // from a fixed set, and `metric_params` a mapping.
+ "SklearnAdvancedKNNParameters.metric" should
+ "convert with str, the accepted metrics being words" in {
+ assert(
+ codeFor(hyperParam(SklearnAdvancedKNNParameters.metric, "minkowski"))
+ .contains("metric = str (")
+ )
+ }
+
+ "SklearnAdvancedKNNParameters.metric_params" should
+ "convert with json.loads, and the template must import json for it" in {
+ val code = codeFor(hyperParam(SklearnAdvancedKNNParameters.metric_params, """{"p": 2}"""))
+ assert(code.contains("metric_params = json.loads ("))
+ assert(code.contains("import json"))
+ }
+
+ it should "apply both conversions in the model call, not only in the summary" in {
+ val code = codeFor(
+ hyperParam(SklearnAdvancedKNNParameters.metric, "minkowski"),
+ hyperParam(SklearnAdvancedKNNParameters.metric_params, """{"p": 2}""")
+ )
+ // The values themselves travel base64-encoded, so what is checked here is
+ // which callable each one is handed to.
+ val modelCall = code.linesIterator.find(_.contains("model = KNeighborsClassifier(")).get
+ assert(modelCall.contains("metric = str ("))
+ assert(modelCall.contains("metric_params = json.loads ("))
+ }
}
diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala
index f151fc3b3f..9477539b00 100644
--- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala
+++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/machineLearning/sklearnAdvanced/base/SklearnAdvancedBaseDescSpec.scala
@@ -94,6 +94,9 @@ class SklearnAdvancedBaseDescSpec extends AnyFlatSpec with Matchers {
val d = newOp(List(hyperParam("n_neighbors", "int", fromWorkflow = false, value = "5")))
val code = d.generatePythonCode()
code should include("from pytexera import *")
+ // Unconditional: a hyperparameter's declared type is emitted as the callable
+ // that converts the user's text, and the one taking a mapping names json.loads.
+ code should include("import json")
code should include("from sklearn.neighbors import KNeighborsClassifier")
code should include("class ProcessTableOperator(UDFTableOperator):")
code should include("def process_table(")
(texera) 01/02: feat(frontend): remove an operator property when its field is cleared (#7584)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
commit 7db229c41f9a28b8e6ca6c107dd21e820e03dbb3
Author: Kary Zheng <15...@users.noreply.github.com>
AuthorDate: Wed Sep 2 00:53:52 2026 +0000
feat(frontend): remove an operator property when its field is cleared (#7584)
### What changes were proposed in this PR?
Clearing a text or number field in the property panel left the box empty
while the operator kept its previous value, so the workflow ran on a
value the user had removed and could no longer see. Setting CSV File
Scan's Limit to 5 and clearing it left the scan reading 5 rows, and the
value came back on the next reload.
A cleared field is simply not among the properties the panel sends:
formly drops a cleared text field from the model, and a cleared number
arrives as `null`, which `onFormChanges` filters out.
`updateYTypeFromObject` updates a value in place or sets a new one, and
for a key the new object no longer carries it did neither, so the old
entry stayed in the Y.Map. It now deletes such a key, deciding on
membership rather than on the value so that an explicit `undefined`
still takes the update path.
Clearing a property that has a schema default now shows that default
when the panel is next rendered; one without a default stays empty.
### Any related issues, documentation, discussions?
Closes #7394
The issue also read this as evidence that an empty string cannot be
stored from the UI. That part has been withdrawn: an absent property
deserializes to its Scala field initializer, which already gives an
empty value where one is meaningful.
### How was this PR tested?
The test that pinned the old behaviour is updated, since it described
what the function did rather than what it should do, and
`setOperatorProperty` gains a case that fails without this change. The
specs that write operator properties pass together, 465 cases.
The issue's reproduction was run before and after. Delimiter set to `X`
and cleared came back as `X`; it now leaves `,`, and Limit set to 5 and
cleared stays empty.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
Co-authored-by: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../workflow-graph/model/workflow-graph.spec.ts | 14 ++++++++++++++
.../workspace/types/shared-editing.interface.spec.ts | 19 ++++++++++++++++---
.../app/workspace/types/shared-editing.interface.ts | 20 +++++++++++++++-----
3 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
index 2e1e6b2ef2..41942cd7a5 100644
--- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
+++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
@@ -199,6 +199,20 @@ describe("WorkflowGraph", () => {
expect(operator.operatorProperties).toEqual(testProperty);
});
+ // The property panel clears a field by sending the properties without it, so a
+ // key left out has to be dropped here. Retaining it leaves the operator running
+ // on a value the user removed and the panel no longer shows.
+ it("should drop a property the new object leaves out", () => {
+ workflowGraph.addOperator(mockScanPredicate);
+ workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable", limit: 5 });
+
+ workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable" });
+
+ expect(workflowGraph.getOperator(mockScanPredicate.operatorID).operatorProperties).toEqual({
+ tableName: "testTable",
+ });
+ });
+
it("should throw an error when trying to set the property of an nonexist operator", () => {
expect(() => {
const testProperty = { tableName: "testTable" };
diff --git a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
index 40141d0006..83af7d2fda 100644
--- a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
+++ b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
@@ -142,7 +142,7 @@ describe("updateYTypeFromObject", () => {
expect(originalText.toJSON()).toBe("world");
});
- it("adds new object keys but leaves keys absent from the new object in place", () => {
+ it("adds new object keys and removes the ones the new object no longer carries", () => {
const doc = new Y.Doc();
const yMap = attach(doc, "m", createYTypeFromObject({ keep: 1, drop: 2 }));
@@ -153,8 +153,21 @@ describe("updateYTypeFromObject", () => {
// A key present only in the new object is added.
expect(asMap.has("add")).toBe(true);
expect(asMap.get("add")).toBe(3);
- // A key omitted from the new object is retained (an undefined new value never deletes).
- expect(asMap.get("drop")).toBe(2);
+ // A key the new object has dropped is deleted rather than left behind. Clearing
+ // an operator property is exactly this: the editor sends the properties without
+ // it, and anything retained here is a value the user removed and can no longer see.
+ expect(asMap.has("drop")).toBe(false);
+ });
+
+ it("keeps a key whose new value is explicitly undefined", () => {
+ const doc = new Y.Doc();
+ const yMap = attach(doc, "m", createYTypeFromObject({ a: 1 }));
+
+ updateYTypeFromObject(yMap, { a: undefined } as any);
+
+ // Carrying the key with an undefined value is not the same as dropping it, and
+ // only the latter means removal.
+ expect((yMap as unknown as Y.Map<any>).get("a")).toBe(1);
});
it("appends new items to a Y.Array in place", () => {
diff --git a/frontend/src/app/workspace/types/shared-editing.interface.ts b/frontend/src/app/workspace/types/shared-editing.interface.ts
index fc7843992d..f68beef1e4 100644
--- a/frontend/src/app/workspace/types/shared-editing.interface.ts
+++ b/frontend/src/app/workspace/types/shared-editing.interface.ts
@@ -246,11 +246,21 @@ export function updateYTypeFromObject<T extends object>(oldYObj: YType<T>, newOb
const keySet = new Set([...Object.keys(oldObj), ...Object.keys(newObj)]);
keySet.forEach((k: string) => {
const newValue = newObj[k as keyof T] as any;
- if (!_.isEqual(oldObj[k as keyof T], newValue)) {
- if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) {
- if (newValue !== undefined) {
- oldYObjAsYMap.set(k, createYTypeFromObject(newValue));
- }
+ if (_.isEqual(oldObj[k as keyof T], newValue)) {
+ return;
+ }
+ // A key the new object does not carry is one the caller removed, and deleting
+ // it is the only way that removal can reach the shared type: the update below
+ // changes a value or adds one, and for a key that is simply gone it did
+ // neither, leaving the old value in place. `in` rather than an undefined check,
+ // so a key explicitly carrying undefined still takes the path below.
+ if (!(k in newObj)) {
+ oldYObjAsYMap.delete(k);
+ return;
+ }
+ if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) {
+ if (newValue !== undefined) {
+ oldYObjAsYMap.set(k, createYTypeFromObject(newValue));
}
}
});