Assistant Archival Cleanup #1011

stephherbers · 2024-12-19T16:12:16Z

Description

990
Followup logic changes for assistant versioning archiving.

Now, when a assistant is archived that is a working version, all its versions is also archived and deleted from open AI.

Also from a UI standpoint. When it checks whether is can be archived, if its the working version it checks for all dependencies for all the versions not just the working version and displays everything that needs to be archived first. This way, the version won't be archived with the open AI deleted with something (exp or pipeline) still referencing it)

User Impact

Demo

Docs

…all assitatns before archiving and deleteling those references as well

…assistants when the referenced assistant is going to be archived

codecov-commenter · 2024-12-19T16:18:48Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
854	1	853	0

View the top 1 failed tests by shortest run time

apps/assistants/tests/test_delete.py::TestAssistantArchival::test_archive_assistant_fails_with_working_related_pipeline

Stack Traces | 0.626s run time

.../assistants/tests/test_delete.py:148: in test_archive_assistant_fails_with_working_related_pipeline
    assert assistant.is_archived is False  # archiving failed
E   assert True is False
E    +  where True = <OpenAiAssistant: >.is_archived

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

SmittieC · 2025-01-06T07:43:16Z

apps/assistants/views.py

+            version_query = None
+            if assistant.is_working_version:
+                version_query = list(
+                    map(


I might be wrong, but I don't think we have to map the ids to strings?

SmittieC · 2025-01-06T07:46:35Z

apps/assistants/models.py

        return self.experiment_set.filter(is_archived=False)

-    def get_related_pipeline_node_queryset(self):
+    def get_related_pipeline_node_queryset(self, query=None):


Since the query param is a list of ids, I suggest that we name and type it accordingly:

def get_related_pipeline_node_queryset(self, assistant_ids: list):

We could even pass in a queryset that returns the ids. We did something similar here for example. This wouldn't require parsing the query results to a list, which is nice, but up to you on which approach you'd like to take.

Also, looks like this parameter will come in useful to optimize the archive method. So instead of iterating through all assistant versions (in the case where we're archiving the working version), we can simply fetch the version ids and call this method with it. Same with get_related_experiments_queryset.

snopoke

After reviewing this and testing it locally I think we need to take a step back and look more holistically at how we trace references to objects and make that visible in the UI.

I've started a doc to make it easier to collaborate on the thoughts and ideas: https://docs.google.com/document/d/1Z09GNpO17izoVcRPGSmppNthFruyRJ0033T-RYvapoE/edit?tab=t.0

snopoke · 2025-01-08T11:26:31Z

apps/assistants/tests/test_delete.py

+        experiment = ExperimentFactory(pipeline=pipeline)
+        experiment.assistant = v2_assistant


this isn't a valid state - and experiment should either have an assistant or a pipeline but not both

snopoke · 2025-01-08T11:27:04Z

apps/assistants/tests/test_delete.py

+        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):


This test seems to be the same as the previous one since the presence of the pipeline has no effect because it does't reference the assistant

snopoke · 2025-01-08T11:31:10Z

apps/assistants/tests/test_delete.py

+        pipeline = PipelineFactory()
+        NodeFactory(type="AssistantNode", pipeline=pipeline, params={"assistant_id": str(assistant.id)})
+        exp = ExperimentFactory(pipeline=pipeline)
+        exp.assistant = assistant


you shouldn't set this property since you're testing the reference through the pipeline

snopoke · 2025-01-08T11:32:18Z

apps/assistants/tests/test_delete.py

+        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()


I don't follow what's happening here. Creating a new version should take care of creating versioned nodes as well.

snopoke · 2025-01-08T11:42:25Z

apps/assistants/models.py

+                    or version.get_related_pipeline_node_queryset().exists()
+                ):
+                    return False
+            for version in self.versions.all():


rather than re-iterating over the queryset you could accumulate the IDs in the previous loop

stephherbers · 2025-01-09T20:09:34Z

noting that we are moving forward with changes from the doc so those commits will be coming in later and I will re-requests reviews

…n OR defualt version to display, else we don't need to block

…arity

…riments and pipelines

SmittieC · 2025-01-16T07:26:50Z

apps/assistants/tests/test_delete.py

+    def test_archive_assistant_succeeds_with_released_experiment_experiment(self):
+        experiment = ExperimentFactory()
+        exp_v2 = experiment.create_new_version()
+        experiment.is_default = True


Did you mean to make exp_v2 the default here?

exp_v2.is_default_version = True

Edit: exp_v2 should be exp_v1

SmittieC · 2025-01-16T07:28:21Z

apps/assistants/tests/test_delete.py

+        experiment.is_default = True
+        experiment.save()
+        assistant = OpenAiAssistantFactory()
+        exp_v2 = experiment.create_new_version()


Nit: Since this will be version 2 (exp_v2), the previous exp_v2 should be exp_v1

SmittieC · 2025-01-16T07:45:27Z

apps/assistants/models.py


-    def get_related_pipeline_node_queryset(self):
+    def get_related_pipeline_node_queryset(self, assistant_ids: list = None):


These should check the experiments that the pipeline belongs to instead of the pipeline itself?

SmittieC · 2025-01-16T07:48:58Z

apps/assistants/tests/test_delete.py

-    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 TestAssistantArchival:


Nice tests. It would be great if we can add tests for pipelines using an assistant as well.

stephherbers added 5 commits December 18, 2024 13:25

prefactor: add the deletions into a test class

3753f52

modify archive for assistants such that it checks for references for …

70a78bf

…all assitatns before archiving and deleteling those references as well

add tests

35ce4f0

modify the reference local objects to include those of the versioned …

b5404ef

…assistants when the referenced assistant is going to be archived

add tests for queryset functions

527e24e

stephherbers requested review from snopoke and SmittieC December 19, 2024 16:12

SmittieC reviewed Jan 6, 2025

View reviewed changes

snopoke mentioned this pull request Jan 8, 2025

Convert assistant ID to str when creating pipeline versions #1030

Merged

snopoke reviewed Jan 8, 2025

View reviewed changes

Merge branch 'main' into smh/assistent-deletion-and-archiving-cleanup

742c032

stephherbers added 4 commits January 14, 2025 13:11

add AND to filter to to check if the assistant is also working_versio…

63b5e3c

…n OR defualt version to display, else we don't need to block

rename param and add typing for get related queryset functions for cl…

ea2fac1

…arity

fix logic- checks for working version and default version on the expe…

5fb1bad

…riments and pipelines

tests

bff8ccf

SmittieC reviewed Jan 16, 2025

View reviewed changes

stephherbers added 2 commits January 17, 2025 16:22

add pipeline tests

03020f8

Merge branch 'main' into smh/assistent-deletion-and-archiving-cleanup

eccb21e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assistant Archival Cleanup #1011

Assistant Archival Cleanup #1011

stephherbers commented Dec 19, 2024

codecov-commenter commented Dec 19, 2024 •

edited

Loading

SmittieC Jan 6, 2025

SmittieC Jan 6, 2025 •

edited

Loading

snopoke left a comment

snopoke Jan 8, 2025

snopoke Jan 8, 2025

snopoke Jan 8, 2025

snopoke Jan 8, 2025

snopoke Jan 8, 2025

stephherbers commented Jan 9, 2025

SmittieC Jan 16, 2025 •

edited

Loading

SmittieC Jan 16, 2025

SmittieC Jan 16, 2025

SmittieC Jan 16, 2025

		experiment = ExperimentFactory(pipeline=pipeline)
		experiment.assistant = v2_assistant


		def get_related_pipeline_node_queryset(self):
		def get_related_pipeline_node_queryset(self, assistant_ids: list = None):

Assistant Archival Cleanup #1011

Are you sure you want to change the base?

Assistant Archival Cleanup #1011

Conversation

stephherbers commented Dec 19, 2024

Description

User Impact

Demo

Docs

codecov-commenter commented Dec 19, 2024 • edited Loading

❌ 1 Tests Failed:

Choose a reason for hiding this comment

SmittieC Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

snopoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephherbers commented Jan 9, 2025

SmittieC Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2024 •

edited

Loading

SmittieC Jan 6, 2025 •

edited

Loading

SmittieC Jan 16, 2025 •

edited

Loading