Skip to content

Commit aa22008

Browse files
aglinxinyuanaglinxinyuan
committed
refactor(frontend): take loop-block membership from the compile result
Which Loop Starts enclose an operator now comes from the editing-time compile (`operatorLoopStarts`, LogicalPlan.enclosingLoopStarts) instead of the frontend's own graph walk. WorkflowCompilingService records it from every response, successful or failed, in a new LoopBlockService (a service of its own because ValidationWorkflowService, which WorkflowCompilingService injects, reads it). Until the first compile result no operator counts as inside a block. The variable names are still read from each enclosing Loop Start's `initialization`. The property panel and ValidationWorkflowService both look again after every compile result, so neither keeps its own list of graph edits. The graph sent to the compiler keeps an operator whose only errors are at loop-variable references: left out, the compiler could never place it inside its block, and a reloaded workflow with `$K` in a numeric property would stay invalid. Removes loop-block.util.ts, its spec, and WorkflowGraph's getEnclosingLoopStarts.
1 parent 55b495b commit aa22008

15 files changed

Lines changed: 457 additions & 463 deletions

‎frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts‎

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { ComponentFixture, discardPeriodicTasks, fakeAsync, TestBed, tick } from
2222
import { conditionalRequiredRules, OperatorPropertyEditFrameComponent } from "./operator-property-edit-frame.component";
2323
import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service";
2424
import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service";
25+
import { LoopBlockService } from "../../../service/compile-workflow/loop-block.service";
2526
import { CustomJSONSchema7, ValueRuleSet } from "../../../types/custom-json-schema.interface";
2627
import { OperatorMetadataService } from "../../../service/operator-metadata/operator-metadata.service";
2728
import { StubOperatorMetadataService } from "../../../service/operator-metadata/stub-operator-metadata.service";
@@ -2065,7 +2066,20 @@ describe("OperatorPropertyEditFrameComponent", () => {
20652066
fixture.detectChanges();
20662067
}
20672068

2068-
/** Loop Start (K = 2) -> body, and body -> Loop End unless `closed` is false. */
2069+
/**
2070+
* A compile result, which is what tells which operators sit inside a block: the body inside the Loop
2071+
* Start's block, or inside none when `inside` is false.
2072+
*/
2073+
function compileWithBodyInside(inside = true): void {
2074+
TestBed.inject(LoopBlockService).setOperatorLoopStarts(
2075+
inside ? { [body.operatorID]: [mockLoopStartPredicate.operatorID] } : {}
2076+
);
2077+
}
2078+
2079+
/**
2080+
* Loop Start (K = 2) -> body, and body -> Loop End unless `closed` is false; then the compile result
2081+
* that follows those edits.
2082+
*/
20692083
function addBlockAroundBody(closed = true): void {
20702084
workflowActionService.addOperator(mockLoopStartPredicate, mockPoint);
20712085
workflowActionService.addOperator(body, mockPoint);
@@ -2074,6 +2088,7 @@ describe("OperatorPropertyEditFrameComponent", () => {
20742088
if (closed) {
20752089
workflowActionService.addLink(mockScalaExecutorLoopEndLink);
20762090
}
2091+
compileWithBodyInside(closed);
20772092
}
20782093

20792094
it("lets every primitive field take a $reference, offering the variables of the enclosing Loop Start", () => {
@@ -2121,37 +2136,66 @@ describe("OperatorPropertyEditFrameComponent", () => {
21212136
expect(getField("limit")?.validators?.["type"].expression(control("$K"))).toBe(false);
21222137
});
21232138

2124-
it("rebuilds the open form when a link or the Loop Start's variables change its scope", () => {
2139+
it("counts the operator outside every block until a compile result puts it inside one", () => {
2140+
workflowActionService.addOperator(mockLoopStartPredicate, mockPoint);
2141+
workflowActionService.addOperator(body, mockPoint);
2142+
workflowActionService.addOperator(mockLoopEndPredicate, mockPoint);
2143+
workflowActionService.addLink(mockLoopStartScalaExecutorLink);
2144+
workflowActionService.addLink(mockScalaExecutorLoopEndLink);
2145+
openBody();
2146+
expect(getField("limit")?.type).toBe("integer");
2147+
2148+
compileWithBodyInside();
2149+
expect(getField("limit")?.type).toBe("loopvariableinput");
2150+
});
2151+
2152+
it("rebuilds the open form when a compile result changes its scope, and only then", () => {
21252153
addBlockAroundBody(false);
21262154
openBody();
21272155
expect(getField("limit")?.type).toBe("integer");
21282156

2129-
// closing the block puts the open operator inside it
2157+
// closing the block puts the open operator inside it, once the compile result says so
21302158
workflowActionService.addLink(mockScalaExecutorLoopEndLink);
2159+
expect(getField("limit")?.type).toBe("integer");
2160+
compileWithBodyInside();
21312161
expect(getField("limit")?.type).toBe("loopvariableinput");
21322162
expect(getField("limit")?.props?.["loopVariableOptions"]).toEqual(["$K"]);
21332163

2134-
// renaming the variable changes what the field offers
2164+
// renaming the variable changes what the field offers, read from the Loop Start in the graph
21352165
workflowActionService.setOperatorProperty(mockLoopStartPredicate.operatorID, {
21362166
initialization: "N = 1",
21372167
output: "table.iloc[N]",
21382168
});
2169+
expect(getField("limit")?.props?.["loopVariableOptions"]).toEqual(["$K"]);
2170+
compileWithBodyInside();
21392171
expect(getField("limit")?.props?.["loopVariableOptions"]).toEqual(["$N"]);
21402172

2141-
// an edit that leaves the scope as it is does not rebuild the form
2173+
// a compile result that leaves the scope as it is does not rebuild the form
21422174
const rerender = vi.spyOn(component, "rerenderEditorForm");
21432175
workflowActionService.setOperatorProperty(mockLoopStartPredicate.operatorID, {
21442176
initialization: "N = 1",
21452177
output: "table.iloc[0]",
21462178
});
2179+
compileWithBodyInside();
21472180
expect(rerender).not.toHaveBeenCalled();
21482181

21492182
// opening the block again takes the operator out of it
21502183
workflowActionService.deleteLinkWithID(mockLoopStartScalaExecutorLink.linkID);
2184+
expect(rerender).not.toHaveBeenCalled();
2185+
compileWithBodyInside(false);
21512186
expect(rerender).toHaveBeenCalledTimes(1);
21522187
expect(getField("limit")?.type).toBe("integer");
21532188
});
21542189

2190+
it("rebuilds nothing for a compile result after the open operator was deleted", () => {
2191+
addBlockAroundBody();
2192+
openBody();
2193+
const rerender = vi.spyOn(component, "rerenderEditorForm");
2194+
workflowActionService.deleteOperator(body.operatorID);
2195+
compileWithBodyInside();
2196+
expect(rerender).not.toHaveBeenCalled();
2197+
});
2198+
21552199
describe("a field with value rules", () => {
21562200
// the body given a sklearn trainer's hyperparameter rows, whose value's rules follow the parameter
21572201
const valueRules: ValueRuleSet = {

‎frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import { ExposePropertyWrapperComponent } from "../../../../common/formly/expose
2222
import { FormBindingService } from "../../../service/form-binding/form-binding.service";
2323
import { ExecuteWorkflowService } from "../../../service/execute-workflow/execute-workflow.service";
2424
import { WorkflowStatusService } from "../../../service/workflow-status/workflow-status.service";
25-
import { merge, Subject } from "rxjs";
25+
import { Subject } from "rxjs";
2626
import { AbstractControl, FormGroup, FormsModule, ReactiveFormsModule } from "@angular/forms";
2727
import { FormlyFieldConfig, FormlyFormOptions, FormlyModule } from "@ngx-formly/core";
2828
import Ajv from "ajv";
@@ -41,10 +41,10 @@ import { isDefined } from "../../../../common/util/predicate";
4141
import { customFormlyFieldType, NON_FORM_FIELD_TYPES } from "../../../util/custom-formly-type";
4242
import { applyLoopVariableField, primitiveSchemaType } from "../../../util/loop-variable-field.util";
4343
import { loopVariablesInScope } from "../../../util/loop-variable.util";
44-
import { LOOP_START_OP_TYPE } from "../../../service/workflow-graph/model/loop-block.util";
4544
import { ExecutionState, OperatorState } from "src/app/workspace/types/execute-workflow.interface";
4645
import { DynamicSchemaService } from "../../../service/dynamic-schema/dynamic-schema.service";
4746
import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service";
47+
import { LoopBlockService } from "../../../service/compile-workflow/loop-block.service";
4848
import {
4949
createOutputFormChangeEventStream,
5050
createShouldHideFieldFunc,
@@ -528,7 +528,8 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On
528528
private config: GuiConfigService,
529529
private workflowPveService: WorkflowPveService,
530530
private computingUnitStatusService: ComputingUnitStatusService,
531-
private uiUdfParametersSyncService: UiUdfParametersSyncService
531+
private uiUdfParametersSyncService: UiUdfParametersSyncService,
532+
private loopBlockService: LoopBlockService
532533
) {}
533534

534535
private patchPythonUdfEnvironmentSchema(schema: CustomJSONSchema7, environments: string[]): CustomJSONSchema7 {
@@ -874,30 +875,23 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On
874875
return undefined;
875876
}
876877
const graph = this.workflowActionService.getTexeraGraph();
877-
return graph.hasOperator(this.currentOperatorId) ? loopVariablesInScope(graph, this.currentOperatorId) : undefined;
878+
return graph.hasOperator(this.currentOperatorId)
879+
? loopVariablesInScope(graph, this.loopBlockService.getEnclosingLoopStarts(this.currentOperatorId))
880+
: undefined;
878881
}
879882

880883
/**
881-
* Which fields take a $reference, and which variables they offer, depend on the block around the
882-
* operator and on its Loop Starts' variables, which other operators' links and properties decide. So
883-
* on the edits that re-validate operators holding a reference (a link added or removed, a property
884-
* change of another operator that is a Loop Start), rebuild the form when the operator's scope is no
885-
* longer the one it was built with, rather than only when the operator is next opened.
884+
* Which fields take a $reference, and which variables they offer, depend on the blocks around the
885+
* operator, which the compile result tells, and on their Loop Starts' variables, which other operators'
886+
* links and properties decide; every such edit is followed by a compile. So after every compile result,
887+
* the same one on which ValidationWorkflowService re-validates the operators holding a reference,
888+
* rebuild the form when the operator's scope is no longer the one it was built with, rather than only
889+
* when the operator is next opened.
886890
*/
887891
private registerLoopScopeChangeHandler(): void {
888892
const graph = this.workflowActionService.getTexeraGraph();
889-
merge(
890-
graph.getLinkAddStream(),
891-
graph.getLinkDeleteStream(),
892-
graph
893-
.getOperatorPropertyChangeStream()
894-
.pipe(
895-
filter(
896-
({ operator }) =>
897-
operator.operatorType === LOOP_START_OP_TYPE && operator.operatorID !== this.currentOperatorId
898-
)
899-
)
900-
)
893+
this.loopBlockService
894+
.getCompileResultStream()
901895
.pipe(untilDestroyed(this))
902896
.subscribe(() => {
903897
if (

‎frontend/src/app/workspace/service/compile-workflow/loop-block.service.spec.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
import { TestBed } from "@angular/core/testing";
21+
import { LoopBlockService } from "./loop-block.service";
22+
23+
describe("LoopBlockService", () => {
24+
let service: LoopBlockService;
25+
26+
beforeEach(() => {
27+
service = TestBed.inject(LoopBlockService);
28+
});
29+
30+
it("counts no operator inside a loop block before the first compile result", () => {
31+
expect(service.getEnclosingLoopStarts("body")).toEqual([]);
32+
});
33+
34+
it("reports the Loop Starts a compile result puts around an operator, and none around one it leaves out", () => {
35+
service.setOperatorLoopStarts({ body: ["start"] });
36+
expect(service.getEnclosingLoopStarts("body")).toEqual(["start"]);
37+
expect(service.getEnclosingLoopStarts("start")).toEqual([]);
38+
expect(service.getEnclosingLoopStarts("outside")).toEqual([]);
39+
});
40+
41+
it("orders nested blocks' Loop Starts outermost first, whatever order the compile result gives them in", () => {
42+
// outer -> inner -> body -> ...: the inner Loop Start is itself inside the outer block
43+
service.setOperatorLoopStarts({ body: ["a-inner", "b-outer"], "a-inner": ["b-outer"] });
44+
expect(service.getEnclosingLoopStarts("body")).toEqual(["b-outer", "a-inner"]);
45+
// blocks that do not nest keep the compile result's order
46+
service.setOperatorLoopStarts({ body: ["s2", "s1"] });
47+
expect(service.getEnclosingLoopStarts("body")).toEqual(["s2", "s1"]);
48+
});
49+
50+
it("replaces the previous compile result with each new one", () => {
51+
service.setOperatorLoopStarts({ body: ["start"] });
52+
service.setOperatorLoopStarts({});
53+
expect(service.getEnclosingLoopStarts("body")).toEqual([]);
54+
});
55+
56+
it("does not take an operator id for a property of a plain object", () => {
57+
service.setOperatorLoopStarts({});
58+
expect(service.getEnclosingLoopStarts("constructor")).toEqual([]);
59+
expect(service.getEnclosingLoopStarts("__proto__")).toEqual([]);
60+
});
61+
62+
it("announces every compile result, including one that leaves every block as it was", () => {
63+
let announcements = 0;
64+
const subscription = service.getCompileResultStream().subscribe(() => announcements++);
65+
service.setOperatorLoopStarts({ body: ["start"] });
66+
service.setOperatorLoopStarts({ body: ["start"] });
67+
expect(announcements).toBe(2);
68+
subscription.unsubscribe();
69+
});
70+
});

‎frontend/src/app/workspace/service/compile-workflow/loop-block.service.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
import { Injectable } from "@angular/core";
21+
import { Observable, Subject } from "rxjs";
22+
23+
/**
24+
* Which Loop Starts enclose each operator, as the latest editing-time compile reports it. The backend
25+
* works it out from the logical plan it compiles, and WorkflowCompilingService records here the
26+
* `operatorLoopStarts` of every compile response, successful or not.
27+
*
28+
* Two readers look again after every compile result: the property panel, which offers `$name`
29+
* loop-variable references only inside a loop block, and ValidationWorkflowService, which waives the
30+
* type check at such a reference only there. Every edit that can move an operator into or out of a block,
31+
* or change the variables a Loop Start declares, is followed by a compile, so neither needs to know which
32+
* edits those are.
33+
*
34+
* Until the first compile result, no operator counts as inside a loop block.
35+
*
36+
* A service of its own rather than part of WorkflowCompilingService because ValidationWorkflowService,
37+
* which WorkflowCompilingService injects, reads it.
38+
*/
39+
@Injectable({
40+
providedIn: "root",
41+
})
42+
export class LoopBlockService {
43+
private operatorLoopStarts: ReadonlyMap<string, ReadonlyArray<string>> = new Map();
44+
private readonly compileResultStream = new Subject<void>();
45+
46+
/**
47+
* Records the `operatorLoopStarts` of a compile response, replacing the previous one, and announces it.
48+
* @param operatorLoopStarts for each operator inside a loop block, the Loop Starts of the blocks it is
49+
* inside; an operator outside every block is absent
50+
*/
51+
public setOperatorLoopStarts(operatorLoopStarts: Readonly<Record<string, ReadonlyArray<string>>>): void {
52+
this.operatorLoopStarts = new Map(Object.entries(operatorLoopStarts));
53+
this.compileResultStream.next();
54+
}
55+
56+
/**
57+
* The Loop Starts of the blocks the operator is inside, outermost first; empty for an operator outside
58+
* every block or not compiled yet. An outer block's Loop Start encloses an inner block's, so the fewer
59+
* Loop Starts enclose a Loop Start, the further out its block is; blocks that do not nest keep the
60+
* compile result's order.
61+
*/
62+
public getEnclosingLoopStarts(operatorID: string): ReadonlyArray<string> {
63+
const depth = (loopStartID: string): number => this.operatorLoopStarts.get(loopStartID)?.length ?? 0;
64+
return [...(this.operatorLoopStarts.get(operatorID) ?? [])].sort((a, b) => depth(a) - depth(b));
65+
}
66+
67+
/**
68+
* Emits after every compile result, including one that leaves every block as it was: the variables a
69+
* Loop Start declares may have changed all the same.
70+
*/
71+
public getCompileResultStream(): Observable<void> {
72+
return this.compileResultStream.asObservable();
73+
}
74+
}

0 commit comments

Comments
 (0)