Skip to content

Commit 74939e5

Browse files
jtranjtran
authored
Fix execution caching to cache artifact graph NodePath (#6978)
* Fix to add NodePaths to SketchOnFace and SketchOnPlane artifacts * Fix to only compute the new part of the artifact graph * Change to early-return sooner when in mock mode * Add another test * Fix to propagate NodePath for sketch on face * Update output
1 parent 9906c99 commit 74939e5

File tree

63 files changed

+357
-274
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

63 files changed

+357
-274
lines changed

‎rust/kcl-lib/e2e/executor/cache.rs

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -259,18 +259,23 @@ extrude(profile001, length = 100)"#
259259
#[tokio::test(flavor = "multi_thread")]
260260
async fn kcl_test_cache_add_line_preserves_artifact_commands() {
261261
let code = r#"sketch001 = startSketchOn(XY)
262-
|> startProfile(at = [5.5229, 5.25217])
263-
|> line(end = [10.50433, -1.19122])
264-
|> line(end = [8.01362, -5.48731])
265-
|> line(end = [-1.02877, -6.76825])
266-
|> line(end = [-11.53311, 2.81559])
262+
profile001 = startProfile(sketch001, at = [5.5, 5.25])
263+
|> line(end = [10.5, -1.19])
264+
|> line(end = [8, -5.5])
265+
|> line(end = [-1.02, -6.76])
266+
|> line(end = [-11.5, 2.8])
267267
|> close()
268+
plane001 = offsetPlane(XY, offset = 20)
268269
"#;
269270
// Use a new statement; don't extend the prior pipeline. This allows us to
270271
// detect a prefix.
271272
let code_with_extrude = code.to_owned()
272273
+ r#"
273-
extrude(sketch001, length = 4)
274+
profile002 = startProfile(plane001, at = [0, 0])
275+
|> line(end = [0, 10])
276+
|> line(end = [10, 0])
277+
|> close()
278+
extrude001 = extrude(profile001, length = 4)
274279
"#;
275280

276281
let result = cache_test(
@@ -305,6 +310,24 @@ extrude(sketch001, length = 4)
305310
first.artifact_graph.len(),
306311
second.artifact_graph.len()
307312
);
313+
// Make sure we have NodePaths referring to the old code.
314+
let graph = &second.artifact_graph;
315+
assert!(!graph.is_empty());
316+
for artifact in graph.values() {
317+
assert!(!artifact.code_ref().map(|c| c.node_path.is_empty()).unwrap_or(false));
318+
assert!(
319+
!artifact
320+
.face_code_ref()
321+
// TODO: This fails, but it shouldn't.
322+
// .map(|c| c.node_path.is_empty())
323+
// Allowing the NodePath to be empty if the SourceRange is [0,
324+
// 0] as a more lenient check.
325+
.map(|c| !c.range.is_synthetic() && c.node_path.is_empty())
326+
.unwrap_or(false),
327+
"artifact={:?}",
328+
artifact
329+
);
330+
}
308331
}
309332

310333
#[tokio::test(flavor = "multi_thread")]

‎rust/kcl-lib/e2e/executor/outputs/cache_add_line_preserves_artifact_commands_0.png

4.44 KB
Loading

‎rust/kcl-lib/e2e/executor/outputs/cache_add_line_preserves_artifact_commands_1.png

-216 Bytes
Loading

‎rust/kcl-lib/src/execution/artifact.rs

Lines changed: 87 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ where
115115
seq.end()
116116
}
117117

118-
#[derive(Debug, Clone, Serialize, PartialEq, Eq, ts_rs::TS)]
118+
#[derive(Debug, Clone, Default, Serialize, PartialEq, Eq, ts_rs::TS)]
119119
#[ts(export_to = "Artifact.ts")]
120120
#[serde(rename_all = "camelCase")]
121121
pub struct CodeRef {
@@ -396,7 +396,6 @@ pub enum Artifact {
396396
Cap(Cap),
397397
SweepEdge(SweepEdge),
398398
EdgeCut(EdgeCut),
399-
#[expect(unused)]
400399
EdgeCutEdge(EdgeCutEdge),
401400
Helix(Helix),
402401
}
@@ -550,8 +549,9 @@ impl Artifact {
550549
}
551550
}
552551

