Xiao-zhen-Liu commented on code in PR #5912:
URL: https://github.com/apache/texera/pull/5912#discussion_r3696823325
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -91,6 +107,13 @@ class PythonUDFOpDescV2 extends LogicalOp {
)
var outputColumns: List[Attribute] = List()
+ @JsonProperty
+ @JsonSchemaTitle("Parameters")
+ @JsonPropertyDescription(
+ "Parameters inferred from active self.UiParameter(...) calls in the Python
script"
+ )
+ var uiParameters: List[UiUDFParameter] = List()
Review Comment:
The parser seeds new rows with an empty value, and an empty value parses to
`0` / `False` / 1970-01-01 in the Python runtime — no warning. Combined with
the rows not showing up in the panel, a first run can silently use 0 for a
number the user never filled in. Worth deciding whether an empty value should
block the run instead.
Also, this same property block is repeated in all three descriptors; a small
shared trait would keep them from drifting.
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -516,6 +518,23 @@ export class OperatorPropertyEditFrameComponent implements
OnInit, OnChanges, On
this.currentOperatorStatus = update[this.currentOperatorId];
}
});
+
+ this.uiUdfParametersSyncService.uiParametersChanged$
+ .pipe(untilDestroyed(this))
+ .subscribe(({ operatorId, parameters }) => {
+ if (operatorId !== this.currentOperatorId) return;
+
+ const currentOperator =
this.workflowActionService.getTexeraGraph().getOperator(operatorId);
+
+ const newModel = {
+ ...cloneDeep(currentOperator.operatorProperties),
+ uiParameters: cloneDeep(parameters),
+ };
+
+ this.listeningToChange = false;
Review Comment:
Setting `listeningToChange = false` here blocks the only thing that
refreshes the panel. Yjs observers run synchronously inside `transact`, so the
property-change event fires before `setOperatorProperty` returns and gets
dropped by the filter on line 714 — `formData` keeps the old list, so the new
rows never appear until you click away and back.
Worse, since `checkOperatorProperty` compares that stale `formData` against
the graph, editing any other field afterwards writes the old list back and
drops the parsed parameters.
Refresh `formData` from the graph after the write, and consider doing the
write inside the sync service so it doesn't depend on which panel is open.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -130,7 +153,7 @@ class PythonUDFOpDescV2 extends LogicalOp {
workflowId,
executionId,
operatorIdentifier,
- OpExecWithCode(code, "python")
+ OpExecWithCode(PythonUdfUiParameterInjector.inject(code,
uiParameters), "python")
Review Comment:
No test for the injector wiring, though `PythonUDFOpDescV2Spec` already
asserts `OpExecWithCode(code, "python")` on this exact line — it only still
passes because an empty list returns the code unchanged. Please add a case per
descriptor with a non-empty list, asserting the hook shows up in the generated
code.
Small thing: `inject(...)` is called in both branches of the if/else;
compute it once above.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/udf/python/PythonUDFOpDescV2.scala:
##########
@@ -36,6 +36,22 @@ class PythonUDFOpDescV2 extends LogicalOp {
required = true,
defaultValue =
"# Choose from the following templates:\n" +
+ "# \n" +
+ "# UiParameter notes:\n" +
Review Comment:
Three concerns with this block: it's 16 lines of prose copy-pasted into
three files, the example `def open(self)` sits above the class so uncommenting
it as written won't work, and it leaves out the rule that actually breaks
things — the class has to be named one of `ProcessTupleOperator` /
`ProcessBatchOperator` / `ProcessTableOperator` / `GenerateOperator`, or the
parameters silently disappear.
Consider trimming this to two or three lines and putting the details in the
operator docs. Those docs also need updating:
`docs/reference/operators/user-defined-functions/python/*.md` still show the
old default code and don't list the new Parameters property.
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -516,6 +518,23 @@ export class OperatorPropertyEditFrameComponent implements
OnInit, OnChanges, On
this.currentOperatorStatus = update[this.currentOperatorId];
}
});
+
+ this.uiUdfParametersSyncService.uiParametersChanged$
Review Comment:
`uiParametersChanged$` is a `ReplaySubject(1)`, so when the panel is
destroyed and rebuilt (click blank canvas, then the operator again) this
subscription immediately gets the last parse result and writes it again —
including the empty values from before the user typed anything. That can
quietly replace a value the user already entered.
Worth ignoring the replayed event, or comparing against what's in the graph
before writing.
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.scss:
##########
@@ -212,3 +212,14 @@
padding-top: 8px;
border-top: 1px solid #f0f0f0;
}
+
+/* Style only the UDF Parameters field. */
+:host ::ng-deep label[for*="ui-udf-parameters"] {
Review Comment:
Matching `label[for*="ui-udf-parameters"]` relies on Formly building an id
that contains the widget type name, so a rename elsewhere would silently drop
the styling. A class on the widget's own template would be sturdier. Also
`#d1d1d1` and `1.5px` don't match the values used elsewhere in this file.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -403,6 +410,13 @@ export class CodeEditorComponent implements AfterViewInit,
SafeStyle, OnDestroy
}
this.setupAIAssistantActions(editor);
this.initCodeDebuggerComponent(editor);
+ if (this.detachYCodeListener) {
+ this.detachYCodeListener();
+ }
+
+ if (this.code) {
+ this.detachYCodeListener =
this.uiUdfParametersSyncService.attachToYCode(this.currentOperatorId,
this.code);
Review Comment:
Now that the sync is turned on, nothing subscribes to
`uiParametersParseError$`. Duplicate parameter names or two UDF classes make
the parser throw, the sync stops early, and the user sees no message while the
panel keeps the old rows. Worth showing that error somewhere in this PR, since
it's the only signal the user gets before running.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
