-
Notifications
You must be signed in to change notification settings - Fork 571
UN-1722 [FEAT] Add export reminder for Prompt Studio projects in use #1547
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
1cec880
3b84ae8
afde3a8
d151e6d
b87a9db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,27 +122,40 @@ def perform_destroy(self, instance: CustomTool) -> None: | |
| organization_id = UserSessionUtils.get_organization_id(self.request) | ||
| instance.delete(organization_id) | ||
|
|
||
| def destroy( | ||
| self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] | ||
| ) -> Response: | ||
| instance: CustomTool = self.get_object() | ||
| # Checks if tool is exported | ||
| def _check_tool_usage(self, instance: CustomTool) -> tuple[bool, set]: | ||
| """Check if a tool is being used in any workflows. | ||
|
|
||
| Args: | ||
| instance: The CustomTool instance to check | ||
|
|
||
| Returns: | ||
| Tuple of (is_used: bool, dependent_workflows: set) | ||
| """ | ||
| if hasattr(instance, "prompt_studio_registry"): | ||
| exported_tool_instances_in_use = ToolInstance.objects.filter( | ||
| tool_id__exact=instance.prompt_studio_registry.pk | ||
| ) | ||
| dependent_wfs = set() | ||
| for tool_instance in exported_tool_instances_in_use: | ||
| dependent_wfs.add(tool_instance.workflow_id) | ||
| if len(dependent_wfs) > 0: | ||
| logger.info( | ||
| f"Cannot destroy custom tool {instance.tool_id}," | ||
| f" depended by workflows {dependent_wfs}" | ||
| ) | ||
| raise ToolDeleteError( | ||
| "Failed to delete tool, its used in other workflows. " | ||
| "Delete its usages first" | ||
| ) | ||
| return len(dependent_wfs) > 0, dependent_wfs | ||
| return False, set() | ||
|
|
||
| def destroy( | ||
| self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any] | ||
| ) -> Response: | ||
| instance: CustomTool = self.get_object() | ||
| # Checks if tool is exported | ||
| is_used, dependent_wfs = self._check_tool_usage(instance) | ||
| if is_used: | ||
| logger.info( | ||
| f"Cannot destroy custom tool {instance.tool_id}," | ||
| f" depended by workflows {dependent_wfs}" | ||
| ) | ||
| raise ToolDeleteError( | ||
| "Failed to delete tool, its used in other workflows. " | ||
| "Delete its usages first" | ||
| ) | ||
| return super().destroy(request, *args, **kwargs) | ||
|
|
||
| def partial_update( | ||
|
|
@@ -637,3 +650,89 @@ def import_project(self, request: Request) -> Response: | |
| {"error": "Failed to import project"}, | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) | ||
|
|
||
| @action(detail=True, methods=["get"]) | ||
| def check_deployment_usage(self, request: Request, pk: Any = None) -> Response: | ||
| """Check if the Prompt Studio project is used in any deployments. | ||
|
|
||
| This endpoint checks if the exported tool from this project is being used in: | ||
| - API Deployments | ||
| - ETL Pipelines | ||
| - Task Pipelines | ||
| - Manual Review (Human Quality Review) | ||
|
|
||
| Returns: | ||
| Response: Contains is_used flag and deployment types where it's used | ||
| """ | ||
| try: | ||
| instance: CustomTool = self.get_object() | ||
| is_used, workflow_ids = self._check_tool_usage(instance) | ||
|
|
||
| deployment_info = {"is_used": is_used, "deployment_types": [], "message": ""} | ||
|
|
||
| if is_used and workflow_ids: | ||
| # Import necessary models | ||
| from api_v2.models import APIDeployment | ||
| from pipeline_v2.models import Pipeline | ||
| from workflow_manager.endpoint_v2.models import WorkflowEndpoint | ||
|
Comment on lines
+714
to
+717
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @athul-rs move these imports to the top of the file |
||
|
|
||
| deployment_types = set() | ||
|
|
||
| # Check API Deployments | ||
| if APIDeployment.objects.filter( | ||
| workflow_id__in=workflow_ids, is_active=True | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @athul-rs do you think we need to filter for active pipelines / deployments alone? This might cause an issue / drift in behaviour when they enable those pipelines/ deployments again |
||
| ).exists(): | ||
| deployment_types.add("API Deployment") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maintain this |
||
|
|
||
| # Check Pipelines | ||
| pipelines = ( | ||
| Pipeline.objects.filter(workflow_id__in=workflow_ids, is_active=True) | ||
| .values_list("pipeline_type", flat=True) | ||
| .distinct() | ||
| ) | ||
|
|
||
| for pipeline_type in pipelines: | ||
| if pipeline_type == "ETL": | ||
| deployment_types.add("ETL Pipeline") | ||
| elif pipeline_type == "TASK": | ||
| deployment_types.add("Task Pipeline") | ||
|
Comment on lines
+734
to
+738
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chainUse Pipeline model constants for pipeline types. The string literals Verify that the Pipeline model defines these types and update the code accordingly: 🏁 Script executed: #!/bin/bash
# Search for pipeline_type field definition in Pipeline model
rg -nP -A5 'class Pipeline.*:' --type=py | rg -A10 'pipeline_type'Length of output: 1502 I need to search for the Pipeline model definition more specifically to verify if it defines constants for pipeline types. Length of output: 168 🏁 Script executed: #!/bin/bash
# Find and display Pipeline model class with pipeline_type field
rg -nP 'class Pipeline\b' --type=py -A 50 | head -100Length of output: 3598 🏁 Script executed: #!/bin/bash
# Check the exact code at the flagged lines and imports
rg -nP -B5 -A10 'for pipeline_type in pipelines:' backend/prompt_studio/prompt_studio_core_v2/views.pyLength of output: 818 🏁 Script executed: #!/bin/bash
# Check imports in the views.py file
head -50 backend/prompt_studio/prompt_studio_core_v2/views.py | rg -n 'import|from'Length of output: 1911 🏁 Script executed: #!/bin/bash
# Check if Pipeline is imported anywhere in the file
rg -n 'Pipeline' backend/prompt_studio/prompt_studio_core_v2/views.py | head -20Length of output: 379 🏁 Script executed: #!/bin/bash
# Check all imports in the file more comprehensively
grep -n "^from\|^import" backend/prompt_studio/prompt_studio_core_v2/views.py | head -80Length of output: 2514 Replace magic strings with Pipeline model constants. At lines 735 and 737, replace 🤖 Prompt for AI Agents |
||
|
|
||
| # Check for Manual Review | ||
| workflows_with_manual_review = ( | ||
| WorkflowEndpoint.objects.filter( | ||
| workflow_id__in=workflow_ids, | ||
| connection_type=WorkflowEndpoint.ConnectionType.MANUALREVIEW, | ||
| ) | ||
| .values_list("workflow_id", flat=True) | ||
| .distinct() | ||
| ) | ||
|
|
||
| if workflows_with_manual_review: | ||
| deployment_types.add("Human Quality Review") | ||
|
|
||
| deployment_info["deployment_types"] = list(deployment_types) | ||
|
|
||
| # Construct the message if deployments exist | ||
| if deployment_types: | ||
| types_list = sorted(deployment_types) | ||
| if len(types_list) == 1: | ||
| types_text = types_list[0] | ||
| elif len(types_list) == 2: | ||
| types_text = f"{types_list[0]} or {types_list[1]}" | ||
| else: | ||
| types_text = ", ".join(types_list[:-1]) + f", or {types_list[-1]}" | ||
|
|
||
| deployment_info["message"] = ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For better UX, I think we should attempt to display the actual workflows / deployments that its used in as clickable links which will take the user there. Not sure on the lift here but do you think you can give this a shot with any agent @athul-rs ? |
||
| f"You have made changes to this Prompt Studio project. " | ||
| f"This project is used in {types_text}. " | ||
| f"Please export it for the changes to take effect in the deployment(s)." | ||
| ) | ||
|
|
||
| return Response(deployment_info, status=status.HTTP_200_OK) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error checking deployment usage for tool {pk}: {e}") | ||
| return Response( | ||
| {"error": "Failed to check deployment usage"}, | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) | ||
|
Comment on lines
+775
to
+778
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @athul-rs in case of an error, please raise an exception instead |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| .export-reminder-bar { | ||
| position: sticky; | ||
| top: 0; | ||
| z-index: 100; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .export-reminder-bar .ant-alert { | ||
| border-radius: 0; | ||
| border: none; | ||
| background-color: #fffbe6; | ||
| border-bottom: 1px solid #ffe58f; | ||
| } | ||
|
|
||
| .export-reminder-content { | ||
| width: 100%; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| gap: 12px; | ||
| } | ||
|
|
||
| .export-reminder-text { | ||
| flex: 1; | ||
| max-width: 800px; | ||
| text-align: left; | ||
| } | ||
|
|
||
| .export-reminder-button { | ||
| margin-left: auto; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import { ExclamationCircleOutlined } from "@ant-design/icons"; | ||
| import { Alert, Button, Space } from "antd"; | ||
| import PropTypes from "prop-types"; | ||
| import "./ExportReminderBar.css"; | ||
|
|
||
| function ExportReminderBar({ message, onExport, isExporting }) { | ||
| return ( | ||
| <div className="export-reminder-bar"> | ||
| <Alert | ||
| message={ | ||
| <Space className="export-reminder-content"> | ||
| <ExclamationCircleOutlined /> | ||
| <span className="export-reminder-text">{message}</span> | ||
| <Button | ||
| type="primary" | ||
| size="small" | ||
| onClick={onExport} | ||
| loading={isExporting} | ||
| className="export-reminder-button" | ||
| > | ||
| Export Now | ||
| </Button> | ||
| </Space> | ||
| } | ||
| type="warning" | ||
| banner | ||
| closable={false} | ||
| /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| ExportReminderBar.propTypes = { | ||
| message: PropTypes.string.isRequired, | ||
| onExport: PropTypes.func.isRequired, | ||
| isExporting: PropTypes.bool, | ||
| }; | ||
|
|
||
| ExportReminderBar.defaultProps = { | ||
| isExporting: false, | ||
| }; | ||
|
|
||
| export { ExportReminderBar }; |
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.
NIT: Consider making the function name explicit itself.
_check_prompt_studio_usage_in_wf/_check_tool_usage_in_wf