Feature/Include const_eval layer when flattening - #82
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to preserve the const_eval layer grouping when flattening graph layers in Model Explorer. The feature addresses issue #79, where const eval graphs were being lost during flattening, making visualization complex.
Changes:
- Added
addSeparateConstEvalLayerfunction that reorganizes nodes with "const_eval" in their namespace into a dedicated layer - Refactored duplicate single-child group node removal logic into reusable
removeSingleChildGroupNodesfunction - Added
removeFromParentOrRoothelper function for node relationship management
Reviewed changes
Copilot reviewed 1 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/src/components/visualizer/worker/graph_processor.ts | Implements const_eval layer preservation logic and refactors existing code for reusability |
| src/server/package/src/model_explorer/web_app/index.html | Build artifact update (script filename change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Rebuild parent-child relationships (same pattern as processNamespaceRelationships) | ||
| const nodesToLink = [ | ||
| constEvalGroupNode, | ||
| ...Array.from(seenNamespaces).map(ns => modelGraph.nodesById[this.getGroupNodeIdFromNamespace(ns)]), |
There was a problem hiding this comment.
Consider adding a filter to handle any potential undefined nodes. While createGroupNodeForNamespace should create all nodes in seenNamespaces, adding a safety check would make the code more robust:
.filter(node => node != null) after the map operation.
| ...Array.from(seenNamespaces).map(ns => modelGraph.nodesById[this.getGroupNodeIdFromNamespace(ns)]), | |
| ...Array.from(seenNamespaces) | |
| .map(ns => modelGraph.nodesById[this.getGroupNodeIdFromNamespace(ns)]) | |
| .filter(node => node != null), |
| this.removeFromParentOrRoot(modelGraph, node); | ||
|
|
||
| // Restore full namespace hierarchy | ||
| node.namespace = node.fullNamespace || node.namespace; |
There was a problem hiding this comment.
When restoring the namespace hierarchy, consider updating savedNamespace to match the restored namespace. Looking at line 625 in removeSingleChildGroupNodes, when node namespaces are updated, savedNamespace is also updated. For consistency, add:
node.savedNamespace = node.namespace;
after line 495.
| node.namespace = node.fullNamespace || node.namespace; | |
| node.namespace = node.fullNamespace || node.namespace; | |
| node.savedNamespace = node.namespace; |
| const ancestorNamespaces = this.getAncestorNamespaces(node.namespace); | ||
| for (const ns of ancestorNamespaces) { | ||
| this.createGroupNodeForNamespace(modelGraph, ns, seenNamespaces); | ||
| } |
There was a problem hiding this comment.
If a node has namespace exactly equal to "const_eval", then getAncestorNamespaces will return ["const_eval"], and the loop at lines 501-503 will call createGroupNodeForNamespace with namespace "const_eval". This will try to create a group node with ID "const_eval___group___", which is the same ID as the root const_eval group node created at line 474. This causes a duplicate node in modelGraph.nodes and overwrites the original node in modelGraph.nodesById.
To fix this, add "const_eval" to seenNamespaces before the loop:
seenNamespaces.add(CONST_EVAL_LAYER_NAME);
after line 488.
Adds a new function that runs when flatten layers is requested which will put anything under the const_eval namespace into its own layer called const_eval.
As it duplicates some existing functionality, some data processing has been moved into reusable functions.
Closes #79.