-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZenML 0.80.0 Migration and Project View Implementation #130
Conversation
…pace/project id along with server info
…jects in python tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/views/activityBar/projectView/ProjectDataProvider.ts (1)
62-70
: Inline handler logic is straightforward.
The active project update call is concise. Consider adding minimal logging to streamline debugging if unexpected changes occur.bundled/tool/zenml_wrappers.py (4)
150-157
: Update docstring to reflect the additional parameterThe method docstring doesn't mention the new
projects_wrapper
parameter. Consider updating it to include this information for better clarity.- """Initializes ZenServerWrapper with a configuration wrapper.""" + """Initializes ZenServerWrapper with a configuration wrapper and projects wrapper."""
231-235
: Redundant type check can be simplifiedThe
isinstance(active_project, dict)
check is redundant since we've already confirmed thatactive_project
exists and doesn't have an error key. Theget_active_project()
method always returns a dictionary due to the@serialize_response
decorator.- if active_project and not active_project.get("error"): - if isinstance(active_project, dict): - active_project_id = active_project.get("id") - active_project_name = active_project.get("name") + if active_project and not active_project.get("error"): + active_project_id = active_project.get("id") + active_project_name = active_project.get("name")
637-666
: Improve validation in get_active_workspace methodThe method should validate the Pro API URL and provide a clearer implementation for determining the active workspace. Simply assuming the first workspace in the list is the active one may not be accurate.
@serialize_response def get_active_workspace(self) -> Union[Dict[str, str], ErrorResponse]: """Gets the active workspace for the current user. Returns: A dictionary containing the active workspace information or an error message. """ try: # Initialize Pro client for workspace access store_info = self.gc.zen_store.get_store_info() pro_api_url = store_info.get("pro_api_url") + + if not pro_api_url: + return {"error": "Pro API URL is not available"} pro_client = self.ZenMLProClient(url=pro_api_url) - # Get the first workspace as the active one - workspaces = pro_client.workspace.list(limit=1) + # Try to get the active workspace from the global configuration + active_workspace_name = self.gc.active_workspace_name + + # If we have an active workspace name, get that specific workspace + if active_workspace_name: + try: + workspace = pro_client.workspace.get(active_workspace_name) + return { + "id": str(workspace.id), + "name": workspace.name, + "organization_id": str(workspace.organization_id), + } + except self.ZenMLBaseException: + # If getting the specific workspace fails, fall back to the list approach + pass + + # Fall back to getting the first workspace if no active workspace is set + workspaces = pro_client.workspace.list(limit=1) if not workspaces or len(workspaces) == 0: return {"error": "No workspaces found"} workspace = workspaces[0] return { "id": str(workspace.id), "name": workspace.name, "organization_id": str(workspace.organization_id), } except self.ZenMLBaseException as e: return {"error": f"Failed to get active workspace: {str(e)}"} except Exception as e: return {"error": f"Unexpected error getting active workspace: {str(e)}"}
778-802
: Enhance get_project_by_name with client-side argument validationWhile the method handles exceptions well, consider adding validation for the
project_name
parameter to ensure it's not empty or None before making the API call.@serialize_response def get_project_by_name(self, project_name: str) -> Union[Dict[str, str], ErrorResponse]: """Gets a project by name. Args: project_name: The name of the project to get Returns: A dictionary containing the project information or an error message. """ try: + if not project_name: + return {"error": "Project name is required"} + project = self.client.get_project(project_name) return { "id": str(project.id), "name": project.name, "display_name": project.body.display_name, "created": project.body.created.isoformat(), "updated": project.body.updated.isoformat(), } except self.ZenMLBaseException as e: return {"error": f"Failed to get project by name: {str(e)}"} except Exception as e: return {"error": f"Unexpected error getting project by name: {str(e)}"}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bundled/tool/zenml_wrappers.py
(8 hunks)src/services/LSClient.ts
(9 hunks)src/views/activityBar/pipelineView/PipelineDataProvider.ts
(6 hunks)src/views/activityBar/projectView/ProjectDataProvider.ts
(1 hunks)src/views/activityBar/stackView/StackDataProvider.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/activityBar/stackView/StackDataProvider.ts
- src/views/activityBar/pipelineView/PipelineDataProvider.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
**/*.ts
: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/activityBar/projectView/ProjectDataProvider.ts
src/services/LSClient.ts
`**/*.py`: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
**/*.py
: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
bundled/tool/zenml_wrappers.py
🧬 Code Definitions (3)
src/views/activityBar/projectView/ProjectDataProvider.ts (2)
src/services/LSClient.ts (1) (1)
LSClient
(34-275)src/views/activityBar/projectView/ProjectTreeItems.ts (1) (1)
ProjectTreeItem
(34-85)
src/services/LSClient.ts (3)
src/utils/constants.ts (1) (1)
LSP_ZENML_PROJECT_CHANGED
(29-29)src/types/ServerInfoTypes.ts (1) (1)
ConfigUpdateDetails
(46-50)src/commands/stack/utils.ts (1) (1)
storeActiveStack
(76-79)
bundled/tool/zenml_wrappers.py (1)
bundled/tool/type_hints.py (4) (4)
ListPipelineRunsResponse
(171-177)ListProjectsResponse
(210-215)ListWorkspacesResponse
(194-198)ErrorResponse
(58-59)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (23)
src/services/LSClient.ts (6)
15-15
: Good addition of project change supportThe addition of the
storeActiveProject
import and theLSP_ZENML_PROJECT_CHANGED
constant properly prepares the class for handling project change notifications, which aligns with the PR's objective of supporting the new Project View implementation.Also applies to: 24-24
57-61
: Good refactoring to arrow functionsConverting these notification handlers to arrow functions is a good practice as it ensures
this
context is preserved when the methods are used as callbacks, preventing potentialthis
binding issues.
59-59
: Appropriate integration of project change notificationAdding the notification listener for
LSP_ZENML_PROJECT_CHANGED
correctly integrates the new functionality with the existing notification system, providing consistent behavior with other notification types.
88-96
: Consistent method refactoring to arrow functionsThe consistent refactoring of all handler methods to use arrow function syntax improves code maintainability and prevents
this
binding issues. This is particularly important for class methods used as callbacks in event handlers.Also applies to: 104-114, 121-142, 168-172, 250-253
173-184
: Well-implemented project change handlerThe
handleProjectChanged
method follows the established pattern of the other handler methods. It properly logs the notification, stores the updated project state, and emits an event to notify other parts of the system, which is essential for the Project View implementation mentioned in the PR objectives.
270-273
: Minor formatting improvementThe minor formatting changes in the
getInstance
method improve code readability while maintaining the singleton pattern implementation.src/views/activityBar/projectView/ProjectDataProvider.ts (12)
1-12
: License setup looks correct.
No concerns with how the Apache 2.0 license header is added.
29-39
: Singleton instantiation and constructor logic appear consistent.
The constructor sets a default loading state and subscribes to events immediately, aligning well with the singleton pattern.
41-50
: Good pattern for event subscription management.
Unsubscribing before subscribing prevents duplicate listeners. The arrow function references are stable and correctly matched.
52-60
: Efficient loading state trigger.
This approach promptly updates the UI with a loading indicator, enhancing user feedback without blocking.
71-82
: LS client state management.
Refreshing the view only when the client state is running is a sensible choice. Ensure edge cases (e.g., transition from Stopped to Starting) are tested.
84-99
: ZenML client readiness check.
Reusing the loading state before the client is initialized is a clean solution. Re-subscribing toLSP_ZENML_PROJECT_CHANGED
after initialization also appears well-managed.
101-111
: Singleton access method is fine.
Using the class name instead ofthis
helps avoid confusion in static context. No further changes needed.
113-132
: Refresh logic handles errors gracefully.
Notably, the code sets loading state first, attempts to fetch data, and then falls back to error items if necessary—robust and user-friendly.
134-203
: Reliable fetchProjects method with pagination and error handling.
- Good fallback for authentication errors and unexpected responses.
- Logging the error before returning ensures better troubleshooting.
- The usage of typed responses clarifies the structure for maintainers.
205-223
: getChildren method is neatly structured.
Directly returningthis.items
for the root, or the children of aProjectTreeItem
, is consistent with VS Code’s tree data pattern.
225-239
: Active project check is straightforward.
Comparing names from config with the project name is a clean approach. This logic is easy to maintain and test.
241-274
: Efficient partial refresh for active project updates.
- Avoids refetching all data, thus improving performance for project switching.
- Firing tree change events only for the updated item is also a good pattern to minimize UI refresh overhead.
bundled/tool/zenml_wrappers.py (5)
345-359
: Good error handling for missing argumentsThe implementation properly validates required arguments before proceeding and returns an appropriate error response. The method signature has been updated correctly with appropriate type annotations.
590-636
: Improve error handling specificity in list_workspaces.The method uses a catch-all Exception which could mask implementation bugs. Also, the pagination data uses
len(workspaces_data)
for the total count which may not be accurate if there are more workspaces than returned in a single request.-try: - store_info = self.gc.zen_store.get_store_info() - pro_api_url = store_info.get("pro_api_url") - - # Initialize Pro client for workspace access - pro_client = self.ZenMLProClient(url=pro_api_url) - - offset = args[0] if len(args) > 0 else 0 - limit = args[1] if len(args) > 1 else 10 - - workspaces = pro_client.workspace.list(offset=offset, limit=limit) - - workspaces_data = [ - { - "id": str(workspace.id), - "name": workspace.name, - "description": workspace.description, - "display_name": workspace.display_name, - "organization_id": str(workspace.organization.id), - "organization_name": workspace.organization.name, - "status": workspace.status, - "zenml_version": workspace.version, - "zenml_server_url": workspace.url, - "dashboard_url": workspace.dashboard_url, - "dashboard_organization_url": workspace.dashboard_organization_url, - } - for workspace in workspaces - ] - - return { - "workspaces": workspaces_data, - "total": len(workspaces_data), - "offset": offset, - "limit": limit, - } -except Exception as e: - return {"error": f"Failed to list workspaces: {str(e)}"} +try: + store_info = self.gc.zen_store.get_store_info() + pro_api_url = store_info.get("pro_api_url") + + if not pro_api_url: + return {"error": "Pro API URL is not available"} + + # Initialize Pro client for workspace access + pro_client = self.ZenMLProClient(url=pro_api_url) + + offset = args[0] if len(args) > 0 else 0 + limit = args[1] if len(args) > 1 else 10 + + workspaces = pro_client.workspace.list(offset=offset, limit=limit) + + workspaces_data = [ + { + "id": str(workspace.id), + "name": workspace.name, + "description": workspace.description, + "display_name": workspace.display_name, + "organization_id": str(workspace.organization.id), + "organization_name": workspace.organization.name, + "status": workspace.status, + "zenml_version": workspace.version, + "zenml_server_url": workspace.url, + "dashboard_url": workspace.dashboard_url, + "dashboard_organization_url": workspace.dashboard_organization_url, + } + for workspace in workspaces + ] + + # Use total count from API response if available + total_count = getattr(workspaces, 'total', len(workspaces_data)) + + return { + "workspaces": workspaces_data, + "total": total_count, + "offset": offset, + "limit": limit, + } +except self.ZenMLBaseException as e: + return {"error": f"Failed to list workspaces: {str(e)}"} +except Exception as e: + return {"error": f"Unexpected error listing workspaces: {str(e)}"}
678-723
: Good implementation of list_projectsThe method properly handles pagination, provides appropriate default values, and has structured error handling. The response structure aligns well with the type definitions in
type_hints.py
.
724-745
: Good error handling in get_active_projectThis method appropriately handles both ZenML-specific exceptions and generic exceptions separately, providing detailed error messages in each case.
746-777
: Proper argument validation in set_active_projectThe method correctly validates that the required project_id argument is provided before proceeding with any operations. Error handling is comprehensive with specific error messages.
… steps, config, and model in tree view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundled/tool/zenml_wrappers.py (1)
659-704
: 🛠️ Refactor suggestionImprove error handling specificity in list_workspaces.
The method uses a catch-all Exception which could mask implementation bugs. Also, the pagination data uses
len(workspaces_data)
for the total count which may not be accurate if there are more workspaces than returned in a single request.try: store_info = self.gc.zen_store.get_store_info() pro_api_url = store_info.get("pro_api_url") + if not pro_api_url: + return {"error": "Pro API URL is not available"} + # Initialize Pro client for workspace access pro_client = self.ZenMLProClient(url=pro_api_url) offset = args[0] if len(args) > 0 else 0 limit = args[1] if len(args) > 1 else 10 workspaces = pro_client.workspace.list(offset=offset, limit=limit) workspaces_data = [ # ... existing code ... ] + # Use total count from API response if available + total_count = getattr(workspaces, 'total', len(workspaces_data)) + return { "workspaces": workspaces_data, - "total": len(workspaces_data), + "total": total_count, "offset": offset, "limit": limit, } -except Exception as e: +except self.ZenMLBaseException as e: return {"error": f"Failed to list workspaces: {str(e)}"} +except Exception as e: + return {"error": f"Unexpected error listing workspaces: {str(e)}"}
🧹 Nitpick comments (4)
bundled/tool/type_hints.py (1)
194-198
: Note inconsistent pagination patterns between responses.There's an inconsistency in pagination approaches between ListWorkspacesResponse (using offset/limit) and ListProjectsResponse (using page numbers). This might cause confusion for developers integrating with both APIs.
class ListWorkspacesResponse(TypedDict): workspaces: List[Workspace] total: int - offset: int - limit: int + total_pages: int + current_page: int + items_per_page: intAlso applies to: 210-215
src/views/activityBar/pipelineView/PipelineTreeItems.ts (2)
45-45
: Doc comment alignment
The updated doc comment better reflects the properties displayed. If additional information (e.g., timestamp or step-level data) becomes relevant, consider updating the comment to stay in sync.
55-56
: Tooltip usage
Setting an empty tooltip can reduce clutter if the context is already obvious. Alternatively, consider including run details if there's added value for users hovering over the item.src/types/PipelineTypes.ts (1)
40-47
: Defining PipelineModel
This interface consolidates key metadata for models used in pipelines. Make sure that versioning and licensing details are consistently handled wherever they're consumed.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.json
is excluded by!**/package-lock.json
resources/logo.svg
is excluded by!**/*.svg
📒 Files selected for processing (13)
bundled/tool/type_hints.py
(2 hunks)bundled/tool/zenml_wrappers.py
(10 hunks)package.json
(9 hunks)src/types/PipelineTypes.ts
(1 hunks)src/types/ProjectTypes.ts
(1 hunks)src/types/QuickPickItemTypes.ts
(1 hunks)src/types/ServerInfoTypes.ts
(2 hunks)src/utils/constants.ts
(3 hunks)src/views/activityBar/pipelineView/PipelineDataProvider.ts
(4 hunks)src/views/activityBar/pipelineView/PipelineTreeItems.ts
(2 hunks)src/views/activityBar/stackView/StackDataProvider.ts
(3 hunks)src/views/activityBar/stackView/StackTreeItems.ts
(1 hunks)src/views/statusBar/index.ts
(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
**/*.ts
: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/activityBar/stackView/StackTreeItems.ts
src/types/QuickPickItemTypes.ts
src/types/ServerInfoTypes.ts
src/utils/constants.ts
src/views/activityBar/pipelineView/PipelineTreeItems.ts
src/views/activityBar/stackView/StackDataProvider.ts
src/types/ProjectTypes.ts
src/types/PipelineTypes.ts
src/views/statusBar/index.ts
src/views/activityBar/pipelineView/PipelineDataProvider.ts
`**/*.py`: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
**/*.py
: "Review the Python code for conformity with Python best practices and industry standards, highlighting any deviations."
bundled/tool/type_hints.py
bundled/tool/zenml_wrappers.py
🧬 Code Definitions (3)
src/views/activityBar/stackView/StackDataProvider.ts (2)
src/utils/constants.ts (3)
LSCLIENT_STATE_CHANGED
(35-35)LSP_ZENML_CLIENT_INITIALIZED
(27-27)LSP_ZENML_STACK_CHANGED
(29-29)src/views/activityBar/common/LoadingTreeItem.ts (1)
LOADING_TREE_ITEMS
(23-32)
src/views/activityBar/pipelineView/PipelineDataProvider.ts (7)
src/views/activityBar/common/PaginatedDataProvider.ts (1)
PaginatedDataProvider
(24-154)src/utils/constants.ts (3)
LSCLIENT_STATE_CHANGED
(35-35)LSP_ZENML_CLIENT_INITIALIZED
(27-27)LSP_ZENML_PROJECT_CHANGED
(30-30)src/views/activityBar/common/LoadingTreeItem.ts (1)
LOADING_TREE_ITEMS
(23-32)src/types/PipelineTypes.ts (6)
PipelineRunsResponse
(98-98)PipelineRunsData
(16-22)PipelineRun
(49-61)PipelineRunStep
(24-29)PipelineModel
(40-47)PipelineRunConfig
(31-38)src/services/LSClient.ts (1)
LSClient
(34-275)src/views/activityBar/common/ErrorTreeItem.ts (2)
createAuthErrorItem
(60-78)createErrorItem
(45-52)src/views/activityBar/common/TreeItemWithChildren.ts (1)
TreeItemWithChildren
(19-21)
bundled/tool/zenml_wrappers.py (1)
bundled/tool/type_hints.py (4)
ListPipelineRunsResponse
(171-177)ListProjectsResponse
(210-215)ListWorkspacesResponse
(194-198)ErrorResponse
(58-59)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: build-and-test
🔇 Additional comments (107)
package.json (14)
28-28
: VS Code Engine Version Downgrade:
The VS Code engine version is now set to"^1.93.0"
, which ensures compatibility with VS Code 1.93 and above (including Cursor IDE). Please verify that none of the extension features inadvertently rely on APIs introduced after 1.93.
141-145
: New Configuration Property: Active Project Name:
The"zenml.activeProjectName"
property is added with a default value of an empty string and an appropriate description. Ensure that this new setting is correctly integrated with the extension logic (for example, within the project view and status bar updates) and covered by tests.
159-164
: New Command: Set Projects Per Page:
The addition of the"zenml.setProjectItemsPerPage"
command under the "ZenML Projects" category provides a way to control project pagination. Confirm that this command triggers the proper logic when invoked from the UI.
166-170
: New Command: Refresh Project View:
The"zenml.refreshProjectView"
command is now available to refresh the projects view. Please ensure that it calls the correct function to update the UI and handles any potential errors gracefully.
172-176
: New Command: Set Active Project:
The command"zenml.setActiveProject"
is introduced with an appropriate icon and categorization. Verify that this command properly updates the active project state within the extension and that the change is reflected in the UI (e.g., status bar indicators).
178-182
: New Command: Go to Project URL:
The"zenml.goToProjectUrl"
command enables users to navigate to the project URL. It is important to check that the URL is constructed correctly (e.g., using the updated project IDs) and that error states (if the URL is invalid) are handled appropriately.
359-363
: New Project View in Activity Bar:
A new view with"id": "zenmlProjectView"
has been added to present projects, using the"$(symbol-variable)"
icon. Ensure that the associated data provider correctly populates the view and that event listeners are properly set up and cleaned up for performance.
334-337
: Updated Activity Bar Container Icon:
Within the"activitybar"
container, the icon is updated to"resources/logo.svg"
. Confirm that this icon aligns with the overall branding and provides clear visual cues consistent with the extension’s design.
405-408
: Menu Entry: Set Project Items Per Page:
The menu item for"zenml.setProjectItemsPerPage"
in theview/title
section (conditional onprojectCommandsRegistered && view == zenmlProjectView
) ensures easy access from the project view. Verify that the condition accurately reflects when the command should be available.
409-413
: Menu Entry: Refresh Project View:
This menu item ties the"zenml.refreshProjectView"
command to the project view title. Please confirm that invoking this menu item properly refreshes the project data and updates the UI.
461-470
: Context Menu for Project Items:
New context menu entries for"zenml.setActiveProject"
and"zenml.goToProjectUrl"
have been added underview/item/context
with conditions targeting project items. Verify that these commands appear only in the context of valid project items and that they perform the expected actions when selected.
538-539
: Updated @types/vscode Dependency:
The"@types/vscode"
dependency has been downgraded to"^1.93.0"
to align with the engine version. Make sure that no newer APIs are inadvertently used in the extension that might not be covered by these types.
542-543
: Updated @vscode/vsce Dependency:
The version for"@vscode/vsce"
is updated to"^3.3.0"
. Verify that this version is compatible with the current build and publishing process.
558-559
: Dependency Addition: Axios:
The inclusion of"axios": "^1.8.4"
as a dependency should be reviewed to ensure that its usage in the project is consistent and that there are no breaking changes relative to previous versions.bundled/tool/type_hints.py (5)
16-25
: Good addition of flexible MetadataType.The
MetadataType
union provides a comprehensive type definition for metadata fields, supporting both primitive types and collections. This facilitates proper type hinting while maintaining flexibility for various metadata formats.
159-169
: LGTM on PipelineRun definition.The PipelineRun TypedDict provides a clear structure for pipeline run data with appropriate optional fields for timestamps and configuration.
171-177
: PaginatedResponse pattern applied correctly.The ListPipelineRunsResponse correctly includes both the data collection and pagination metadata. The optional project_name is a good addition for filtering contexts.
180-192
: Comprehensive Workspace type definition.The Workspace TypedDict includes all necessary fields for workspace management including organization relationships and relevant URLs.
201-208
: Good use of MetadataType for Project.The Project TypedDict makes appropriate use of the new MetadataType for flexible metadata handling. Optional fields allow for varying levels of detail in project representations.
bundled/tool/zenml_wrappers.py (13)
13-22
: Excellent module documentation improvement.The expanded docstring clearly explains the file organization with section numbering, making navigation easier in this large file. This helps developers understand the purpose and structure of the module.
27-39
: LGTM on updated imports.The imports have been properly updated to include the new type definitions needed for project and workspace functionality.
48-50
: Great code organization with section headers.The addition of clear section headers with consistent formatting significantly improves readability and organization of this large file, making it easier to navigate and maintain.
Also applies to: 159-161, 338-340, 628-630, 737-739, 876-878
165-172
: Good dependency injection for projects_wrapper.The ZenServerWrapper constructor now accepts a projects_wrapper parameter, enabling proper separation of concerns while allowing access to project functionality.
212-215
: Good lazy import pattern for ZenMLProClient.Using the lazy_import pattern for ZenMLProClient follows the established pattern in the codebase and avoids unnecessary imports until needed.
236-250
: Enhanced server info with project and workspace details.The get_server_info method now includes workspace, project, and organization information, properly supporting the migration to ZenML 0.80.0. The code carefully handles potential None values and properly validates the active_project response.
Also applies to: 262-268
362-382
: Project filtering added to pipeline runs.The fetch_pipeline_runs method now properly supports filtering by project_name, aligning with the project-centric architecture. The argument validation has been improved to prevent index errors.
474-474
: Project context added to runs response.Including the project_name in the response is a good practice as it allows consumers to know which project context the runs belong to.
757-794
: Consistent error handling in list_projects.The method properly uses specific exception handling with ZenMLBaseException first, followed by a general Exception catch, which is better than the approach in list_workspaces. Consider applying the same pattern to list_workspaces for consistency.
797-816
: Well-implemented get_active_project method.The method correctly retrieves and formats the active project information, handling exceptions appropriately. The separation between ZenMLBaseException and general Exception is a good practice.
819-848
: Proper argument validation in set_active_project.The method validates that the required project_id argument is provided before attempting to set the active project, which helps prevent errors and provides clear feedback.
851-873
: Useful get_project_by_name implementation.This method provides a convenient way to retrieve projects by name rather than ID, improving the usability of the API. Error handling follows the established pattern.
707-734
:❓ Verification inconclusive
Simplistic approach to get_active_workspace.
The method assumes the first workspace in the list is the active one, which may not be accurate. Consider checking if there's a more direct way to get the active workspace from the ZenML API.
🏁 Script executed:
#!/bin/bash # Check if ZenML Pro API provides a direct method to get active workspace rg -A 10 "active_workspace" bundled/tool/Length of output: 5086
Active Workspace Retrieval – Revisit Implementation
The current implementation in
get_active_workspace
simply selects the first workspace from the list provided bypro_client.workspace.list(limit=1)
. However, the grep output shows that the codebase already makes use ofstore_info.metadata
to retrieveworkspace_id
andworkspace_name
. This suggests that there may be a more reliable or direct approach available through the ZenML API or the store metadata. Please consider verifying whether the API offers a dedicated method for obtaining the active workspace or if it would be more appropriate to leverage the metadata instore_info
.src/views/activityBar/stackView/StackTreeItems.ts (2)
42-44
: Updated icon for active stack statusThe change from 'pass-filled' to 'layers-active' better aligns with the visual metaphor of stacks, while maintaining the green color to indicate active status.
44-46
: Updated icon for inactive stack statusChanged from 'archive' to 'layers' to maintain visual consistency with the active state, now using a unified icon family that better represents the concept of stacks.
src/types/QuickPickItemTypes.ts (3)
14-25
: Well-structured MainMenuQuickPickItem interfaceThis interface properly extends QuickPickItem with additional properties needed for the main menu. The buttons array with iconPath and tooltip properties will enable rich interactive buttons in the QuickPick UI.
27-31
: Properly defined StackQuickPickItem interfaceThe interface correctly extends QuickPickItem and includes the necessary optional properties to support stack selection functionality.
33-38
: Properly defined ProjectQuickPickItem interfaceThis interface appropriately extends QuickPickItem with properties specific to project selection, aligning with the PR's objective of implementing the new Project View.
src/types/ProjectTypes.ts (5)
16-18
: Flexible MetadataType interfaceThis interface is well-designed to accommodate various types of metadata. Using an index signature with a union type allows for flexible storage of different data types.
20-27
: Comprehensive Project interfaceThe Project interface includes all necessary properties to represent a ZenML project, with appropriate optional flags for fields that may not always be present.
29-35
: Well-structured ProjectsData interface for paginationThis interface properly captures both the array of Project objects and the pagination metadata, which will be essential for handling large collections of projects.
37-41
: Concise GetProjectByNameResponse interfaceThis interface defines the minimal set of properties needed when retrieving a project by name, following the principle of returning only what's necessary.
43-43
: Appropriate union type for ProjectsResponseUsing a union type allows for flexible handling of different response scenarios (success, error, version mismatch), which improves error handling in the codebase.
src/utils/constants.ts (4)
13-13
: Added necessary VS Code importsAdded imports for ThemeColor and ThemeIcon to support the updated PIPELINE_RUN_STATUS_ICONS constant.
19-19
: Updated MIN_ZENML_VERSION to 0.80.0This change aligns with the PR objective of migrating to ZenML version 0.80.0 from 0.75.0.
30-30
: Added LSP_ZENML_PROJECT_CHANGED notification constantThis new constant supports the project view implementation by enabling notifications when the active project changes.
56-62
: Enhanced PIPELINE_RUN_STATUS_ICONS with ThemeIcon objectsImproved the pipeline status icons by using ThemeIcon objects with appropriate ThemeColors. This provides better visual feedback and integrates well with VS Code's theming system.
src/types/ServerInfoTypes.ts (3)
29-35
: Add optional properties for extended project and workspace contexts
The new optional properties for ZenML 0.80.0 appear consistent with the existing naming convention in the interface, and using them as optional fields helps ensure backward compatibility.
38-40
: Introduction of StatusBarServerStatus interface
DefiningStatusBarServerStatus
clarifies the minimal data needed to show server connectivity in the status bar, which is a helpful abstraction for UI updates.
71-77
: Extending ZenServerStoreInfo
These additional optional properties align with the new focus on workspaces, projects, and organization context in ZenML 0.80.0, allowing richer metadata in store configurations.src/views/activityBar/pipelineView/PipelineTreeItems.ts (3)
36-36
: Direct iconPath assignment
UsingiconPath = PIPELINE_RUN_STATUS_ICONS[run.status]
is more concise and avoids unnecessary wrapper objects. This straightforward mapping is a neat improvement.
48-49
: Optional children property for nested runs
Addingpublic children?: PipelineRunTreeItem[];
enables a hierarchical pipeline run display. Ensure you handle possible empty arrays or undefined values in the parent tree logic.
52-53
: New constructor signature
Providing a default collapsible state makes the component more flexible and re-usable in various contexts. Good addition.src/types/PipelineTypes.ts (5)
16-16
: Making PipelineRunsData exportable
Exporting this interface increases its usability in other parts of the codebase, promoting consistency for pipeline data structures.
24-29
: Introducing PipelineRunStep
This interface clearly encapsulates step-level data such asstatus
and timing, improving the granularity of pipeline run tracking.
31-38
: PipelineRunConfig for flexible options
Optional flags likeenable_cache
andenable_step_logs
centralize run configurations, enabling more dynamic behavior per pipeline run.
56-56
: pipelineName property
Substituting OS-specific fields for a direct reference to the pipeline name clarifies the run's main identifier, making the code easier to maintain.
57-60
: Optional config and steps
Allowing bothconfig
andsteps
to be optional acknowledges different run complexities. This design choice supports a wide range of pipeline scenarios.src/views/statusBar/index.ts (15)
13-24
: Expanded imports from VS Code & project utilities
Importing additionalvscode
items along withgetActiveProject
andswitchActiveProject
is crucial for implementing the new Project management workflow within the status bar.
27-41
: EventBus and item type imports
These imports support event-driven stack and project changes, plus QuickPick item definitions. Ensuring each is utilized effectively avoids dead code and streamlines the extension’s logic.
50-50
: New fields for active project and loading states
StoringactiveStack
,activeProjectName
, and the corresponding loading booleans provides clarity and predictability when updating the status bar. Good approach for isolating UI state.Also applies to: 52-54
65-65
: Binding status bar command
Assigning the'zenml/statusBar/openSwitchMenu'
command tostatusBarItem
enables rapid access to project/stack switching straight from the UI element.
72-72
: Registering zenml/statusBar/openSwitchMenu
Registering this command ensures that opening the switch menu is discoverable and can be invoked from the command palette in addition to the status bar.
81-87
: Refined event subscriptions
Removing direct references torefreshActiveStack
andrefreshActiveProject
in favor of dedicated handlers (stackChangeHandler
,projectChangeHandler
) neatly encapsulates update logic.
100-109
: serverStatusUpdateHandler
Centralizing server status updates simplifies maintenance. Listening forSERVER_STATUS_UPDATED
ensures cohesive UI refreshes when connectivity shifts.
117-122
: stackChangeHandler
ComparingactiveStackId
guards against redundant refresh calls, reducing unnecessary network requests or UI updates. Good efficiency pattern.
136-143
: showLoading method
Leveraging the spinner icon to indicate loading states is a helpful visual cue. Ensuring it’s invoked before async updates maintains user awareness of ongoing work.
149-162
: refreshActiveStack
Fetching the current stack name asynchronously and updating the status bar is straightforward. Error handling is adequately surfaced with a console log and fallback label.
165-181
: refreshActiveProject
This mirrors the stack refresh logic for projects, keeping behaviors consistent. Catching errors and assigning "Error" or "(not set)" helps keep the UI informative in failure cases.
183-188
: refresh method
RunningrefreshActiveStack()
andrefreshActiveProject()
in parallel (via Promise.all) optimizes loading time, which benefits user experience.
232-290
: switchStack method
The QuickPick flow for switching stacks is clearly structured—and the active stack is updated in both the data provider and local state. Good error handling ensures user feedback if switching fails.
298-374
: switchProject method
Building a comparable QuickPick flow for projects ensures a coherent UX across stack and project switching. UpdatingProjectDataProvider
and local fields keeps the UI in sync.
379-403
: updateStatusBarItem and tooltip
Combining the active project and stack in the status bar text clarifies the context at a glance. Using Markdown in the tooltip for deeper details is a thoughtful touch.src/views/activityBar/stackView/StackDataProvider.ts (8)
31-31
: No changes detected.
42-52
: Singleton pattern implementation is sound.
No concurrency issues are apparent for this approach in TypeScript.
58-63
: Good practice removing old event listeners before reattaching.
This helps avoid duplicate subscriptions and potential memory leaks.
65-74
: Initialization of a loading state is clear and concise.
Appropriately updating the tree items while firing the change event is good.
80-86
: State check for LSP is well-handled.
Ensures users see a loading or refreshed view based on client state.
93-103
: ZenML initialization logic is well-structured.
Switching between loading and refreshed states is responsive.
110-112
: Stack change handler is straightforward.
Cleanly delegates to updateActiveStack.
227-229
: Uses a distinctive icon for active stacks.
Visually communicates the active vs. inactive stack states effectively.src/views/activityBar/pipelineView/PipelineDataProvider.ts (27)
13-13
: Importing vscode is necessary for TreeItem usage.
16-16
: Importing utility for active project retrieval is appropriate.
19-26
: Expanded imports from PipelineTypes appear relevant.
28-31
: Inclusion of new constants is consistent with LSP events.
35-35
: Import for TreeItemWithChildren is well-placed.
43-43
: activeProjectName field adds clarity.
Helps track context for fetching pipeline runs.
54-64
: Singleton pattern for PipelineDataProvider mirrors best practices.
No obvious threading or concurrency issues.
70-75
: Unsubscribing before re-subscribing prevents duplicate event handlers.
82-85
: triggerLoadingState systematically updates the UI.
92-97
: projectChangeHandler correctly refreshes on project switch.
104-110
: LSP client state handler keeps the UI in sync.
117-127
: ZenML client state logic is coherent.
Event subscription is properly toggled.
135-154
: Optional project parameter in refresh adds flexibility.
Good approach to specific or general refresh calls.
164-168
: fetchPipelineRuns signature updated to accept project name.
Promotes narrower data retrieval.
174-189
: Robust checks for error cases and empty runs.
Proper fallback to icons/messages.
204-223
: getPipelineRunsData method organizes request logic neatly.
231-238
: Error detection logic covers multiple response types.
246-267
: Comprehensive error handling for pipeline runs.
Authentication and project errors are distinctly handled.
274-282
: Pagination updates are straightforward.
Ensures the UI reflects the correct page details.
289-295
: createNoRunsFoundItem clarifies empty state to users.
303-315
: Grouping runs by pipeline is efficient and logical.
323-337
: createPipelineTreeItems organizes pipelines and runs effectively.
345-376
: createPipelineRunTreeItem includes run details thoroughly.
Time formatting is user-friendly.
384-421
: createStepsTreeItem neatly enumerates steps and statuses.
429-446
: Model data handling is comprehensive.
454-471
: createConfigTreeItem logs config details well.
480-488
: createValueTreeItem appropriately covers arrays, objects, and scalars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/views/activityBar/pipelineView/PipelineDataProvider.ts (3)
54-64
: Fix the doc comment to reference PipelineDataProvider instead of ServerDataProvider.
The doc comment here says “Retrieves the singleton instance of ServerDataProvider,” but this file implementsPipelineDataProvider
.- * Retrieves the singleton instance of ServerDataProvider. + * Retrieves the singleton instance of PipelineDataProvider.
77-85
: Consider gracefully handling missing keys in LOADING_TREE_ITEMS.
Currently using the non-null assertion operator!
may cause runtime errors if the key isn’t present. You might add a fallback or check before accessing.- this.items = [LOADING_TREE_ITEMS.get(entity)!]; + const loadingTreeItem = LOADING_TREE_ITEMS.get(entity); + this.items = [loadingTreeItem ?? new LoadingTreeItem(`Loading ${entity}...`)];
240-267
: Unified error handling strategy.
The method neatly categorizes authentication errors, missing projects, version conflicts, and final unknown cases. Consider adding logs for unrecognized errors for easier debugging.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md
(3 hunks)src/views/activityBar/pipelineView/PipelineDataProvider.ts
(4 hunks)src/views/activityBar/pipelineView/PipelineTreeItems.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- src/views/activityBar/pipelineView/PipelineTreeItems.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
**/*.ts
: Review the Typescript code for conformity with industry standards and best practices, highlighting any deviations.
src/views/activityBar/pipelineView/PipelineDataProvider.ts
🧬 Code Definitions (1)
src/views/activityBar/pipelineView/PipelineDataProvider.ts (8)
src/views/activityBar/common/PaginatedDataProvider.ts (1)
PaginatedDataProvider
(24-154)src/utils/constants.ts (3)
LSCLIENT_STATE_CHANGED
(35-35)LSP_ZENML_CLIENT_INITIALIZED
(27-27)LSP_ZENML_PROJECT_CHANGED
(30-30)src/views/activityBar/common/LoadingTreeItem.ts (1)
LOADING_TREE_ITEMS
(23-32)src/types/PipelineTypes.ts (5)
PipelineRunsData
(16-22)PipelineRun
(49-61)PipelineRunStep
(24-29)PipelineModel
(40-47)PipelineRunConfig
(31-38)src/services/LSClient.ts (1)
LSClient
(34-275)src/views/activityBar/common/ErrorTreeItem.ts (1)
createErrorItem
(45-52)src/views/activityBar/common/TreeItemWithChildren.ts (1)
TreeItemWithChildren
(19-21)src/views/activityBar/pipelineView/PipelineTreeItems.ts (2)
PipelineTreeItem
(21-41)PipelineRunTreeItem
(47-60)
🔇 Additional comments (19)
src/views/activityBar/pipelineView/PipelineDataProvider.ts (19)
13-35
: No issues with the updated imports.
These imports properly bring in required modules and data types, and there are no noticeable circular imports or redundancies.
43-43
: Property for active project.
Storing the active project name here looks good for maintaining state across refresh cycles.
70-75
: Event subscriptions look correct.
Unsubscribing first and then subscribing helps avoid multiple bindings. This prevents potential event-handling duplication.
87-97
: Project change handler looks appropriate.
Avoiding unnecessary refresh when the project name hasn’t changed is a reasonable optimization.
99-105
: LS client state handler logic is clear.
Triggering a loading state if not running, and refreshing otherwise, aligns well with the lifecycle of the LS client.
117-127
: ZenML client state change.
Good approach to handle re-initialization and dynamic event binding. This ensures the provider’s data is refreshed properly when the client changes state.
135-147
: Enhanced refresh method with optional projectName.
This update provides flexibility in specifying the active project or falling back to the config. The fallback logic is straightforward.
164-188
: New fetchPipelineRuns implementation.
- Including an optional
projectName
parameter extends functionality without breaking existing usage.- Checking for errors first via
isErrorResponse
helps avoid unhandled statuses.- Returning a “No runs found” message is a user-friendly way to handle empty datasets.
204-223
: Fetching pipeline runs data from LSClient.
The approach of callingsendLsClientRequest('getPipelineRuns', [ ... ])
is consistent with the rest of the extension’s architecture.
231-238
: Checks for multiple error response conditions.
The broad checks capture both array-based error payloads and potential version-mismatch scenarios. This is thorough for the known error patterns.
269-282
: Pagination updates are straightforward.
Storing total items, total pages, etc., paves the way for user-friendly pagination logic.
284-295
: Creating a “No Runs Found” item.
Great for user feedback. The context value “noRuns” can help in future conditional UI customizations.
297-315
: Grouping runs by pipeline.
Simple loop logic that organizes pipeline runs by name. No immediate issues or performance concerns for typical usage.
317-337
: Creating pipeline tree items.
UsingcontextValue
to distinguish among items is a good approach. The result is easy to navigate in the activity bar.
339-377
: Creating pipeline run tree items.
Including detailed subitems (ID, name, times, status, etc.) offers a comprehensive view for each run. The approach is consistent with the extension’s pattern.
379-422
: Steps tree creation.
Dynamically adding child nodes for each step, with its sub-attributes, is well-structured.
424-447
: Model tree creation.
Collapsing the tree for model details is sensible. Testing a case with many model properties would confirm the UI’s responsiveness.
449-472
: Config tree creation.
Excluding themodel
property from the loop avoids duplication with the separate model item. This design keeps items organized.
474-489
: Value tree items handle various data types gracefully.
Array values are appended as a comma-separated string, objects are JSON-stringified, and primitive values are shown in plain text—covering common cases.
This PR accomplishes two main goals:
Key Changes
WorkspacesWrapper
andProjectsWrapper
classes with appropriate UI updatessteps
,config
, andmodel
(if applicable) tree items1.98
->^1.93
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation