From 3753f52c72429401af730a909f3fa2a3e565fcd1 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Wed, 18 Dec 2024 13:25:57 -0500 Subject: [PATCH 01/23] prefactor: add the deletions into a test class --- apps/assistants/tests/test_delete.py | 67 ++++++++++++++-------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 2c8485116..7e43b7935 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -27,37 +27,36 @@ def code_resource(assistant): return tool_resource -@pytest.mark.django_db() -def test_files_to_delete_when_only_referenced_by_one_resource(code_resource): - files_to_delete = list(_get_files_to_delete(code_resource.assistant.team, code_resource.id)) - assert len(files_to_delete) == 2 - assert {f.id for f in files_to_delete} == {f.id for f in code_resource.files.all()} - - -@pytest.mark.django_db() -def test_files_not_to_delete_when_referenced_by_multiple_resources(code_resource): - all_files = list(code_resource.files.all()) - tool_resource = ToolResources.objects.create(tool_type="file_search", assistant=code_resource.assistant) - tool_resource.files.set([all_files[0]]) - - # only the second file should be deleted - files_to_delete = list(_get_files_to_delete(code_resource.assistant.team, code_resource.id)) - assert len(files_to_delete) == 1 - assert files_to_delete[0].id == all_files[1].id - - files_to_delete = list(_get_files_to_delete(tool_resource.assistant.team, tool_resource.id)) - assert len(files_to_delete) == 0 - - -@pytest.mark.django_db() -def test_delete_openai_files_for_resource(code_resource): - all_files = list(code_resource.files.all()) - assert all(f.external_id for f in all_files) - assert all(f.external_source for f in all_files) - client = Mock() - delete_openai_files_for_resource(client, code_resource.assistant.team, code_resource) - - assert client.files.delete.call_count == 2 - all_files = list(code_resource.files.all()) - assert not any(f.external_id for f in all_files) - assert not any(f.external_source for f in all_files) +class TestAssistantDeletion: + @pytest.mark.django_db() + def test_files_to_delete_when_only_referenced_by_one_resource(self, code_resource): + files_to_delete = list(_get_files_to_delete(code_resource.assistant.team, code_resource.id)) + assert len(files_to_delete) == 2 + assert {f.id for f in files_to_delete} == {f.id for f in code_resource.files.all()} + + @pytest.mark.django_db() + def test_files_not_to_delete_when_referenced_by_multiple_resources(self, code_resource): + all_files = list(code_resource.files.all()) + tool_resource = ToolResources.objects.create(tool_type="file_search", assistant=code_resource.assistant) + tool_resource.files.set([all_files[0]]) + + # only the second file should be deleted + files_to_delete = list(_get_files_to_delete(code_resource.assistant.team, code_resource.id)) + assert len(files_to_delete) == 1 + assert files_to_delete[0].id == all_files[1].id + + files_to_delete = list(_get_files_to_delete(tool_resource.assistant.team, tool_resource.id)) + assert len(files_to_delete) == 0 + + @pytest.mark.django_db() + def test_delete_openai_files_for_resource(self, code_resource): + all_files = list(code_resource.files.all()) + assert all(f.external_id for f in all_files) + assert all(f.external_source for f in all_files) + client = Mock() + delete_openai_files_for_resource(client, code_resource.assistant.team, code_resource) + + assert client.files.delete.call_count == 2 + all_files = list(code_resource.files.all()) + assert not any(f.external_id for f in all_files) + assert not any(f.external_source for f in all_files) From 70a78bf877dbdec14f5568c872861a18b296bc02 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Wed, 18 Dec 2024 15:37:59 -0500 Subject: [PATCH 02/23] modify archive for assistants such that it checks for references for all assitatns before archiving and deleteling those references as well --- apps/assistants/models.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index 391b61bc1..d3209c74e 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -176,20 +176,23 @@ def get_custom_action_operations(self) -> models.QuerySet: def archive(self): from apps.assistants.tasks import delete_openai_assistant_task - # don't archive assistant if it's still referenced by an active experiment or pipeline - if self.get_related_experiments_queryset().exists(): + if self.get_related_experiments_queryset().exists() or self.get_related_pipeline_node_queryset().exists(): return False - - if self.get_related_pipeline_node_queryset().exists(): - return False - - super().archive() if self.is_working_version: - # TODO: should this delete the assistant from OpenAI? + for ( + version + ) in self.versions.all(): # first perform all checks so assistants are not archived prior to return False + if ( + version.get_related_experiments_queryset().exists() + or version.get_related_pipeline_node_queryset().exists() + ): + return False + for version in self.versions.all(): + delete_openai_assistant_task.delay(version.id) self.versions.update(is_archived=True, audit_action=AuditAction.AUDIT) - else: - delete_openai_assistant_task.delay(self.id) + super().archive() + delete_openai_assistant_task.delay(self.id) return True def get_related_experiments_queryset(self): From 35ce4f08613e5c2f14f12af41c9e056747119ffc Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Wed, 18 Dec 2024 15:39:39 -0500 Subject: [PATCH 03/23] add tests --- apps/assistants/tests/test_delete.py | 54 +++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 7e43b7935..52cb34c82 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -1,12 +1,14 @@ import uuid -from unittest.mock import Mock +from unittest.mock import Mock, patch import pytest from apps.assistants.models import ToolResources from apps.assistants.sync import _get_files_to_delete, delete_openai_files_for_resource from apps.utils.factories.assistants import OpenAiAssistantFactory +from apps.utils.factories.experiment import ExperimentFactory from apps.utils.factories.files import FileFactory +from apps.utils.factories.pipelines import PipelineFactory @pytest.fixture() @@ -27,14 +29,13 @@ def code_resource(assistant): return tool_resource +@pytest.mark.django_db() class TestAssistantDeletion: - @pytest.mark.django_db() def test_files_to_delete_when_only_referenced_by_one_resource(self, code_resource): files_to_delete = list(_get_files_to_delete(code_resource.assistant.team, code_resource.id)) assert len(files_to_delete) == 2 assert {f.id for f in files_to_delete} == {f.id for f in code_resource.files.all()} - @pytest.mark.django_db() def test_files_not_to_delete_when_referenced_by_multiple_resources(self, code_resource): all_files = list(code_resource.files.all()) tool_resource = ToolResources.objects.create(tool_type="file_search", assistant=code_resource.assistant) @@ -48,7 +49,6 @@ def test_files_not_to_delete_when_referenced_by_multiple_resources(self, code_re files_to_delete = list(_get_files_to_delete(tool_resource.assistant.team, tool_resource.id)) assert len(files_to_delete) == 0 - @pytest.mark.django_db() def test_delete_openai_files_for_resource(self, code_resource): all_files = list(code_resource.files.all()) assert all(f.external_id for f in all_files) @@ -60,3 +60,49 @@ def test_delete_openai_files_for_resource(self, code_resource): all_files = list(code_resource.files.all()) assert not any(f.external_id for f in all_files) assert not any(f.external_source for f in all_files) + + +# assistant.refresh_from_db() + + +@pytest.mark.django_db() +class TestAssistantArchival: + def test_archive_assistant(self): + assistant = OpenAiAssistantFactory() + assert assistant.is_archived is False + assistant.archive() + assert assistant.is_archived is True + + def test_archive_assistant_with_still_exisiting_experiment_and_pipeline(self): + experiment = ExperimentFactory() + assistant = OpenAiAssistantFactory() + experiment.pipeline = PipelineFactory(team=experiment.team) + experiment.assistant = assistant + experiment.save() + + assistant.archive() + assert assistant.is_archived is False # archiving failed + + experiment.archive() + assistant.archive() + assert assistant.is_archived is True # archiving successful + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_archive_versioned_assistant_with_still_exisiting_experiment_and_pipeline(self): + assistant = OpenAiAssistantFactory() + v2_assistant = assistant.create_new_version() + experiment = ExperimentFactory() + experiment.pipeline = PipelineFactory(team=experiment.team) + experiment.assistant = v2_assistant + experiment.save() + + assistant.archive() + assert assistant.is_archived is False # archiving failed + assert v2_assistant.is_archived is False + + experiment.archive() + assistant.archive() + v2_assistant.refresh_from_db() + + assert assistant.is_archived is True # archiving successful + assert v2_assistant.is_archived is True From b5404ef02781e8626ba02b21f5f03dc172196159 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 19 Dec 2024 11:05:34 -0500 Subject: [PATCH 04/23] modify the reference local objects to include those of the versioned assistants when the referenced assistant is going to be archived --- apps/assistants/models.py | 15 ++++++++++++--- apps/assistants/views.py | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index d3209c74e..87f0c0941 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -9,7 +9,7 @@ from field_audit.models import AuditAction, AuditingManager from apps.chat.agent.tools import get_assistant_tools -from apps.experiments.models import VersionsMixin, VersionsObjectManagerMixin +from apps.experiments.models import Experiment, VersionsMixin, VersionsObjectManagerMixin from apps.experiments.versioning import VersionField from apps.teams.models import BaseTeamModel from apps.utils.models import BaseModel @@ -195,12 +195,21 @@ def archive(self): delete_openai_assistant_task.delay(self.id) return True - def get_related_experiments_queryset(self): + def get_related_experiments_queryset(self, query=None): + if query: + return Experiment.objects.filter(assistant_id__in=query, is_archived=False) + return self.experiment_set.filter(is_archived=False) - def get_related_pipeline_node_queryset(self): + def get_related_pipeline_node_queryset(self, query=None): from apps.pipelines.models import Node + if query: + return Node.objects.filter(type="AssistantNode").filter( + params__assistant_id__in=query, + pipeline__is_archived=False, + ) + return Node.objects.filter(type="AssistantNode").filter( params__assistant_id=str(self.id), pipeline__is_archived=False, diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 82d5a0d03..5a95fc862 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -1,6 +1,7 @@ from django.contrib import messages from django.contrib.auth.mixins import PermissionRequiredMixin from django.db import transaction +from django.db.models import Q from django.http import HttpResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404, render from django.template.loader import render_to_string @@ -212,13 +213,23 @@ def delete(self, request, team_slug: str, pk: int): messages.success(request, "Assistant Archived") return HttpResponse() else: + version_query = None + if assistant.is_working_version: + version_query = list( + map( + str, + OpenAiAssistant.objects.filter( + Q(id=assistant.id) | Q(working_version__id=assistant.id) + ).values_list("id", flat=True), + ) + ) experiments = [ Chip(label=experiment.name, url=experiment.get_absolute_url()) - for experiment in assistant.get_related_experiments_queryset() + for experiment in assistant.get_related_experiments_queryset(query=version_query) ] pipeline_nodes = [ Chip(label=node.pipeline.name, url=node.pipeline.get_absolute_url()) - for node in assistant.get_related_pipeline_node_queryset().select_related("pipeline") + for node in assistant.get_related_pipeline_node_queryset(query=version_query).select_related("pipeline") ] response = render_to_string( "assistants/partials/referenced_objects.html", From 527e24e244d3e46613f723e4758de73088eedea4 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 19 Dec 2024 11:08:37 -0500 Subject: [PATCH 05/23] add tests for queryset functions --- apps/assistants/tests/test_delete.py | 71 +++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 6 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 52cb34c82..ce85a48b0 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -2,13 +2,14 @@ from unittest.mock import Mock, patch import pytest +from django.db.models import Q -from apps.assistants.models import ToolResources +from apps.assistants.models import OpenAiAssistant, ToolResources from apps.assistants.sync import _get_files_to_delete, delete_openai_files_for_resource from apps.utils.factories.assistants import OpenAiAssistantFactory from apps.utils.factories.experiment import ExperimentFactory from apps.utils.factories.files import FileFactory -from apps.utils.factories.pipelines import PipelineFactory +from apps.utils.factories.pipelines import NodeFactory, PipelineFactory @pytest.fixture() @@ -73,10 +74,9 @@ def test_archive_assistant(self): assistant.archive() assert assistant.is_archived is True - def test_archive_assistant_with_still_exisiting_experiment_and_pipeline(self): + def test_archive_assistant_with_still_exisiting_experiment(self): experiment = ExperimentFactory() assistant = OpenAiAssistantFactory() - experiment.pipeline = PipelineFactory(team=experiment.team) experiment.assistant = assistant experiment.save() @@ -91,8 +91,8 @@ def test_archive_assistant_with_still_exisiting_experiment_and_pipeline(self): def test_archive_versioned_assistant_with_still_exisiting_experiment_and_pipeline(self): assistant = OpenAiAssistantFactory() v2_assistant = assistant.create_new_version() - experiment = ExperimentFactory() - experiment.pipeline = PipelineFactory(team=experiment.team) + pipeline = PipelineFactory() + experiment = ExperimentFactory(pipeline=pipeline) experiment.assistant = v2_assistant experiment.save() @@ -106,3 +106,62 @@ def test_archive_versioned_assistant_with_still_exisiting_experiment_and_pipelin assert assistant.is_archived is True # archiving successful assert v2_assistant.is_archived is True + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_get_related_pipeline_node_queryset_with_versions(self): + assistant = OpenAiAssistantFactory() + v2_assistant = assistant.create_new_version() + pipeline = PipelineFactory() + NodeFactory(type="AssistantNode", pipeline=pipeline, params={"assistant_id": str(assistant.id)}) + exp = ExperimentFactory(pipeline=pipeline) + exp.assistant = assistant + exp.save() + v2_exp = exp.create_new_version() + NodeFactory(type="AssistantNode", pipeline=v2_exp.pipeline, params={"assistant_id": str(v2_assistant.id)}) + NodeFactory(type="AssistantNode", pipeline=v2_exp.pipeline, params={"assistant_id": str(v2_assistant.id)}) + v2_exp.assistant = v2_assistant + v2_exp.save() + assistant.refresh_from_db() + v2_assistant.refresh_from_db() + + version_query = list( + map( + str, + OpenAiAssistant.objects.filter(Q(id=assistant.id) | Q(working_version__id=assistant.id)).values_list( + "id", flat=True + ), + ) + ) + + assert len(assistant.get_related_pipeline_node_queryset()) == 1 + assert len(v2_assistant.get_related_pipeline_node_queryset()) == 2 + assert len(assistant.get_related_pipeline_node_queryset(query=version_query)) == 3 + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_get_related_experiments_queryset_with_versions(self): + assistant = OpenAiAssistantFactory() + v2_assistant = assistant.create_new_version() + exp = ExperimentFactory() + exp.assistant = assistant + exp.save() + exp2 = ExperimentFactory() + exp2.assistant = assistant + exp2.save() + v2_exp = exp.create_new_version() + v2_exp.assistant = v2_assistant + v2_exp.save() + assistant.refresh_from_db() + v2_assistant.refresh_from_db() + + version_query = list( + map( + str, + OpenAiAssistant.objects.filter(Q(id=assistant.id) | Q(working_version__id=assistant.id)).values_list( + "id", flat=True + ), + ) + ) + + assert len(assistant.get_related_experiments_queryset()) == 2 + assert len(v2_assistant.get_related_experiments_queryset()) == 1 + assert len(assistant.get_related_experiments_queryset(query=version_query)) == 3 From 63b5e3c8b029d76e6eeae7ed80772bac88069948 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 14 Jan 2025 13:11:14 -0500 Subject: [PATCH 06/23] add AND to filter to to check if the assistant is also working_version OR defualt version to display, else we don't need to block --- apps/assistants/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 0007c1a34..0030c649d 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -219,7 +219,9 @@ def delete(self, request, team_slug: str, pk: int): map( str, OpenAiAssistant.objects.filter( - Q(id=assistant.id) | Q(working_version__id=assistant.id) + Q(id=assistant.id) + | Q(working_version__id=assistant.id) + & (Q(is_working_version=True) | Q(is_default_version=True)) ).values_list("id", flat=True), ) ) From ea2fac1333268b93f712f8ea17f07ccbb28ff154 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 14 Jan 2025 13:11:57 -0500 Subject: [PATCH 07/23] rename param and add typing for get related queryset functions for clarity --- apps/assistants/models.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index 6e366c49f..be929cb5d 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -190,18 +190,18 @@ def archive(self): delete_openai_assistant_task.delay(self.id) return True - def get_related_experiments_queryset(self, query=None): - if query: - return Experiment.objects.filter(assistant_id__in=query, is_archived=False) + def get_related_experiments_queryset(self, assistant_ids: list = None): + if assistant_ids: + return Experiment.objects.filter(assistant_id__in=assistant_ids, is_archived=False) return self.experiment_set.filter(is_archived=False) - def get_related_pipeline_node_queryset(self, query=None): + def get_related_pipeline_node_queryset(self, assistant_ids: list = None): from apps.pipelines.models import Node - if query: + if assistant_ids: return Node.objects.filter(type="AssistantNode").filter( - params__assistant_id__in=query, + params__assistant_id__in=assistant_ids, pipeline__is_archived=False, ) From 5fb1bade4c3363a34b2da6b0c698d12c061e2bcf Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 14 Jan 2025 13:37:52 -0500 Subject: [PATCH 08/23] fix logic- checks for working version and default version on the experiments and pipelines --- apps/assistants/models.py | 11 +++++++++-- apps/assistants/views.py | 4 +--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index be929cb5d..e2150507a 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -3,6 +3,7 @@ from django.contrib.postgres.fields import ArrayField from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models, transaction +from django.db.models import Q from django.urls import reverse from field_audit import audit_fields from field_audit.models import AuditAction, AuditingManager @@ -192,20 +193,26 @@ def archive(self): def get_related_experiments_queryset(self, assistant_ids: list = None): if assistant_ids: - return Experiment.objects.filter(assistant_id__in=assistant_ids, is_archived=False) + return Experiment.objects.filter( + Q(working_version_id=None) | Q(is_default_version=True), + assistant_id__in=assistant_ids, + is_archived=False, + ) - return self.experiment_set.filter(is_archived=False) + return self.experiment_set.filter(Q(working_version_id=None) | Q(is_default_version=True), is_archived=False) def get_related_pipeline_node_queryset(self, assistant_ids: list = None): from apps.pipelines.models import Node if assistant_ids: return Node.objects.filter(type="AssistantNode").filter( + Q(pipeline__working_version_id=None), params__assistant_id__in=assistant_ids, pipeline__is_archived=False, ) return Node.objects.filter(type="AssistantNode").filter( + Q(pipeline__working_version_id=None), params__assistant_id=str(self.id), pipeline__is_archived=False, ) diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 0030c649d..0007c1a34 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -219,9 +219,7 @@ def delete(self, request, team_slug: str, pk: int): map( str, OpenAiAssistant.objects.filter( - Q(id=assistant.id) - | Q(working_version__id=assistant.id) - & (Q(is_working_version=True) | Q(is_default_version=True)) + Q(id=assistant.id) | Q(working_version__id=assistant.id) ).values_list("id", flat=True), ) ) From bff8ccf0cdd11d907041df3b0c38d199791c6de3 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 14 Jan 2025 14:40:04 -0500 Subject: [PATCH 09/23] tests --- apps/assistants/tests/test_delete.py | 101 +++++++-------------------- 1 file changed, 27 insertions(+), 74 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index ce85a48b0..91e93d7c6 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -2,14 +2,12 @@ from unittest.mock import Mock, patch import pytest -from django.db.models import Q -from apps.assistants.models import OpenAiAssistant, ToolResources +from apps.assistants.models import ToolResources from apps.assistants.sync import _get_files_to_delete, delete_openai_files_for_resource from apps.utils.factories.assistants import OpenAiAssistantFactory from apps.utils.factories.experiment import ExperimentFactory from apps.utils.factories.files import FileFactory -from apps.utils.factories.pipelines import NodeFactory, PipelineFactory @pytest.fixture() @@ -74,94 +72,49 @@ def test_archive_assistant(self): assistant.archive() assert assistant.is_archived is True - def test_archive_assistant_with_still_exisiting_experiment(self): + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_archive_assistant_succeeds_with_released_experiment_experiment(self): experiment = ExperimentFactory() - assistant = OpenAiAssistantFactory() - experiment.assistant = assistant + exp_v2 = experiment.create_new_version() + experiment.is_default = True experiment.save() - + assistant = OpenAiAssistantFactory() + exp_v2 = experiment.create_new_version() + exp_v2.assistant = assistant + exp_v2.save() + assert exp_v2.is_default_version is False + assert exp_v2.is_working_version is False assistant.archive() - assert assistant.is_archived is False # archiving failed + assistant.refresh_from_db() + + assert assistant.is_archived is True # archiving succeeded - experiment.archive() + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_asistant_archive_blocked_by_working_related_experiment(self): + assistant = OpenAiAssistantFactory() + experiment = ExperimentFactory(assistant=assistant) + experiment.save() + + assert experiment.is_working_version is True assistant.archive() - assert assistant.is_archived is True # archiving successful + assistant.refresh_from_db() + assert assistant.is_archived is False # archiving blocked @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_archive_versioned_assistant_with_still_exisiting_experiment_and_pipeline(self): + def test_asistant_archive_blocked_by_published_related_experiment(self): assistant = OpenAiAssistantFactory() v2_assistant = assistant.create_new_version() - pipeline = PipelineFactory() - experiment = ExperimentFactory(pipeline=pipeline) - experiment.assistant = v2_assistant + experiment = ExperimentFactory(assistant=v2_assistant) + experiment.is_default = True experiment.save() assistant.archive() assert assistant.is_archived is False # archiving failed assert v2_assistant.is_archived is False - experiment.archive() + experiment.archive() # first archive related experiment through v2_assistant assistant.archive() v2_assistant.refresh_from_db() assert assistant.is_archived is True # archiving successful assert v2_assistant.is_archived is True - - @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_get_related_pipeline_node_queryset_with_versions(self): - assistant = OpenAiAssistantFactory() - v2_assistant = assistant.create_new_version() - pipeline = PipelineFactory() - NodeFactory(type="AssistantNode", pipeline=pipeline, params={"assistant_id": str(assistant.id)}) - exp = ExperimentFactory(pipeline=pipeline) - exp.assistant = assistant - exp.save() - v2_exp = exp.create_new_version() - NodeFactory(type="AssistantNode", pipeline=v2_exp.pipeline, params={"assistant_id": str(v2_assistant.id)}) - NodeFactory(type="AssistantNode", pipeline=v2_exp.pipeline, params={"assistant_id": str(v2_assistant.id)}) - v2_exp.assistant = v2_assistant - v2_exp.save() - assistant.refresh_from_db() - v2_assistant.refresh_from_db() - - version_query = list( - map( - str, - OpenAiAssistant.objects.filter(Q(id=assistant.id) | Q(working_version__id=assistant.id)).values_list( - "id", flat=True - ), - ) - ) - - assert len(assistant.get_related_pipeline_node_queryset()) == 1 - assert len(v2_assistant.get_related_pipeline_node_queryset()) == 2 - assert len(assistant.get_related_pipeline_node_queryset(query=version_query)) == 3 - - @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_get_related_experiments_queryset_with_versions(self): - assistant = OpenAiAssistantFactory() - v2_assistant = assistant.create_new_version() - exp = ExperimentFactory() - exp.assistant = assistant - exp.save() - exp2 = ExperimentFactory() - exp2.assistant = assistant - exp2.save() - v2_exp = exp.create_new_version() - v2_exp.assistant = v2_assistant - v2_exp.save() - assistant.refresh_from_db() - v2_assistant.refresh_from_db() - - version_query = list( - map( - str, - OpenAiAssistant.objects.filter(Q(id=assistant.id) | Q(working_version__id=assistant.id)).values_list( - "id", flat=True - ), - ) - ) - - assert len(assistant.get_related_experiments_queryset()) == 2 - assert len(v2_assistant.get_related_experiments_queryset()) == 1 - assert len(assistant.get_related_experiments_queryset(query=version_query)) == 3 From 03020f88088b3fff8e4cab23c432b7a44a22c04c Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Fri, 17 Jan 2025 16:22:30 -0500 Subject: [PATCH 10/23] add pipeline tests --- apps/assistants/tests/test_delete.py | 47 ++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 91e93d7c6..1ac176567 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -8,6 +8,7 @@ from apps.utils.factories.assistants import OpenAiAssistantFactory from apps.utils.factories.experiment import ExperimentFactory from apps.utils.factories.files import FileFactory +from apps.utils.factories.pipelines import NodeFactory, PipelineFactory @pytest.fixture() @@ -73,13 +74,12 @@ def test_archive_assistant(self): assert assistant.is_archived is True @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_archive_assistant_succeeds_with_released_experiment_experiment(self): - experiment = ExperimentFactory() - exp_v2 = experiment.create_new_version() - experiment.is_default = True - experiment.save() + def test_archive_assistant_succeeds_with_released_related_experiment(self): + exp_v1 = ExperimentFactory() + exp_v2 = exp_v1.create_new_version() + exp_v1.save() assistant = OpenAiAssistantFactory() - exp_v2 = experiment.create_new_version() + exp_v2 = exp_v1.create_new_version() exp_v2.assistant = assistant exp_v2.save() assert exp_v2.is_default_version is False @@ -118,3 +118,38 @@ def test_asistant_archive_blocked_by_published_related_experiment(self): assert assistant.is_archived is True # archiving successful assert v2_assistant.is_archived is True + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_archive_assistant_succeeds_with_released_related_pipeline(self): + pipeline = PipelineFactory() + exp_v1 = ExperimentFactory(pipeline=pipeline) + exp_v2 = exp_v1.create_new_version() + assistant = OpenAiAssistantFactory() + NodeFactory(pipeline=exp_v2.pipeline, type="AssistantNode", params={"assistant_id": assistant.id}) + exp_v2.is_default_version = False + exp_v2.save() + + assert exp_v2.pipeline.is_working_version is False + assistant.archive() + assistant.refresh_from_db() + + assert assistant.is_archived is True # archiving successful + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_archive_assistant_fails_with_working_related_pipeline(self): + pipeline = PipelineFactory() + assistant = OpenAiAssistantFactory() + NodeFactory(pipeline=pipeline, type="AssistantNode", params={"assistant_id": assistant.id}) + exp_v1 = ExperimentFactory(pipeline=pipeline) + exp_v1.save() + + assert pipeline.is_working_version is True + assistant.archive() + assert assistant.is_archived is False # archiving failed + + exp_v1.archive() + pipeline.archive() + assistant.archive() + + assert pipeline.is_archived is True + assert assistant.is_archived is True # archiving successful From a5e138c13a0bc34f3157218da6a78de390f5e38b Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Mon, 20 Jan 2025 11:04:18 -0500 Subject: [PATCH 11/23] remove wrong & unneccessary line --- apps/assistants/tests/test_delete.py | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 1ac176567..4a8bdab78 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -105,7 +105,6 @@ def test_asistant_archive_blocked_by_published_related_experiment(self): assistant = OpenAiAssistantFactory() v2_assistant = assistant.create_new_version() experiment = ExperimentFactory(assistant=v2_assistant) - experiment.is_default = True experiment.save() assistant.archive() From f5ff020be3960720ac5c91ea06075ce97abd5f88 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Mon, 20 Jan 2025 11:36:48 -0500 Subject: [PATCH 12/23] goal: trace references back to origin --- apps/assistants/models.py | 21 ++++++++++++++------- apps/assistants/tests/test_delete.py | 1 - apps/assistants/views.py | 8 ++++---- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index e2150507a..94468d44e 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -205,17 +205,24 @@ def get_related_pipeline_node_queryset(self, assistant_ids: list = None): from apps.pipelines.models import Node if assistant_ids: - return Node.objects.filter(type="AssistantNode").filter( + nodes = Node.objects.filter(type="AssistantNode").filter( Q(pipeline__working_version_id=None), params__assistant_id__in=assistant_ids, pipeline__is_archived=False, ) - - return Node.objects.filter(type="AssistantNode").filter( - Q(pipeline__working_version_id=None), - params__assistant_id=str(self.id), - pipeline__is_archived=False, - ) + else: + nodes = Node.objects.filter(type="AssistantNode").filter( + Q(pipeline__working_version_id=None), + params__assistant_id=str(self.id), + pipeline__is_archived=False, + ) + if nodes.exists(): + pipeline_ids = nodes.values_list("pipeline_id", flat=True) + return Experiment.objects.filter( + pipeline_id__in=pipeline_ids, + is_archived=False, + ) + return nodes @audit_fields( diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 4a8bdab78..3c5e8c30b 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -147,7 +147,6 @@ def test_archive_assistant_fails_with_working_related_pipeline(self): assert assistant.is_archived is False # archiving failed exp_v1.archive() - pipeline.archive() assistant.archive() assert pipeline.is_archived is True diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 0007c1a34..79733d44b 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -227,16 +227,16 @@ def delete(self, request, team_slug: str, pk: int): Chip(label=experiment.name, url=experiment.get_absolute_url()) for experiment in assistant.get_related_experiments_queryset(query=version_query) ] - pipeline_nodes = [ - Chip(label=node.pipeline.name, url=node.pipeline.get_absolute_url()) - for node in assistant.get_related_pipeline_node_queryset(query=version_query).select_related("pipeline") + experiments_with_pipeline_nodes = [ + Chip(label=experiment.name, url=experiment.get_absolute_url()) + for experiment in assistant.get_related_pipeline_node_queryset(query=version_query) ] response = render_to_string( "assistants/partials/referenced_objects.html", context={ "object_name": "assistant", "experiments": experiments, - "pipeline_nodes": pipeline_nodes, + "pipeline_nodes": experiments_with_pipeline_nodes, }, ) return HttpResponse(response, headers={"HX-Reswap": "none"}, status=400) From 1669af25852186182977c57ee13aebdb96d62113 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Mon, 20 Jan 2025 15:16:50 -0500 Subject: [PATCH 13/23] add version numbers to experiment links and update pipeline header to clairfy it links to experiments and not pipelines --- apps/assistants/views.py | 8 ++++---- apps/experiments/models.py | 3 +++ templates/assistants/partials/referenced_objects.html | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 79733d44b..471906571 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -224,12 +224,12 @@ def delete(self, request, team_slug: str, pk: int): ) ) experiments = [ - Chip(label=experiment.name, url=experiment.get_absolute_url()) - for experiment in assistant.get_related_experiments_queryset(query=version_query) + Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) + for experiment in assistant.get_related_experiments_queryset(assistant_ids=version_query) ] experiments_with_pipeline_nodes = [ - Chip(label=experiment.name, url=experiment.get_absolute_url()) - for experiment in assistant.get_related_pipeline_node_queryset(query=version_query) + Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) + for experiment in assistant.get_related_pipeline_node_queryset(assistant_ids=version_query) ] response = render_to_string( "assistants/partials/referenced_objects.html", diff --git a/apps/experiments/models.py b/apps/experiments/models.py index 2fa525a98..0055d3b3a 100644 --- a/apps/experiments/models.py +++ b/apps/experiments/models.py @@ -178,6 +178,9 @@ def archive(self): def is_editable(self) -> bool: return not self.is_archived + def get_version_name(self): + return f"v{self.version_number}" + @audit_fields(*model_audit_fields.SOURCE_MATERIAL_FIELDS, audit_special_queryset_writes=True) class SourceMaterial(BaseTeamModel, VersionsMixin): diff --git a/templates/assistants/partials/referenced_objects.html b/templates/assistants/partials/referenced_objects.html index cbfffd117..ce143ecdf 100644 --- a/templates/assistants/partials/referenced_objects.html +++ b/templates/assistants/partials/referenced_objects.html @@ -10,7 +10,7 @@