553-
#[expect(dead_code)]
554-
pub(crate) fn code_ref(&self) -> Option<&CodeRef> {
552+
/// The [`CodeRef`] for the artifact itself. See also
553+
/// [`Self::face_code_ref`].
554+
pub fn code_ref(&self) -> Option<&CodeRef> {
555555
match self {
556556
Artifact::CompositeSolid(a) => Some(&a.code_ref),
557557
Artifact::Plane(a) => Some(&a.code_ref),
@@ -570,6 +570,24 @@ impl Artifact {
570570
}
571571
}
572572

573+
/// The [`CodeRef`] referring to the face artifact that it's on, not the
574+
/// artifact itself.
575+
pub fn face_code_ref(&self) -> Option<&CodeRef> {
576+
match self {
577+
Artifact::CompositeSolid(_)
578+
| Artifact::Plane(_)
579+
| Artifact::Path(_)
580+
| Artifact::Segment(_)
581+
| Artifact::Solid2d(_)
582+
| Artifact::StartSketchOnFace(_)
583+
| Artifact::StartSketchOnPlane(_)
584+
| Artifact::Sweep(_) => None,
585+
Artifact::Wall(a) => Some(&a.face_code_ref),
586+
Artifact::Cap(a) => Some(&a.face_code_ref),
587+
Artifact::SweepEdge(_) | Artifact::EdgeCut(_) | Artifact::EdgeCutEdge(_) | Artifact::Helix(_) => None,
588+
}
589+
}
590+
573591
/// Merge the new artifact into self. If it can't because it's a different
574592
/// type, return the new artifact which should be used as a replacement.
575593
fn merge(&mut self, new: Artifact) -> Option<Artifact> {
@@ -704,6 +722,19 @@ impl ArtifactGraph {
704722
self.map.len()
705723
}
706724

725+
pub fn is_empty(&self) -> bool {
726+
self.map.is_empty()
727+
}
728+
729+
pub fn values(&self) -> impl Iterator<Item = &Artifact> {
730+
self.map.values()
731+
}
732+
733+
/// Consume the artifact graph and return the map of artifacts.
734+
fn into_map(self) -> IndexMap<ArtifactId, Artifact> {
735+
self.map
736+
}
737+
707738
/// Used to make the mermaid tests deterministic.
708739
#[cfg(test)]
709740
pub(crate) fn sort(&mut self) {
@@ -712,17 +743,29 @@ impl ArtifactGraph {
712743
}
713744
}
714745

746+
/// Build the artifact graph from the artifact commands and the responses. The
747+
/// initial graph is the graph cached from a previous execution. NodePaths of
748+
/// `exec_artifacts` are filled in from the AST.
715749
pub(super) fn build_artifact_graph(
716750
artifact_commands: &[ArtifactCommand],
717751
responses: &IndexMap<Uuid, WebSocketResponse>,
718752
ast: &Node<Program>,
719-
exec_artifacts: &IndexMap<ArtifactId, Artifact>,
753+
exec_artifacts: &mut IndexMap<ArtifactId, Artifact>,
754+
initial_graph: ArtifactGraph,
720755
) -> Result<ArtifactGraph, KclError> {
721-
let mut map = IndexMap::new();
756+
let mut map = initial_graph.into_map();
722757

723758
let mut path_to_plane_id_map = FnvHashMap::default();
724759
let mut current_plane_id = None;
725760

761+
// Fill in NodePaths for artifacts that were added directly to the map
762+
// during execution.
763+
for exec_artifact in exec_artifacts.values_mut() {
764+
// Note: We only have access to the new AST. So if these artifacts
765+
// somehow came from cached AST, this won't fill in anything.
766+
fill_in_node_paths(exec_artifact, ast);
767+
}
768+
726769
for artifact_command in artifact_commands {
727770
if let ModelingCmd::EnableSketchMode(EnableSketchMode { entity_id, .. }) = artifact_command.command {
728771
current_plane_id = Some(entity_id);
@@ -762,6 +805,24 @@ pub(super) fn build_artifact_graph(
762805
Ok(ArtifactGraph { map })
763806
}
764807

808+
/// These may have been created with placeholder `CodeRef`s because we didn't
809+
/// have the entire AST available. Now we fill them in.
810+
fn fill_in_node_paths(artifact: &mut Artifact, program: &Node<Program>) {
811+
match artifact {
812+
Artifact::StartSketchOnFace(face) => {
813+
if face.code_ref.node_path.is_empty() {
814+
face.code_ref.node_path = NodePath::from_range(program, face.code_ref.range).unwrap_or_default();
815+
}
816+
}
817+
Artifact::StartSketchOnPlane(plane) => {
818+
if plane.code_ref.node_path.is_empty() {
819+
plane.code_ref.node_path = NodePath::from_range(program, plane.code_ref.range).unwrap_or_default();
820+
}
821+
}
822+
_ => {}
823+
}
824+
}
825+
765826
/// Flatten the responses into a map of command IDs to modeling command
766827
/// responses. The raw responses from the engine contain batches.
767828
fn flatten_modeling_command_responses(
@@ -846,6 +907,12 @@ fn artifacts_to_update(
846907
ast: &Node<Program>,
847908
exec_artifacts: &IndexMap<ArtifactId, Artifact>,
848909
) -> Result<Vec<Artifact>, KclError> {
910+
let uuid = artifact_command.cmd_id;
911+
let Some(response) = responses.get(&uuid) else {
912+
// Response not found or not successful.
913+
return Ok(Vec::new());
914+
};
915+
849916
// TODO: Build path-to-node from artifact_command source range. Right now,
850917
// we're serializing an empty array, and the TS wrapper fills it in with the
851918
// correct value based on NodePath.
@@ -858,14 +925,7 @@ fn artifacts_to_update(
858925
path_to_node,
859926
};
860927

861-
let uuid = artifact_command.cmd_id;
862928
let id = ArtifactId::new(uuid);
863-
864-
let Some(response) = responses.get(&uuid) else {
865-
// Response not found or not successful.
866-
return Ok(Vec::new());
867-
};
868-
869929
let cmd = &artifact_command.command;
870930

871931
match cmd {
@@ -1100,16 +1160,19 @@ fn artifacts_to_update(
11001160
let extra_artifact = exec_artifacts.values().find(|a| {
11011161
if let Artifact::StartSketchOnFace(s) = a {
11021162
s.face_id == face_id
1163+
} else if let Artifact::StartSketchOnPlane(s) = a {
1164+
s.plane_id == face_id
11031165
} else {
11041166
false
11051167
}
11061168
});
1107-
let sketch_on_face_source_range = extra_artifact
1169+
let sketch_on_face_code_ref = extra_artifact
11081170
.and_then(|a| match a {
1109-
Artifact::StartSketchOnFace(s) => Some(s.code_ref.range),
1110-
// TODO: If we didn't find it, it's probably a bug.
1171+
Artifact::StartSketchOnFace(s) => Some(s.code_ref.clone()),
1172+
Artifact::StartSketchOnPlane(s) => Some(s.code_ref.clone()),
11111173
_ => None,
11121174
})
1175+
// TODO: If we didn't find it, it's probably a bug.
11131176
.unwrap_or_default();
11141177

11151178
return_arr.push(Artifact::Wall(Wall {
@@ -1118,11 +1181,7 @@ fn artifacts_to_update(
11181181
edge_cut_edge_ids: Vec::new(),
11191182
sweep_id: path_sweep_id,
11201183
path_ids: Vec::new(),
1121-
face_code_ref: CodeRef {
1122-
range: sketch_on_face_source_range,
1123-
node_path: NodePath::from_range(ast, sketch_on_face_source_range).unwrap_or_default(),
1124-
path_to_node: Vec::new(),
1125-
},
1184+
face_code_ref: sketch_on_face_code_ref,
11261185
cmd_id: artifact_command.cmd_id,
11271186
}));
11281187
let mut new_seg = seg.clone();
@@ -1155,27 +1214,27 @@ fn artifacts_to_update(
11551214
let extra_artifact = exec_artifacts.values().find(|a| {
11561215
if let Artifact::StartSketchOnFace(s) = a {
11571216
s.face_id == face_id
1217+
} else if let Artifact::StartSketchOnPlane(s) = a {
1218+
s.plane_id == face_id
11581219
} else {
11591220
false
11601221
}
11611222
});
1162-
let sketch_on_face_source_range = extra_artifact
1223+
let sketch_on_face_code_ref = extra_artifact
11631224
.and_then(|a| match a {
1164-
Artifact::StartSketchOnFace(s) => Some(s.code_ref.range),
1225+
Artifact::StartSketchOnFace(s) => Some(s.code_ref.clone()),
1226+
Artifact::StartSketchOnPlane(s) => Some(s.code_ref.clone()),
11651227
_ => None,
11661228
})
1229+
// TODO: If we didn't find it, it's probably a bug.
11671230
.unwrap_or_default();
11681231
return_arr.push(Artifact::Cap(Cap {
11691232
id: face_id,
11701233
sub_type,
11711234
edge_cut_edge_ids: Vec::new(),
11721235
sweep_id: path_sweep_id,
11731236
path_ids: Vec::new(),
1174-
face_code_ref: CodeRef {
1175-
range: sketch_on_face_source_range,
1176-
node_path: NodePath::from_range(ast, sketch_on_face_source_range).unwrap_or_default(),
1177-
path_to_node: Vec::new(),
1178-
},
1237+
face_code_ref: sketch_on_face_code_ref,
11791238
cmd_id: artifact_command.cmd_id,
11801239
}));
11811240
let Some(Artifact::Sweep(sweep)) = artifacts.get(&path_sweep_id) else {

‎rust/kcl-lib/src/execution/mod.rs

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,23 +1155,24 @@ impl ExecutorContext {
11551155

11561156
#[cfg(feature = "artifact-graph")]
11571157
{
1158-
// Move the artifact commands and responses to simplify cache management
1159-
// and error creation.
1160-
exec_state
1161-
.global
1162-
.artifact_commands
1163-
.extend(self.engine.take_artifact_commands().await);
1164-
exec_state
1165-
.global
1166-
.artifact_responses
1167-
.extend(self.engine.take_responses().await);
1158+
let new_commands = self.engine.take_artifact_commands().await;
1159+
let new_responses = self.engine.take_responses().await;
1160+
let initial_graph = exec_state.global.artifact_graph.clone();
1161+
11681162
// Build the artifact graph.
1169-
match build_artifact_graph(
1170-
&exec_state.global.artifact_commands,
1171-
&exec_state.global.artifact_responses,
1163+
let graph_result = build_artifact_graph(
1164+
&new_commands,
1165+
&new_responses,
11721166
program,
1173-
&exec_state.global.artifacts,
1174-
) {
1167+
&mut exec_state.global.artifacts,
1168+
initial_graph,
1169+
);
1170+
// Move the artifact commands and responses into ExecState to
1171+
// simplify cache management and error creation.
1172+
exec_state.global.artifact_commands.extend(new_commands);
1173+
exec_state.global.artifact_responses.extend(new_responses);
1174+
1175+
match graph_result {
11751176
Ok(artifact_graph) => {
11761177
exec_state.global.artifact_graph = artifact_graph;
11771178
exec_result.map(|(_, env_ref, _)| env_ref)

‎rust/kcl-lib/tests/artifact_graph_example_code1/artifact_graph_flowchart.snap.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ flowchart LR
3131
1["Plane<br>[12, 29, 0]"]
3232
%% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
3333
2["StartSketchOnFace<br>[343, 382, 0]"]
34-
%% Missing NodePath
34+
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
3535
16["Sweep Extrusion<br>[258, 290, 0]"]
3636
%% [ProgramBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
3737
17["Sweep Extrusion<br>[553, 583, 0]"]

‎rust/kcl-lib/tests/artifact_graph_example_code_offset_planes/artifact_graph_flowchart.snap.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ flowchart LR
1313
3["Plane<br>[110, 138, 0]"]
1414
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit]
1515
4["StartSketchOnPlane<br>[152, 181, 0]"]
16-
%% Missing NodePath
16+
%% [ProgramBodyItem { index: 3 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
1717
1 <--x 4
1818
1 --- 5
1919
5 --- 6

‎rust/kcl-lib/tests/artifact_graph_sketch_on_face_etc/artifact_graph_flowchart.snap.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ flowchart LR
5555
1["Plane<br>[12, 29, 0]"]
5656
%% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
5757
2["StartSketchOnFace<br>[255, 294, 0]"]
58-
%% Missing NodePath
58+
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
5959
3["StartSketchOnFace<br>[511, 550, 0]"]
60-
%% Missing NodePath
60+
%% [ProgramBodyItem { index: 4 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
6161
4["StartSketchOnFace<br>[780, 819, 0]"]
62-
%% Missing NodePath
62+
%% [ProgramBodyItem { index: 6 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
6363
29["Sweep Extrusion<br>[212, 242, 0]"]
6464
%% [ProgramBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit]
6565
30["Sweep Extrusion<br>[468, 498, 0]"]

‎rust/kcl-lib/tests/crazy_multi_profile/artifact_graph_flowchart.snap.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ flowchart LR
130130
2["Plane<br>[1424, 1442, 0]"]
131131
%% [ProgramBodyItem { index: 11 }, VariableDeclarationDeclaration, VariableDeclarationInit]
132132
3["StartSketchOnFace<br>[309, 348, 0]"]
133-
%% Missing NodePath
133+
%% [ProgramBodyItem { index: 3 }, VariableDeclarationDeclaration, VariableDeclarationInit]
134134
60["Sweep Extrusion<br>[264, 296, 0]"]
135135
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit]
136136
61["Sweep RevolveAboutEdge<br>[1300, 1366, 0]"]

‎rust/kcl-lib/tests/error_revolve_on_edge_get_edge/artifact_graph_flowchart.snap.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ flowchart LR
2929
1["Plane<br>[6, 23, 0]"]
3030
%% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
3131
2["StartSketchOnFace<br>[203, 241, 0]"]
32-
%% Missing NodePath
32+
%% [ProgramBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
3333
15["Sweep Extrusion<br>[169, 189, 0]"]
3434
%% [ProgramBodyItem { index: 0 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 6 }]
3535
16[Wall]

‎rust/kcl-lib/tests/import_async/artifact_graph_flowchart.snap.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,11 @@ flowchart LR
6363
4["Plane<br>[1594, 1632, 0]"]
6464
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 11 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }, CallKwUnlabeledArg]
6565
5["StartSketchOnPlane<br>[1580, 1633, 0]"]
66-
%% Missing NodePath
66+
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 11 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
6767
6["StartSketchOnPlane<br>[1580, 1633, 0]"]
68-
%% Missing NodePath
68+
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 11 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
6969
7["StartSketchOnPlane<br>[1580, 1633, 0]"]
70-
%% Missing NodePath
70+
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 11 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 1 }, VariableDeclarationDeclaration, VariableDeclarationInit, PipeBodyItem { index: 0 }]
7171
33["Sweep Loft<br>[3201, 3268, 0]"]
7272
%% [ProgramBodyItem { index: 2 }, VariableDeclarationDeclaration, VariableDeclarationInit, FunctionExpressionBody, FunctionExpressionBodyItem { index: 15 }, VariableDeclarationDeclaration, VariableDeclarationInit]
7373
34[Wall]

0 commit comments

Comments
 (0)