DAG Visualizer #25
DAG Visualizer #25
Conversation
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: Erik Wiens <[email protected]>
Co-authored-by: ErikWiens <[email protected]>
Co-authored-by: ErikWiens <[email protected]>
Co-authored-by: ErikWiens <[email protected]>
WalkthroughThe recent update introduces DAG visualization for ZenML pipeline runs within the VSCode extension. This feature enhances the user experience by allowing users to interactively explore Directed Acyclic Graphs (DAGs) directly from the Activity Bar. The changes encompass new command registrations, webview panel management, and visualization rendering, alongside necessary backend enhancements to fetch and display pipeline run data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VSCode
participant DagRenderer
participant ZenMLBackend
User->>VSCode: Clicks on DAG visualization command
VSCode->>DagRenderer: Initiates DAG render
DagRenderer->>ZenMLBackend: Fetch pipeline run data
ZenMLBackend->>DagRenderer: Returns pipeline run data
DagRenderer->>VSCode: Renders DAG in Webview Panel
VSCode->>User: Displays interactive DAG
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range, codebase verification and nitpick comments (9)
src/types/PipelineTypes.ts (4)
37-45: Add documentation comments for interfaces.Adding documentation comments will improve readability and maintainability.
+ /** + * Represents a step in the DAG. + */
47-55: Add documentation comments for interfaces.Adding documentation comments will improve readability and maintainability.
+ /** + * Represents an artifact in the DAG. + */
59-63: Add documentation comments for interfaces.Adding documentation comments will improve readability and maintainability.
+ /** + * Represents an edge in the DAG. + */
65-71: Add documentation comments for interfaces.Adding documentation comments will improve readability and maintainability.
+ /** + * Represents a pipeline run DAG. + */src/views/panel/panelView/PanelTreeItem.ts (3)
29-50: Ensure correct handling of nested structures and URLs.The constructor correctly handles different value types and sets commands for URLs. However, consider adding type checks or assertions for better type safety.
constructor(key: string, value: JsonType) { const simpleValue = typeof value === 'string' || typeof value === 'number'; super(key, simpleValue ? TreeItemCollapsibleState.None : TreeItemCollapsibleState.Collapsed); if (simpleValue) { this.description = String(value); } else if (value && typeof value === 'object') { this.description = '...'; this.children = Object.entries(value).map( ([key, value]) => new PanelDetailTreeItem(key, value) ); } if (typeof value === 'string' && value.startsWith('http')) { this.command = { title: 'Open URL', command: 'vscode.open', arguments: [Uri.parse(value)], }; this.iconPath = new ThemeIcon('link', new ThemeColor('textLink.foreground')); this.tooltip = `Click to open ${value}`; } }
61-69: Ensure correct handling of nested structures and source code.The constructor correctly handles different data types and creates child items. However, consider adding type checks or assertions for better type safety.
constructor(label: string, data: JsonObject) { super(label, TreeItemCollapsibleState.Expanded); this.children = Object.entries(data).map(([key, value]) => { if (key === 'sourceCode' && typeof value === 'string') { return new SourceCodeTreeItem(key, value); } return new PanelDetailTreeItem(key, value); }); }
80-86: Ensure correct handling of source code strings.The constructor correctly handles source code strings and creates child items. However, consider adding type checks or assertions for better type safety.
constructor(label: string, sourceCode: string) { super(label, TreeItemCollapsibleState.Collapsed); this.description = '...'; const lines = sourceCode.split('\n'); this.children = lines.map(line => new TreeItem(line)); }bundled/tool/zenml_grapher.py (2)
15-17: Consider Renaming the Class for ClarityThe class name
Grapheris generic. Consider a more descriptive name likePipelineRunGrapher.class PipelineRunGrapher: """Quick and dirty implementation of ZenML/LineageGraph to reduce number of api calls"""
89-97: Ensure Proper DocumentationEnsure that the
to_dictmethod's docstring accurately describes the returned dictionary structure.def to_dict(self) -> dict: """Returns dictionary containing graph data. Returns: dict: A dictionary containing nodes, edges, status, name, and version of the pipeline run. """
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (8)
resources/dag-view/icons/alert.svgis excluded by!**/*.svgresources/dag-view/icons/cached.svgis excluded by!**/*.svgresources/dag-view/icons/check.svgis excluded by!**/*.svgresources/dag-view/icons/database.svgis excluded by!**/*.svgresources/dag-view/icons/dataflow.svgis excluded by!**/*.svgresources/dag-view/icons/initializing.svgis excluded by!**/*.svgresources/dag-view/icons/play.svgis excluded by!**/*.svgresources/zenml-extension-dag.gifis excluded by!**/*.gif
Files selected for processing (24)
- .gitignore (2 hunks)
- README.md (2 hunks)
- bundled/tool/lsp_zenml.py (1 hunks)
- bundled/tool/zen_watcher.py (2 hunks)
- bundled/tool/zenml_grapher.py (1 hunks)
- bundled/tool/zenml_wrappers.py (2 hunks)
- package.json (6 hunks)
- resources/dag-view/dag.css (1 hunks)
- resources/dag-view/dag.js (1 hunks)
- src/commands/pipelines/DagRender.ts (1 hunks)
- src/commands/pipelines/cmds.ts (2 hunks)
- src/commands/pipelines/registry.ts (1 hunks)
- src/commands/pipelines/utils.ts (2 hunks)
- src/commands/server/utils.ts (1 hunks)
- src/extension.ts (3 hunks)
- src/services/ZenExtension.ts (2 hunks)
- src/test/ts_tests/mocks/constants.ts (4 hunks)
- src/types/PipelineTypes.ts (1 hunks)
- src/types/ServerInfoTypes.ts (2 hunks)
- src/utils/constants.ts (1 hunks)
- src/views/activityBar/serverView/ServerTreeItems.ts (1 hunks)
- src/views/panel/panelView/PanelDataProvider.ts (1 hunks)
- src/views/panel/panelView/PanelTreeItem.ts (1 hunks)
- webpack.config.js (2 hunks)
Files skipped from review due to trivial changes (2)
- .gitignore
- src/test/ts_tests/mocks/constants.ts
Additional context used
Path-based instructions (19)
src/commands/pipelines/utils.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/types/PipelineTypes.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.webpack.config.js (1)
Pattern
**/*.js: Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations.src/utils/constants.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/extension.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/registry.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.resources/dag-view/dag.js (1)
Pattern
**/*.js: Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations.src/views/panel/panelView/PanelTreeItem.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/views/panel/panelView/PanelDataProvider.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/types/ServerInfoTypes.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/cmds.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/server/utils.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.bundled/tool/zenml_grapher.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."src/views/activityBar/serverView/ServerTreeItems.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.bundled/tool/zen_watcher.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."src/services/ZenExtension.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.bundled/tool/lsp_zenml.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.bundled/tool/zenml_wrappers.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
Biome
src/views/panel/panelView/PanelDataProvider.ts
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 39-39: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/commands/pipelines/DagRender.ts
[error] 106-108: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 116-119: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 141-141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 131-138: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
LanguageTool
README.md
[grammar] ~42-~42: The verb “double-clicking” is spelled with a hyphen.
Context: ... panel(labeled 2 in the above graph) or double clicking anywhere there is not a node. - Mousi...(DOUBLE_HYPHEN)
[grammar] ~45-~45: The verb “Double-clicking” is spelled with a hyphen.
Context: ... view(labeled 3 in the above image) - Double clicking a node will open the dashboard in a web...(DOUBLE_HYPHEN)
Markdownlint
README.md
36-36: null
Images should have alternate text (alt text)(MD045, no-alt-text)
Additional comments not posted (38)
webpack.config.js (1)
49-57: Ensure consistent output paths.Ensure that the output paths are consistent with the project structure and requirements.
Verification successful
Output paths are consistent.
The output paths
distandresources/dag-viewin the webpack configuration are consistent with the project structure and requirements.
distis referenced inwebpack.config.js,package.json,scripts/clear_and_compile.sh, andpackage-lock.json.resources/dag-viewis referenced inwebpack.config.jsandsrc/commands/pipelines/DagRender.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the output paths in webpack configuration. # Test: Check if the output paths are consistent with the project structure. grep -r "path.resolve" webpack.config.jsLength of output: 143
Script:
#!/bin/bash # Description: Verify the usage of output paths in the project. # Test: Check for references to 'dist' and 'resources/dag-view' in the project. rg "dist|resources/dag-view"Length of output: 10597
src/utils/constants.ts (1)
45-45: LGTM!The new field
dashboard_urlis consistent with the existing fields.src/extension.ts (2)
22-22: Import statement forDagRendererlooks good.The import statement is correctly added and follows the existing pattern.
50-51: Integration ofDagRendererduring activation and deactivation looks good.The
DagRendereris instantiated during activation and deactivated during the deactivation of the extension. Ensure thatDagRendereris properly implemented to handle these lifecycle events.Also applies to: 66-66
Verification successful
Integration of
DagRendererduring activation and deactivation looks good.The
DagRendererclass includes adeactivatemethod that sets the static instance toundefinedand disposes of all open panels, ensuring proper cleanup during deactivation.
DagRendererimplementation:src/commands/pipelines/DagRender.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `DagRenderer` to ensure it handles activation and deactivation correctly. # Test: Search for the `DagRenderer` class implementation. rg --type typescript -A 10 'class DagRenderer'Length of output: 83
Script:
#!/bin/bash # Description: Verify the implementation of `DagRenderer` to ensure it handles activation and deactivation correctly. # Test: Search for the `DagRenderer` class implementation in TypeScript files. rg 'class DagRenderer' -t ts -A 10Length of output: 854
Script:
#!/bin/bash # Description: Verify if `DagRenderer` handles deactivation correctly by searching for cleanup methods. # Test: Search for methods related to deactivation or cleanup within the `DagRenderer` class. rg 'class DagRenderer' -t ts -A 50 src/commands/pipelines/DagRender.tsLength of output: 1701
src/commands/pipelines/registry.ts (1)
41-44: Registration ofzenml.renderDagcommand looks good.The command is correctly registered to call
pipelineCommands.renderDagand follows the existing pattern.resources/dag-view/dag.js (4)
1-8: Initialization ofsvgPanZoomlooks good.The
svgPanZoomlibrary is correctly imported and initialized for the DAG element, enabling control icons and setting the maximum zoom level.
11-20: Resize event handling looks good.The resize event is correctly handled to adjust the DAG element's dimensions and re-center the view.
24-40: Mouseover event handling for highlighting edges looks good.The mouseover event is correctly handled to highlight edges connected to the hovered node, enhancing the user experience.
42-72: Click event handling for node interaction looks good.The click event is correctly handled to post messages for step and artifact interactions, enabling user interaction with the DAG nodes.
resources/dag-view/dag.css (1)
1-145: CSS styles for DAG visualization look good.The CSS styles are correctly defined and follow best practices, ensuring the DAG elements are visually distinct and interactive.
src/views/panel/panelView/PanelDataProvider.ts (6)
47-49: LGTM!The method correctly triggers the refresh event.
56-60: LGTM!The method correctly sets the data and triggers a refresh.
62-65: LGTM!The method correctly sets the loading state and triggers a refresh.
73-75: LGTM!The method correctly returns the tree item.
83-101: LGTM!The method correctly returns the child items based on the element type.
83-101: LGTM!The method correctly returns the child items based on the element type.
src/types/ServerInfoTypes.ts (2)
16-16: LGTM!The new property
dashboard_urlis correctly integrated into theServerStatusinterface.
58-58: LGTM!The new property
dashboard_urlis correctly integrated into theZenServerStoreInfointerface.src/views/activityBar/serverView/ServerTreeItems.ts (1)
70-70: Ensure Consistent Icon UsageEnsure that the icon name
linkis consistent with the other icons used in theServerDetailTreeIteminstances.new ServerDetailTreeItem('Dashboard URL', this.serverStatus.dashboard_url, 'link')src/services/ZenExtension.ts (1)
Line range hint
43-65: LGTM! Ensure the new data provider is properly utilized.The code correctly adds a new data provider for the panel view. Ensure that the
PanelDataProvideris properly utilized and integrated into the rest of the application.package.json (2)
234-239: LGTM! Ensure the new command is properly registered.The new command for rendering DAGs is correctly defined. Ensure that it is properly registered and integrated into the rest of the application.
266-272: LGTM! Ensure the new views are properly utilized.The new views for the ZenML panel are correctly defined. Ensure that they are properly utilized and integrated into the rest of the application.
Also applies to: 297-302
bundled/tool/lsp_zenml.py (1)
277-299: LGTM! Ensure the new commands are properly registered.The new commands for fetching pipeline run data, steps, artifacts, and DAGs are correctly defined. Ensure that they are properly registered and integrated into the rest of the application.
src/commands/pipelines/DagRender.ts (8)
153-173: LGTM!The
renderDagmethod looks good and follows best practices.
175-178: LGTM!The
loadSvgWindowLibmethod looks good and follows best practices.
201-203: LGTM!The
deregisterDagPanelmethod looks good and follows best practices.
205-207: LGTM!The
getDagPanelmethod looks good and follows best practices.
209-215: LGTM!The
registerDagPanelmethod looks good and follows best practices.
217-229: LGTM!The
layoutDagmethod looks good and follows best practices.
253-308: LGTM!The
drawDagmethod looks good and follows best practices.
38-47: Verify the path correctness inloadIconsmethod call.Ensure that the path passed to
loadIconsis correct and the icons are loaded as expected.Verification successful
Path correctness in
loadIconsmethod call verified successfully.The icons directory and files exist, confirming that the path used in the
loadIconsmethod call is correct.
- Path:
resources/dag-view/icons/- Files:
alert.svg,cached.svg,check.svg,database.svg,dataflow.svg,initializing.svg,play.svgScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the icons directory and files. # Test: Check if the icons directory and files exist. Expect: All files exist. ls resources/dag-view/icons/Length of output: 113
bundled/tool/zenml_wrappers.py (7)
329-364: LGTM!The
get_pipeline_runmethod looks good and follows best practices.
366-382: LGTM!The
get_pipeline_run_graphmethod looks good and follows best practices.
384-429: LGTM!The
get_run_stepmethod looks good and follows best practices.
431-466: LGTM!The
get_run_artifactmethod looks good and follows best practices.
18-18: LGTM!The
GlobalConfigWrapperclass looks good and follows best practices.
18-18: LGTM!The
ZenServerWrapperclass looks good and follows best practices.
18-18: LGTM!The
StacksWrapperclass looks good and follows best practices.
|
Implements #17 |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (8)
- README.md (2 hunks)
- bundled/tool/zen_watcher.py (2 hunks)
- requirements.txt (3 hunks)
- src/commands/pipelines/DagRender.ts (1 hunks)
- src/commands/pipelines/utils.ts (2 hunks)
- src/commands/server/utils.ts (1 hunks)
- src/test/python_tests/requirements.txt (1 hunks)
- src/views/panel/panelView/PanelDataProvider.ts (1 hunks)
Files skipped from review due to trivial changes (2)
- requirements.txt
- src/test/python_tests/requirements.txt
Files skipped from review as they are similar to previous changes (3)
- bundled/tool/zen_watcher.py
- src/commands/pipelines/utils.ts
- src/commands/server/utils.ts
Additional context used
Path-based instructions (2)
src/views/panel/panelView/PanelDataProvider.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Biome
src/commands/pipelines/DagRender.ts
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (14)
README.md (2)
12-12: LGTM!The term "Directed Acyclic Graphs" is correctly hyphenated.
34-45: LGTM!The term "double-clicking" is correctly hyphenated, and the image includes alt text.
src/commands/pipelines/DagRender.ts (12)
38-47: LGTM!The constructor initializes the
DagRendererinstance and loads necessary resources.
54-56: LGTM!The method retrieves the singleton instance of
DagRenderer.
61-64: LGTM!The method deactivates the
DagRendererand disposes of open panels.
160-180: LGTM!The method renders the DAG visualization in the webview panel.
182-185: LGTM!The method loads the SVG window library.
208-210: LGTM!The method deregisters a DAG panel by removing it from the
openPanelsobject.
212-214: LGTM!The method retrieves a DAG panel from the
openPanelsobject.
216-222: LGTM!The method registers a DAG panel by adding it to the
openPanelsobject and sets up a disposal callback.
224-236: LGTM!The method lays out the DAG using the Dagre library.
238-258: LGTM!The method calculates the edges of the DAG.
260-315: LGTM!The method draws the DAG using the SVG.js library.
318-381: LGTM!The methods generate HTML content for the webview panel.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
src/commands/pipelines/DagRender.ts (1)
13-27: Ensure all imports are necessary and used.The following import is not used in the file and should be removed to keep the code clean:
registerWindowfrom@svgdotjs/svg.jsRemove the unused import.
Analysis chain
Ensure all imports are necessary and used.
Verify that all imported modules and declared constants are necessary and used within the file. Unused imports and constants can be removed to keep the code clean.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all imported modules and declared constants are used within the file. # Test: Search for the usage of imported modules and constants. Expect: All imports and constants are used. rg --type ts 'import |const ' src/commands/pipelines/DagRender.tsLength of output: 3620
Script:
#!/bin/bash # Description: Verify usage of each imported module and declared constant within the file. # List of imports and constants to check imports_and_constants=( 'fs' 'vscode' 'Dagre' 'ArrayXY' 'SVG' 'registerWindow' 'PipelineTreeItem' 'ServerDataProvider' 'PipelineRunDag' 'DagNode' 'LSClient' 'ServerStatus' 'JsonObject' 'PanelDataProvider' 'ROOT_PATH' 'CSS_FILE' 'JS_FILE' 'ICONS_DIRECTORY' ) # Search for usage of each import and constant for item in "${imports_and_constants[@]}"; do echo "Searching for usage of: $item" rg --type ts "$item" src/commands/pipelines/DagRender.ts doneLength of output: 8453
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- src/commands/pipelines/DagRender.ts (1 hunks)
Additional context used
Path-based instructions (1)
src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Learnings (1)
src/commands/pipelines/DagRender.ts (1)
Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: src/commands/pipelines/DagRender.ts:0-0 Timestamp: 2024-07-15T15:55:05.952Z Learning: In `DagRender.ts`, the `const stepData` declaration should be wrapped in a block within the switch statement to restrict its access to the switch clause.
Biome
src/commands/pipelines/DagRender.ts
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (7)
src/commands/pipelines/DagRender.ts (7)
54-64: LGTM!The
getInstanceanddeactivatemethods are correctly implemented to manage the singleton instance and clean up resources.
137-178: LGTM!The
loadStepDataIntoPanelandloadArtifactDataIntoPanelmethods are correctly implemented with proper error handling.
180-200: LGTM!The
renderDagmethod is correctly implemented with proper error handling.
202-226: LGTM!The
loadSvgWindowLibandloadIconsmethods are correctly implemented with proper error handling.
228-242: LGTM!The
deregisterDagPanel,getDagPanel, andregisterDagPanelmethods are correctly implemented to manage the registration and deregistration of DAG panels.
244-278: LGTM!The
layoutDagandcalculateEdgesmethods are correctly implemented with proper logic and performance considerations.
280-401: LGTM!The
drawDag,getLoadingContent, andgetWebviewContentmethods are correctly implemented with proper logic and performance considerations.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (1)
src/commands/pipelines/DagRender.ts (1)
209-212: Ensure proper error handling for dynamic import.The
loadSvgWindowLibmethod does not include error handling for the dynamic import ofsvgdom. To ensure robustness, add a try-catch block to handle potential errors.
- Location:
src/commands/pipelines/DagRender.ts- Lines: 209-212
Analysis chain
Ensure proper error handling for dynamic import.
Ensure that any errors during the dynamic import of
svgdomare properly handled.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that errors during the dynamic import of `svgdom` are properly handled. # Test: Search for error handling in the `loadSvgWindowLib` method. Expect: Proper error handling. rg --type ts 'loadSvgWindowLib'Length of output: 169
Script:
#!/bin/bash # Description: Retrieve the full implementation of the `loadSvgWindowLib` method to check for error handling. # Use ast-grep to find the full implementation of the `loadSvgWindowLib` method in TypeScript files. ast-grep --lang ts --pattern $'class $_ { $$$ private async loadSvgWindowLib() { $$$ } $$$ }' --jsonLength of output: 24961
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- src/commands/pipelines/DagRender.ts (1 hunks)
Additional context used
Path-based instructions (1)
src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Learnings (1)
src/commands/pipelines/DagRender.ts (2)
Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: src/commands/pipelines/DagRender.ts:29-47 Timestamp: 2024-07-15T16:49:13.399Z Learning: In the `DagRenderer` class, the `createSVGWindow` property uses the `Function` type because it holds a dynamically imported function from `svgdom`, which is not compatible with the standard TypeScript import settings. The initial value is a placeholder and will have a different signature once the dynamic import takes place.Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: src/commands/pipelines/DagRender.ts:0-0 Timestamp: 2024-07-15T15:55:05.952Z Learning: In `DagRender.ts`, the `const stepData` declaration should be wrapped in a block within the switch statement to restrict its access to the switch clause.
Biome
src/commands/pipelines/DagRender.ts
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (2)
src/commands/pipelines/DagRender.ts (2)
32-32: Avoid usingFunctionas a type.The
createSVGWindowproperty usesFunctionas a type, which is discouraged. Prefer explicitly defining the function shape.However, given the dynamic nature of the import and the constraints with
svgdom, usingFunctionas a type forcreateSVGWindowis justified in this context. No changes are needed here.Tools
Biome
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
214-231: Improve error handling by logging the error message.Log the error message to provide more information about the failure.
} catch (e) { this.iconSvgs[name] = ''; console.error(`Unable to load icon ${name}: ${e}`); + console.error(e); }Likely invalid or redundant comment.
…ted by code rabbit. Co-authored-by: Erik Wiens <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- src/commands/pipelines/DagRender.ts (1 hunks)
Additional context used
Path-based instructions (1)
src/commands/pipelines/DagRender.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Learnings (1)
src/commands/pipelines/DagRender.ts (1)
Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: src/commands/pipelines/DagRender.ts:29-47 Timestamp: 2024-07-15T16:49:13.399Z Learning: In the `DagRenderer` class, the `createSVGWindow` property uses the `Function` type because it holds a dynamically imported function from `svgdom`, which is not compatible with the standard TypeScript import settings. The initial value is a placeholder and will have a different signature once the dynamic import takes place.
Biome
src/commands/pipelines/DagRender.ts
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Additional comments not posted (4)
src/commands/pipelines/DagRender.ts (4)
32-32: Consider using a more specific type instead ofFunction.The use of
Functionas a type is generally discouraged due to its broad acceptance of any function-like value, which can lead to bugs. However, in this context, it holds a dynamically imported function fromsvgdom, which is not compatible with the standard TypeScript import settings. Therefore, the use ofFunctionis justified here.Tools
Biome
[error] 32-32: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
38-47: Initialization in ConstructorThe constructor initializes various properties and preloads necessary resources. It's well-structured and follows good practices for resource initialization in a class constructor.
54-56: Singleton Pattern ImplementationThe
getInstancemethod correctly implements the singleton pattern, ensuring that only one instance ofDagRendererexists. This is a common pattern for managing resources like webview panels.
61-64: Proper Cleanup indeactivateMethodThe
deactivatemethod correctly disposes of all open panels when the extension is deactivated, preventing potential memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
scripts/lint.sh (1)
12-12: Approve the updated lint command but suggest enhancing the error message for clarity.The update to the lint command is appropriate if it aligns with the latest
ruffusage guidelines.However, consider enhancing the error message to provide more context about the failure.
- echo "Linting Python files failed" + echo "Linting Python files with ruff failed"
There was a problem hiding this comment.
Some other small changes here, @Christopher-R-Perkins and @ErikWiens. Looks good otherwise! Thanks.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
bundled/tool/zenml_grapher.py (2)
15-17: Clarify the class documentation.The class
Grapheris described as a "quick and dirty implementation," which might not instill confidence in its reliability or efficiency. Consider rephrasing this to reflect the purpose more professionally.
18-23: Initialization method review.The
__init__method correctly initializes theGrapherinstance with necessary lists and dictionaries. However, consider adding type hints for therunparameter to enhance code readability and maintainability.- def __init__(self, run): + def __init__(self, run: PipelineRun):
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- bundled/tool/zenml_grapher.py (1 hunks)
Additional context used
Path-based instructions (1)
bundled/tool/zenml_grapher.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
Learnings (1)
bundled/tool/zenml_grapher.py (1)
Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: bundled/tool/zenml_grapher.py:81-87 Timestamp: 2024-07-15T15:39:04.231Z Learning: When building edges from step data within a pipeline run response, duplicates are not expected, as the edges map input->step and step->output.
Additional comments not posted (2)
bundled/tool/zenml_grapher.py (2)
64-79: Review Edge Building Logic.The method
build_edges_from_stepsefficiently builds edges from step inputs and outputs. Given previous discussions and the learning that duplicates are not expected, the current implementation seems appropriate. However, consider adding a brief comment explaining why duplicates are not checked for here, to clarify for future maintainers.
89-97: Reviewto_dictMethod.This method converts the internal state of the grapher to a dictionary format, which is straightforward and correctly implemented. Consider adding type hints for the return type to align with best practices.
- def to_dict(self) -> dict: + def to_dict(self) -> Dict[str, Any]:
bundled/tool/zenml_grapher.py
Outdated
| def add_edge(self, v, w) -> None: | ||
| """Helper method to add an edge to the internal edges list""" | ||
| self.edges.append({ | ||
| "id": f"{v}_{w}", | ||
| "source": v, | ||
| "target": w, | ||
| }) |
There was a problem hiding this comment.
Review add_edge Method.
The method adds edges to the internal list without checking for duplicates. Based on previous discussions, this is acceptable due to the nature of the data. However, adding type hints and a brief description in the method's docstring about the parameters v and w could enhance clarity.
- def add_edge(self, v, w) -> None:
+ def add_edge(self, v: str, w: str) -> None:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_edge(self, v, w) -> None: | |
| """Helper method to add an edge to the internal edges list""" | |
| self.edges.append({ | |
| "id": f"{v}_{w}", | |
| "source": v, | |
| "target": w, | |
| }) | |
| def add_edge(self, v: str, w: str) -> None: | |
| """Helper method to add an edge to the internal edges list""" | |
| self.edges.append({ | |
| "id": f"{v}_{w}", | |
| "source": v, | |
| "target": w, | |
| }) |
There was a problem hiding this comment.
Changed in latest commit
There was a problem hiding this comment.
@Christopher-R-Perkins The changes to the add_edge method, including the type hints and docstring, have been successfully implemented in the latest commit. Thank you for making the updates.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the latest changes to the `add_edge` method in the `Grapher` class
ast-grep --lang python --pattern $'class Grapher {
$$$
def add_edge($_, $_) {
$$$
}
$$$
}'
Length of output: 450
Script:
#!/bin/bash
# Fetch the latest changes to the `add_edge` method in the `Grapher` class
ast-grep --lang python --pattern $'class Grapher {
def add_edge(self, v: str, w: str) -> None:
$$$
}'
Length of output: 602
Script:
#!/bin/bash
# Fetch the latest changes to the `add_edge` method in the `Grapher` class
ast-grep --lang python --pattern $'def add_edge(self, v: str, w: str) -> None:
$$$
'
Length of output: 571
bundled/tool/zenml_grapher.py
Outdated
| def add_artifacts_from_list(self, list) -> None: | ||
| """Used to add unique artifacts to the internal nodes list by build_nodes_from_steps""" | ||
| for artifact in list: | ||
| id = str(list[artifact].body.artifact.id) | ||
| if id in self.artifacts: | ||
| continue | ||
|
|
||
| self.artifacts[id] = True | ||
|
|
||
| self.nodes.append({ | ||
| "type": "artifact", | ||
| "id": id, | ||
| "data": { | ||
| "name": artifact, | ||
| "artifact_type": list[artifact].body.type, | ||
| "execution_id": str(list[artifact].id), | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Improve add_artifacts_from_list Method.
- The variable
listis poorly named and does not convey its purpose. Consider renaming it toartifacts_listor similar. - The method lacks type hints which would improve readability and maintainability.
- Using a dictionary for
self.artifactsto check for duplicates is fine as discussed previously, but ensure this aligns with the overall design philosophy of the application.
- def add_artifacts_from_list(self, list) -> None:
+ def add_artifacts_from_list(self, artifacts_list: Dict[str, Artifact]) -> None:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_artifacts_from_list(self, list) -> None: | |
| """Used to add unique artifacts to the internal nodes list by build_nodes_from_steps""" | |
| for artifact in list: | |
| id = str(list[artifact].body.artifact.id) | |
| if id in self.artifacts: | |
| continue | |
| self.artifacts[id] = True | |
| self.nodes.append({ | |
| "type": "artifact", | |
| "id": id, | |
| "data": { | |
| "name": artifact, | |
| "artifact_type": list[artifact].body.type, | |
| "execution_id": str(list[artifact].id), | |
| }, | |
| }) | |
| def add_artifacts_from_list(self, artifacts_list: Dict[str, Artifact]) -> None: | |
| """Used to add unique artifacts to the internal nodes list by build_nodes_from_steps""" | |
| for artifact in artifacts_list: | |
| id = str(artifacts_list[artifact].body.artifact.id) | |
| if id in self.artifacts: | |
| continue | |
| self.artifacts[id] = True | |
| self.nodes.append({ | |
| "type": "artifact", | |
| "id": id, | |
| "data": { | |
| "name": artifact, | |
| "artifact_type": artifacts_list[artifact].body.type, | |
| "execution_id": str(artifacts_list[artifact].id), | |
| }, | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- src/test/ts_tests/extension.test.ts (1 hunks)
Additional context used
Path-based instructions (1)
src/test/ts_tests/extension.test.ts (1)
Pattern
**/*.ts: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
Additional comments not posted (1)
src/test/ts_tests/extension.test.ts (1)
31-34: Ensure proper initialization ofcontextMockproperties.The added properties
extensionPathandextensionUritocontextMockare initialized with default values. This change is necessary for testing environments that simulate the extension's behavior more accurately.However, consider adding checks to ensure these properties are utilized correctly elsewhere in your tests or extension logic.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (6)
bundled/tool/zenml_grapher.py (3)
18-26: Initial Class Setup: Consider Adding Docstrings for ClarityThe
Grapherclass is initialized with arunobject, but the constructor lacks a docstring that explains the parameters and the expected type ofrun. This could improve readability and maintainability, especially for new developers or external contributors.def __init__(self, run): + """ + Initializes the Grapher object with a pipeline run. + + Args: + run (PipelineRun): The pipeline run data. + """ self.run = run
47-64: Methodadd_artifacts_from_list: Improve Variable Naming and Redundancy HandlingThe method uses
dictOfArtifactswhich could be renamed toartifacts_dictfor better readability. Also, it uses a dictionary to track unique artifacts, which is fine, but consider explaining why this is necessary in the docstring.- def add_artifacts_from_list(self, dictOfArtifacts: Dict[str, StepArtifact]) -> None: + def add_artifacts_from_list(self, artifacts_dict: Dict[str, StepArtifact]) -> None: + """ + Adds unique artifacts to the internal nodes list. Ensures no duplicate artifacts are added. + Args: + artifacts_dict (Dict[str, StepArtifact]): Dictionary of artifacts to add. + """ for artifact in artifacts_dict: id = str(artifacts_dict[artifact].body.artifact.id) if id in self.artifacts: continue self.artifacts[id] = True
84-90: Methodadd_edge: Add Type Hints and Improve DocstringThe
add_edgemethod lacks detailed documentation on its parameters and their types. Adding type hints and a more descriptive docstring could improve code readability and maintainability.- def add_edge(self, v: str, w: str) -> None: + def add_edge(self, v: str, w: str) -> None: + """ + Adds an edge between two nodes identified by their IDs. + + Args: + v (str): ID of the source node. + w (str): ID of the target node. + """ self.edges.append({ "id": f"{v}_{w}", "source": v, "target": w, })README.md (3)
12-12: Grammar Suggestion: Add Comma for ClarityConsider adding a comma after "directly" to improve the readability of the sentence.
- Explore Directed Acyclic Graphs for each pipeline view directly directly on the Activity Bar. + Explore Directed Acyclic Graphs for each pipeline view directly, directly on the Activity Bar.Tools
LanguageTool
[uncategorized] ~12-~12: Consider adding a comma between these intensifiers.
Context: ...d Acyclic Graphs for each pipeline view directly directly on the Activity Bar. - **Python Tool In...(RB_RB_COMMA)
31-31: SectionPipeline Runs: Clarify Feature DescriptionThe description of the pipeline runs could be expanded to provide more details on what managing and monitoring entails, enhancing user understanding.
- **Pipeline Runs**: Monitor and manage pipeline runs, including deleting runs from the system and rendering DAGs. + **Pipeline Runs**: Monitor and manage pipeline runs, including options to delete runs from the system, render DAGs, and view detailed run statistics.
34-45: SectionDAG Visualization: Typographical and Clarity EnhancementsSeveral typographical and clarity enhancements can be made to improve the presentation and readability of this section.
- click on the Render Dag context action (labeled 1 in above image) next to the pipeline run you want to render. This will render the DAG in the editor window. + Click on the 'Render DAG' context action (labeled '1' in the above image) next to the pipeline run you want to render. This action renders the DAG in the editor window. - Panning the graph can be done by clicking and dragging anywhere on the graph. + You can pan the graph by clicking and dragging anywhere on the graph. - Zooming can be controlled by the mousewheel, the control panel (labeled 2 in the above graph) or double-clicking anywhere there is not a node. + Zooming can be controlled by the mouse wheel, the control panel (labeled '2' in the above graph), or by double-clicking anywhere there is not a node. - Mousing over a node will highlight all edges being output by that node + Hovering over a node will highlight all edges being output by that node. - Clicking a node will display the data related to it in the ZenML panel view (labeled 3 in the above image) + Clicking a node displays the data related to it in the ZenML panel view (labeled '3' in the above image). - Double-clicking a node will open the dashboard in a web browser to either the pipeline run or the artifact version. + Double-clicking a node opens the dashboard in a web browser, either to the pipeline run or the artifact version.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- README.md (2 hunks)
- bundled/tool/type_hints.py (1 hunks)
- bundled/tool/zenml_client.py (2 hunks)
- bundled/tool/zenml_grapher.py (1 hunks)
- bundled/tool/zenml_wrappers.py (2 hunks)
Files skipped from review due to trivial changes (1)
- bundled/tool/zenml_client.py
Files skipped from review as they are similar to previous changes (1)
- bundled/tool/zenml_wrappers.py
Additional context used
Path-based instructions (2)
bundled/tool/type_hints.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."bundled/tool/zenml_grapher.py (1)
Pattern
**/*.py: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
Learnings (1)
bundled/tool/zenml_grapher.py (1)
Learnt from: Christopher-R-Perkins PR: zenml-io/vscode-zenml#25 File: bundled/tool/zenml_grapher.py:81-87 Timestamp: 2024-07-15T15:39:04.231Z Learning: When building edges from step data within a pipeline run response, duplicates are not expected, as the edges map input->step and step->output.
LanguageTool
README.md
[uncategorized] ~12-~12: Consider adding a comma between these intensifiers.
Context: ...d Acyclic Graphs for each pipeline view directly directly on the Activity Bar. - **Python Tool In...(RB_RB_COMMA)
Additional comments not posted (8)
bundled/tool/type_hints.py (8)
5-7: Type Definition Approved:StepArtifactBodyThe definition is clear and follows Python best practices for structured data representation.
9-11: Type Definition Approved:StepArtifactThis definition is well-structured, using
UUIDfor unique identifiers and a nestedStepArtifactBody, enhancing clarity and maintainability.
13-16: Type Definition Approved:GraphNodeThe structure is typical and appropriate for graph nodes, enhancing clarity and ease of data manipulation in graph-related operations.
18-21: Type Definition Approved:GraphEdgeThis definition is clear and typical for graph edges, facilitating easy identification and linkage in graph operations.
25-30: Type Definition Approved:GraphResponseThis comprehensive structure effectively encapsulates all necessary components of a graph response, promoting a clear and efficient data handling approach.
32-33: Type Definition Approved:ErrorResponseThe structure is straightforward and typical for API error responses, facilitating clear and effective error handling.
35-48: Type Definition Approved:RunStepResponseThis definition is comprehensive, covering various aspects of a run step with appropriate use of optional fields, enhancing data handling flexibility.
50-58: Type Definition Approved:RunArtifactResponseThis definition is comprehensive, effectively covering all necessary details for a run artifact, with flexibility in metadata handling.
| def build_nodes_from_steps(self) -> None: | ||
| """Builds internal node list from run steps""" | ||
| self.nodes = [] | ||
| self.artifacts = {} | ||
|
|
||
| for step in self.run.metadata.steps: | ||
| step_data = self.run.metadata.steps[step] | ||
| self.nodes.append({ | ||
| "id": str(step_data.id), | ||
| "type": "step", | ||
| "data": { | ||
| "execution_id": str(step_data.id), | ||
| "name": step, | ||
| "status": step_data.body.status, | ||
| }, | ||
| }) | ||
| self.add_artifacts_from_list(step_data.body.inputs) | ||
| self.add_artifacts_from_list(step_data.body.outputs) | ||
|
|
There was a problem hiding this comment.
Method build_nodes_from_steps: Optimization and Redundancy Concern
Each call to build_nodes_from_steps resets self.nodes and self.artifacts, which could lead to inefficiencies if the method is called multiple times within the same context. Consider maintaining state across calls or restructuring how this method is invoked to prevent unnecessary reinitializations.
def build_nodes_from_steps(self) -> None:
"""Builds internal node list from run steps"""
- self.nodes = []
- self.artifacts = {}
+ if not self.nodes and not self.artifacts:
+ self.nodes = []
+ self.artifacts = {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def build_nodes_from_steps(self) -> None: | |
| """Builds internal node list from run steps""" | |
| self.nodes = [] | |
| self.artifacts = {} | |
| for step in self.run.metadata.steps: | |
| step_data = self.run.metadata.steps[step] | |
| self.nodes.append({ | |
| "id": str(step_data.id), | |
| "type": "step", | |
| "data": { | |
| "execution_id": str(step_data.id), | |
| "name": step, | |
| "status": step_data.body.status, | |
| }, | |
| }) | |
| self.add_artifacts_from_list(step_data.body.inputs) | |
| self.add_artifacts_from_list(step_data.body.outputs) | |
| def build_nodes_from_steps(self) -> None: | |
| """Builds internal node list from run steps""" | |
| if not self.nodes and not self.artifacts: | |
| self.nodes = [] | |
| self.artifacts = {} | |
| for step in self.run.metadata.steps: | |
| step_data = self.run.metadata.steps[step] | |
| self.nodes.append({ | |
| "id": str(step_data.id), | |
| "type": "step", | |
| "data": { | |
| "execution_id": str(step_data.id), | |
| "name": step, | |
| "status": step_data.body.status, | |
| }, | |
| }) | |
| self.add_artifacts_from_list(step_data.body.inputs) | |
| self.add_artifacts_from_list(step_data.body.outputs) |
| def build_edges_from_steps(self) -> None: | ||
| """Builds internal edges list from run steps""" | ||
| self.edges = [] | ||
|
|
||
| for step in self.run.metadata.steps: | ||
| step_data = self.run.metadata.steps[step] | ||
| step_id = str(step_data.id) | ||
|
|
||
| for artifact in step_data.body.inputs: | ||
| input_id = str(step_data.body.inputs[artifact].body.artifact.id) | ||
| self.add_edge(input_id, step_id) | ||
|
|
||
| for artifact in step_data.body.outputs: | ||
| output_id = str(step_data.body.outputs[artifact].body.artifact.id) | ||
| self.add_edge(step_id, output_id) | ||
|
|
There was a problem hiding this comment.
Method build_edges_from_steps: Efficiency Concern
This method builds edges from steps but does not check for duplicates, which could lead to redundant data in self.edges. Consider implementing a check to prevent duplicate edges from being added, especially since this could impact performance and memory usage with large datasets.
def add_edge(self, v: str, w: str) -> None:
"""Helper method to add an edge to the internal edges list"""
+ edge_id = f"{v}_{w}"
+ if any(edge['id'] == edge_id for edge in self.edges):
+ return
self.edges.append({
"id": edge_id,
"source": v,
"target": w,
})Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def build_edges_from_steps(self) -> None: | |
| """Builds internal edges list from run steps""" | |
| self.edges = [] | |
| for step in self.run.metadata.steps: | |
| step_data = self.run.metadata.steps[step] | |
| step_id = str(step_data.id) | |
| for artifact in step_data.body.inputs: | |
| input_id = str(step_data.body.inputs[artifact].body.artifact.id) | |
| self.add_edge(input_id, step_id) | |
| for artifact in step_data.body.outputs: | |
| output_id = str(step_data.body.outputs[artifact].body.artifact.id) | |
| self.add_edge(step_id, output_id) | |
| def build_edges_from_steps(self) -> None: | |
| """Builds internal edges list from run steps""" | |
| self.edges = [] | |
| for step in self.run.metadata.steps: | |
| step_data = self.run.metadata.steps[step] | |
| step_id = str(step_data.id) | |
| for artifact in step_data.body.inputs: | |
| input_id = str(step_data.body.inputs[artifact].body.artifact.id) | |
| self.add_edge(input_id, step_id) | |
| for artifact in step_data.body.outputs: | |
| output_id = str(step_data.body.outputs[artifact].body.artifact.id) | |
| self.add_edge(step_id, output_id) | |
| def add_edge(self, v: str, w: str) -> None: | |
| """Helper method to add an edge to the internal edges list""" | |
| edge_id = f"{v}_{w}" | |
| if any(edge['id'] == edge_id for edge in self.edges): | |
| return | |
| self.edges.append({ | |
| "id": edge_id, | |
| "source": v, | |
| "target": w, | |
| }) |
This pull request focuses on adding a feature to render Directed Acyclic Graphs for pipeline runs.
Changes
New Render DAG context action on Pipeline Runs
Added Render DAG context action on Pipeline Run items:
getPipelineRun,getPipelineRunStep,getPipelineRunArtifact, andgetPipelineRunDagactions to Python Tool to support this actionzenml/lineage_graphto python tool to quickly build graph response without added client calls for artifacts (directly using LineageGraph locally took upwards of 90s on more complex runs, while using API wouldn't allow local/no server graphs. +API still feels slower than current solution)Added ZenML data view to Panel
Fixes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
requirements.txt.Documentation
Style