Experiments

{% endif %} {% if pipeline_nodes %} -

Pipelines

+

Experiments referencing Pipelines

    {% for node in pipeline_nodes %}
  • {% include "generic/chip.html" with chip=node %}
    • From 476ba11576829bb87137e3ee147c8ffd61e53249 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 23 Jan 2025 14:36:57 -0500 Subject: [PATCH 14/23] refactor get_related_pipeline_node_queryset --- apps/assistants/models.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index 94468d44e..afe1cb508 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -204,18 +204,12 @@ def get_related_experiments_queryset(self, assistant_ids: list = None): def get_related_pipeline_node_queryset(self, assistant_ids: list = None): from apps.pipelines.models import Node - if assistant_ids: - nodes = Node.objects.filter(type="AssistantNode").filter( - Q(pipeline__working_version_id=None), - params__assistant_id__in=assistant_ids, - pipeline__is_archived=False, - ) - else: - nodes = Node.objects.filter(type="AssistantNode").filter( - Q(pipeline__working_version_id=None), - params__assistant_id=str(self.id), - pipeline__is_archived=False, - ) + assistant_ids = assistant_ids if assistant_ids else [str(self.id)] + nodes = Node.objects.filter(type="AssistantNode").filter( + Q(pipeline__working_version_id=None), + params__assistant_id__in=assistant_ids, + pipeline__is_archived=False, + ) if nodes.exists(): pipeline_ids = nodes.values_list("pipeline_id", flat=True) return Experiment.objects.filter( From a1e85dfe4155fe6cb8072e16394dfa6308fe012c Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 23 Jan 2025 15:24:53 -0500 Subject: [PATCH 15/23] add get_related_experiments_with_pipeline_queryset to separate out when a pipeline needs to be flagged vs an experiment referencing a pipeline --- apps/assistants/models.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index afe1cb508..47cb8f427 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -12,6 +12,7 @@ from apps.custom_actions.mixins import CustomActionOperationMixin from apps.experiments.models import Experiment, VersionsMixin, VersionsObjectManagerMixin from apps.experiments.versioning import VersionField +from apps.pipelines.models import Node from apps.teams.models import BaseTeamModel from apps.utils.models import BaseModel @@ -172,7 +173,11 @@ def compare_with_model(self, new: Self, exclude_fields: list[str], early_abort=F def archive(self): from apps.assistants.tasks import delete_openai_assistant_task - if self.get_related_experiments_queryset().exists() or self.get_related_pipeline_node_queryset().exists(): + if ( + self.get_related_experiments_queryset().exists() + or self.get_related_pipeline_node_queryset().exists() + or self.get_related_experiments_with_pipeline_queryset().exists() + ): return False if self.is_working_version: for ( @@ -181,6 +186,7 @@ def archive(self): if ( version.get_related_experiments_queryset().exists() or version.get_related_pipeline_node_queryset().exists() + or self.get_related_experiments_with_pipeline_queryset().exists() ): return False for version in self.versions.all(): @@ -202,17 +208,24 @@ def get_related_experiments_queryset(self, assistant_ids: list = None): return self.experiment_set.filter(Q(working_version_id=None) | Q(is_default_version=True), is_archived=False) def get_related_pipeline_node_queryset(self, assistant_ids: list = None): - from apps.pipelines.models import Node + assistant_ids = assistant_ids if assistant_ids else [str(self.id)] + return Node.objects.filter(type="AssistantNode").filter( + pipeline__working_version_id=None, + params__assistant_id__in=assistant_ids, + pipeline__is_archived=False, + ) + def get_related_experiments_with_pipeline_queryset(self, assistant_ids: list = None): assistant_ids = assistant_ids if assistant_ids else [str(self.id)] nodes = Node.objects.filter(type="AssistantNode").filter( - Q(pipeline__working_version_id=None), + pipeline__working_version_id__isnull=False, params__assistant_id__in=assistant_ids, pipeline__is_archived=False, ) if nodes.exists(): pipeline_ids = nodes.values_list("pipeline_id", flat=True) return Experiment.objects.filter( + is_default_version=True, pipeline_id__in=pipeline_ids, is_archived=False, ) From 1b997c4acda3a2d5e6b594ff278ca74e1bb7d88c Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 23 Jan 2025 15:26:04 -0500 Subject: [PATCH 16/23] update views to display 3 sections: pipeline, experiments, and experiments with pipelines --- apps/assistants/views.py | 12 ++++++++++-- .../assistants/partials/referenced_objects.html | 12 +++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/apps/assistants/views.py b/apps/assistants/views.py index 471906571..a1acb0cfb 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -223,20 +223,28 @@ def delete(self, request, team_slug: str, pk: int): ).values_list("id", flat=True), ) ) + pipeline_nodes = assistant.get_related_pipeline_node_queryset() experiments = [ Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) for experiment in assistant.get_related_experiments_queryset(assistant_ids=version_query) ] + pipeline_nodes = [ + Chip(label=node.pipeline.name, url=node.pipeline.get_absolute_url()) + for node in assistant.get_related_pipeline_node_queryset(assistant_ids=version_query).select_related( + "pipeline" + ) + ] experiments_with_pipeline_nodes = [ Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) - for experiment in assistant.get_related_pipeline_node_queryset(assistant_ids=version_query) + for experiment in assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query) ] response = render_to_string( "assistants/partials/referenced_objects.html", context={ "object_name": "assistant", "experiments": experiments, - "pipeline_nodes": experiments_with_pipeline_nodes, + "pipeline_nodes": pipeline_nodes, + "experiments_with_pipeline_nodes": experiments_with_pipeline_nodes, }, ) return HttpResponse(response, headers={"HX-Reswap": "none"}, status=400) diff --git a/templates/assistants/partials/referenced_objects.html b/templates/assistants/partials/referenced_objects.html index ce143ecdf..4b6ce3ca8 100644 --- a/templates/assistants/partials/referenced_objects.html +++ b/templates/assistants/partials/referenced_objects.html @@ -10,7 +10,7 @@

    Experiments

    {% endif %} {% if pipeline_nodes %} -

    Experiments referencing Pipelines

    +

    Pipelines

      {% for node in pipeline_nodes %}
    • {% include "generic/chip.html" with chip=node %}
      • @@ -18,6 +18,16 @@

      Experiments referencing Pipelines

    {% endif %} + {% if pipeline_nodes %} +

    Experiments referencing Pipeline

    +
      + {% for experiment in experiments_with_pipeline_nodes %} +
    • {% include "generic/chip.html" with chip=experiment %}
      • + {% endfor %} +
    + {% endif %} + + {% if static_trigger_experiments %}

    Static Trigger Experiments

      From 0d01355b5179fbed025c7629b39903ec6565002a Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 23 Jan 2025 15:26:34 -0500 Subject: [PATCH 17/23] update tests and add 3rd test case --- apps/assistants/tests/test_delete.py | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 3c5e8c30b..617300626 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -134,19 +134,41 @@ def test_archive_assistant_succeeds_with_released_related_pipeline(self): assert assistant.is_archived is True # archiving successful + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_working_experiment(self): + pipeline_v1 = PipelineFactory() + assistant = OpenAiAssistantFactory() + pipeline_v2 = pipeline_v1.create_new_version() + NodeFactory(pipeline=pipeline_v2, type="AssistantNode", params={"assistant_id": assistant.id}) + exp_v1 = ExperimentFactory() + exp_v2 = exp_v1.create_new_version() + exp_v2.pipeline = pipeline_v2 + exp_v2.is_default_version = True + exp_v2.save() + + assert pipeline_v2.is_working_version is False + assert exp_v2.is_default_version is True + assert exp_v2.is_working_version is False + assistant.archive() + assert assistant.is_archived is False # archiving failed + + exp_v2.archive() + assistant.archive() + + assert exp_v2.is_archived is True + assert assistant.is_archived is True # archiving successful + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) def test_archive_assistant_fails_with_working_related_pipeline(self): pipeline = PipelineFactory() assistant = OpenAiAssistantFactory() NodeFactory(pipeline=pipeline, type="AssistantNode", params={"assistant_id": assistant.id}) - exp_v1 = ExperimentFactory(pipeline=pipeline) - exp_v1.save() assert pipeline.is_working_version is True assistant.archive() assert assistant.is_archived is False # archiving failed - exp_v1.archive() + pipeline.archive() assistant.archive() assert pipeline.is_archived is True From e7e717afc36fef6ac88ec69601ed9b9546638edb Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 28 Jan 2025 12:36:00 -0500 Subject: [PATCH 18/23] add function comments, plus logic fixes flagged in code review --- apps/assistants/models.py | 12 ++++++++---- apps/assistants/tables.py | 2 +- .../assistants/partials/referenced_objects.html | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index 47cb8f427..1709c2843 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -186,7 +186,7 @@ def archive(self): if ( version.get_related_experiments_queryset().exists() or version.get_related_pipeline_node_queryset().exists() - or self.get_related_experiments_with_pipeline_queryset().exists() + or version.get_related_experiments_with_pipeline_queryset().exists() ): return False for version in self.versions.all(): @@ -198,6 +198,7 @@ def archive(self): return True def get_related_experiments_queryset(self, assistant_ids: list = None): + """Returns working versions and published experiments containing the assistant ids""" if assistant_ids: return Experiment.objects.filter( Q(working_version_id=None) | Q(is_default_version=True), @@ -208,6 +209,7 @@ def get_related_experiments_queryset(self, assistant_ids: list = None): return self.experiment_set.filter(Q(working_version_id=None) | Q(is_default_version=True), is_archived=False) def get_related_pipeline_node_queryset(self, assistant_ids: list = None): + """Returns working version pipelines with assistant nodes containing the assistant ids""" assistant_ids = assistant_ids if assistant_ids else [str(self.id)] return Node.objects.filter(type="AssistantNode").filter( pipeline__working_version_id=None, @@ -216,20 +218,22 @@ def get_related_pipeline_node_queryset(self, assistant_ids: list = None): ) def get_related_experiments_with_pipeline_queryset(self, assistant_ids: list = None): + """Returns published experiment versions referenced by versioned pipelines with assistant nodes + containing the assistant ids""" assistant_ids = assistant_ids if assistant_ids else [str(self.id)] nodes = Node.objects.filter(type="AssistantNode").filter( pipeline__working_version_id__isnull=False, params__assistant_id__in=assistant_ids, pipeline__is_archived=False, ) - if nodes.exists(): - pipeline_ids = nodes.values_list("pipeline_id", flat=True) + + if pipeline_ids := nodes.values_list("pipeline_id", flat=True): return Experiment.objects.filter( is_default_version=True, pipeline_id__in=pipeline_ids, is_archived=False, ) - return nodes + return Experiment.objects.none() @audit_fields( diff --git a/apps/assistants/tables.py b/apps/assistants/tables.py index 49423c83d..07e52e6a9 100644 --- a/apps/assistants/tables.py +++ b/apps/assistants/tables.py @@ -28,7 +28,7 @@ class OpenAiAssistantTable(tables.Table): title="Archive", icon_class="fa-solid fa-box-archive", required_permissions=["assistants.delete_openaiassistant"], - confirm_message="This will not delete the assistant from OpenAI.", + confirm_message="This will delete the assistant from OpenAI.", hx_method="delete", ), actions.AjaxAction( diff --git a/templates/assistants/partials/referenced_objects.html b/templates/assistants/partials/referenced_objects.html index 4b6ce3ca8..ee668e938 100644 --- a/templates/assistants/partials/referenced_objects.html +++ b/templates/assistants/partials/referenced_objects.html @@ -18,7 +18,7 @@

      Pipelines

    {% endif %} - {% if pipeline_nodes %} + {% if experiments_with_pipeline_nodes %}

    Experiments referencing Pipeline

      {% for experiment in experiments_with_pipeline_nodes %} From 61642e9d3aa94aca8ed6127f3ef7d5ac291e3502 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 28 Jan 2025 13:11:58 -0500 Subject: [PATCH 19/23] fix test names and add test --- apps/assistants/tests/test_delete.py | 49 ++++++++++++++-------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 617300626..6b23e6ff7 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -90,36 +90,42 @@ def test_archive_assistant_succeeds_with_released_related_experiment(self): assert assistant.is_archived is True # archiving succeeded @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_asistant_archive_blocked_by_working_related_experiment(self): + def test_assistant_archive_blocked_by_working_related_experiment(self): assistant = OpenAiAssistantFactory() experiment = ExperimentFactory(assistant=assistant) experiment.save() assert experiment.is_working_version is True - assistant.archive() - assistant.refresh_from_db() - assert assistant.is_archived is False # archiving blocked + assert not assistant.archive() # archiving blocked + + @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) + def test_assistant_archive_blocked_by_published_related_experiment(self): + assistant = OpenAiAssistantFactory() + exp_v1 = ExperimentFactory() + exp_v2 = exp_v1.create_new_version(make_default=True) + exp_v2.assistant = assistant + exp_v2.save() + + assert not assistant.archive() # archiving blocked @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_asistant_archive_blocked_by_published_related_experiment(self): + def test_assistant_blocked_by_assistant_version_referenced_by_unpublished_related_experiment(self): assistant = OpenAiAssistantFactory() v2_assistant = assistant.create_new_version() experiment = ExperimentFactory(assistant=v2_assistant) experiment.save() - assistant.archive() - assert assistant.is_archived is False # archiving failed - assert v2_assistant.is_archived is False + assert experiment.is_working_version is True - experiment.archive() # first archive related experiment through v2_assistant - assistant.archive() - v2_assistant.refresh_from_db() + assert not assistant.archive() # archiving failed + assert not v2_assistant.archive() # archiving failed - assert assistant.is_archived is True # archiving successful - assert v2_assistant.is_archived is True + experiment.archive() # first archive related experiment through v2_assistant + assert assistant.archive() # archiving successful + assert v2_assistant.archive() # archiving successful @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) - def test_archive_assistant_succeeds_with_released_related_pipeline(self): + def test_archive_assistant_succeeds_with_unpublished_related_pipeline(self): pipeline = PipelineFactory() exp_v1 = ExperimentFactory(pipeline=pipeline) exp_v2 = exp_v1.create_new_version() @@ -129,10 +135,7 @@ def test_archive_assistant_succeeds_with_released_related_pipeline(self): exp_v2.save() assert exp_v2.pipeline.is_working_version is False - assistant.archive() - assistant.refresh_from_db() - - assert assistant.is_archived is True # archiving successful + assert assistant.archive() # archiving successful @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_working_experiment(self): @@ -150,13 +153,12 @@ def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_wor assert exp_v2.is_default_version is True assert exp_v2.is_working_version is False assistant.archive() - assert assistant.is_archived is False # archiving failed + assert not assistant.archive() # archiving failed exp_v2.archive() - assistant.archive() assert exp_v2.is_archived is True - assert assistant.is_archived is True # archiving successful + assert assistant.archive() # archiving successful @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) def test_archive_assistant_fails_with_working_related_pipeline(self): @@ -166,10 +168,9 @@ def test_archive_assistant_fails_with_working_related_pipeline(self): assert pipeline.is_working_version is True assistant.archive() - assert assistant.is_archived is False # archiving failed + assert not assistant.archive() # archiving failed pipeline.archive() - assistant.archive() assert pipeline.is_archived is True - assert assistant.is_archived is True # archiving successful + assert assistant.archive() # archiving successful From 6d2b9c07ea18ea3b2e40bad38c9414333c8d6c13 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Tue, 28 Jan 2025 14:12:40 -0500 Subject: [PATCH 20/23] add bracketed labels indicating experiment status in error modal --- apps/assistants/views.py | 15 ++++++++++++--- apps/experiments/models.py | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/apps/assistants/views.py b/apps/assistants/views.py index a1acb0cfb..0bf1301c4 100644 --- a/apps/assistants/views.py +++ b/apps/assistants/views.py @@ -223,9 +223,15 @@ def delete(self, request, team_slug: str, pk: int): ).values_list("id", flat=True), ) ) - pipeline_nodes = assistant.get_related_pipeline_node_queryset() experiments = [ - Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) + Chip( + label=( + f"{experiment.name} [{experiment.get_version_name()}]" + if experiment.is_working_version + else f"{experiment.name} {experiment.get_version_name()} [published]" + ), + url=experiment.get_absolute_url(), + ) for experiment in assistant.get_related_experiments_queryset(assistant_ids=version_query) ] pipeline_nodes = [ @@ -235,7 +241,10 @@ def delete(self, request, team_slug: str, pk: int): ) ] experiments_with_pipeline_nodes = [ - Chip(label=f"{experiment.name} {experiment.get_version_name()}", url=experiment.get_absolute_url()) + Chip( + label=f"{experiment.name} {experiment.get_version_name()} [published]", + url=experiment.get_absolute_url(), + ) for experiment in assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query) ] response = render_to_string( diff --git a/apps/experiments/models.py b/apps/experiments/models.py index 0055d3b3a..93189e45e 100644 --- a/apps/experiments/models.py +++ b/apps/experiments/models.py @@ -179,6 +179,9 @@ def is_editable(self) -> bool: return not self.is_archived def get_version_name(self): + """Returns version name in form of v + version number, or unreleased if working version.""" + if self.is_working_version: + return "unreleased" return f"v{self.version_number}" From 255dffa440ce0d4c088ebdbbcd5b5ccc8cac7e43 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Wed, 29 Jan 2025 10:04:17 -0500 Subject: [PATCH 21/23] add to query check if nodes are archived --- apps/assistants/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/assistants/models.py b/apps/assistants/models.py index 1709c2843..c52a81de6 100644 --- a/apps/assistants/models.py +++ b/apps/assistants/models.py @@ -214,6 +214,7 @@ def get_related_pipeline_node_queryset(self, assistant_ids: list = None): return Node.objects.filter(type="AssistantNode").filter( pipeline__working_version_id=None, params__assistant_id__in=assistant_ids, + is_archived=False, pipeline__is_archived=False, ) @@ -224,6 +225,7 @@ def get_related_experiments_with_pipeline_queryset(self, assistant_ids: list = N nodes = Node.objects.filter(type="AssistantNode").filter( pipeline__working_version_id__isnull=False, params__assistant_id__in=assistant_ids, + is_archived=False, pipeline__is_archived=False, ) From 5a034b1942fef04a73e684891a76c51040f069c5 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Wed, 29 Jan 2025 10:04:37 -0500 Subject: [PATCH 22/23] test cleanup --- apps/assistants/tests/test_delete.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 6b23e6ff7..801883035 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -76,11 +76,10 @@ def test_archive_assistant(self): @patch("apps.assistants.sync.push_assistant_to_openai", Mock()) def test_archive_assistant_succeeds_with_released_related_experiment(self): exp_v1 = ExperimentFactory() - exp_v2 = exp_v1.create_new_version() - exp_v1.save() assistant = OpenAiAssistantFactory() exp_v2 = exp_v1.create_new_version() exp_v2.assistant = assistant + exp_v2.is_default_version = False exp_v2.save() assert exp_v2.is_default_version is False assert exp_v2.is_working_version is False @@ -152,7 +151,6 @@ def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_wor assert pipeline_v2.is_working_version is False assert exp_v2.is_default_version is True assert exp_v2.is_working_version is False - assistant.archive() assert not assistant.archive() # archiving failed exp_v2.archive() From ce1432f86e926cc84359c32a79283ca89dd73431 Mon Sep 17 00:00:00 2001 From: Steph Herbers Date: Thu, 30 Jan 2025 10:36:54 -0500 Subject: [PATCH 23/23] tests make assistant_id the correct type --- apps/assistants/tests/test_delete.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/assistants/tests/test_delete.py b/apps/assistants/tests/test_delete.py index 801883035..e1f75f37d 100644 --- a/apps/assistants/tests/test_delete.py +++ b/apps/assistants/tests/test_delete.py @@ -141,7 +141,7 @@ def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_wor pipeline_v1 = PipelineFactory() assistant = OpenAiAssistantFactory() pipeline_v2 = pipeline_v1.create_new_version() - NodeFactory(pipeline=pipeline_v2, type="AssistantNode", params={"assistant_id": assistant.id}) + NodeFactory(pipeline=pipeline_v2, type="AssistantNode", params={"assistant_id": str(assistant.id)}) exp_v1 = ExperimentFactory() exp_v2 = exp_v1.create_new_version() exp_v2.pipeline = pipeline_v2 @@ -162,7 +162,7 @@ def test_archive_assistant_fails_with_working_related_versioned_pipeline_and_wor def test_archive_assistant_fails_with_working_related_pipeline(self): pipeline = PipelineFactory() assistant = OpenAiAssistantFactory() - NodeFactory(pipeline=pipeline, type="AssistantNode", params={"assistant_id": assistant.id}) + NodeFactory(pipeline=pipeline, type="AssistantNode", params={"assistant_id": str(assistant.id)}) assert pipeline.is_working_version is True assistant.archive()