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.

Sign up for GitHub

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

Jump to bottom

Add `GriptapeCloudConversationMemoryDriver` #1063

Merged

vachillo merged 1 commit into dev from conv_mem_cloud

Aug 19, 2024

Member

vachillo commented Aug 15, 2024 •

edited

Loading

I have read and agree to the contributing guidelines for submitting new pull requests.

Describe your changes

Add GriptapeCloudConversationMemoryDriver
Fix recursive loading issue in other conversation memory drivers

Issue ticket number and link

codecov bot commented Aug 15, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ation/griptape_cloud_conversation_memory_driver.py	98.03%	0 Missing and 1 partial ⚠️
...y/conversation/redis_conversation_memory_driver.py	80.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

vachillo force-pushed the conv_mem_cloud branch from e381255 to 873c919 Compare

August 15, 2024 19:31

collindutter reviewed

View reviewed changes

griptape/clients/griptape_cloud_api_client.py Outdated Show resolved Hide resolved

griptape/clients/griptape_cloud_api_client.py Outdated Show resolved Hide resolved

griptape/clients/griptape_cloud_api_client.py Outdated Show resolved Hide resolved

collindutter reviewed

View reviewed changes

griptape/clients/griptape_cloud_api_client.py Outdated Show resolved Hide resolved

collindutter reviewed

View reviewed changes

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated Show resolved Hide resolved

vachillo force-pushed the conv_mem_cloud branch 2 times, most recently from 2e9557f to 819e2c9 Compare

August 16, 2024 21:52

vachillo marked this pull request as ready for review

August 16, 2024 21:52

vachillo requested a review from collindutter

August 16, 2024 21:52

vachillo force-pushed the conv_mem_cloud branch from 819e2c9 to 805e8f8 Compare

August 16, 2024 21:53

collindutter requested changes

View reviewed changes

docs/griptape-framework/drivers/conversation-memory-drivers.md Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated

Comment on lines 61 to 64

+                      messages = [
+                          {"input": json.dumps(run.input.to_dict()), "output": json.dumps(run.output.to_dict())}
+                          for run in memory.runs
+                      ]

Member

collindutter Aug 16, 2024

Can be simplified to:

        messages = [
            {"input": run.input.to_json(), "output": run.output.to_json()}
            for run in memory.runs
        ]

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated

+                      )
+                      response.raise_for_status()
+                  def load(self) -> Optional[BaseConversationMemory]:

Member

collindutter Aug 16, 2024

Should this be Optional?

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated

Comment on lines 86 to 87

		if messages_response is None:
		raise RuntimeError(f"Error getting messages for thread {self.thread_id}")

Member

collindutter Aug 16, 2024

When would this happen? Shouldn't raise_for_status handle this?

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Show resolved Hide resolved

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py Outdated

+                      # remove runs because they are already stored as Messages
+                      metadata = memory.to_dict()
+                      del metadata["runs"]
+                      metadata = json.dumps(metadata)

Member

collindutter Aug 16, 2024

More of a comment on Cloud but it seems like metadata should be a dictionary. What do we gain from it being a string?

Member Author

vachillo Aug 16, 2024 •

edited

Loading

~~more flexible. it could be used as metadata="test" for all we care~~ we do actually care and it is a dict type

griptape/drivers/memory/conversation/griptape_cloud_conversation_memory_driver.py

+                              {
+                                  **json.loads(thread_response.get("metadata")),
+                                  "runs": [run.to_dict() for run in runs],
+                                  "autoload": False,

Member

collindutter Aug 16, 2024

For visibility of offline conversation, why is this necessary in this Driver but not others?

Member Author

vachillo Aug 16, 2024

no idea

Member

collindutter Aug 16, 2024

Can you confirm that this is still necessary? Or can you test other Drivers to see if they should have this too?

Member Author

vachillo Aug 19, 2024

updated other drivers with this fix

tests/unit/drivers/memory/conversation/test_griptape_cloud_conversation_memory_driver.py Outdated

Comment on lines 15 to 42

+                      def get(*args, **kwargs):
+                          if str(args[0]).startswith("https://cloud.griptape.ai/api/threads/"):
+                              if str(args[0]).endswith("/messages"):
+                                  thread_id = args[0].split("/")[-2]
+                                  if thread_id == "test_error":
+                                      return mocker.Mock(raise_for_status=lambda: None, json=lambda: None)
+                                  return mocker.Mock(
+                                      raise_for_status=lambda: None,
+                                      json=lambda: {
+                                          "messages": [
+                                              {
+                                                  "message_id": "123",
+                                                  "input": '{"type": "TextArtifact", "id": "1234", "value": "Hi There, Hello"}',
+                                                  "output": '{"type": "TextArtifact", "id": "123", "value": "Hello! How can I assist you today?"}',
+                                                  "index": 0,
+                                              }
+                                          ]
+                                      },
+                                  )
+                              else:
+                                  thread_id = args[0].split("/")[-1]
+                                  return mocker.Mock(
+                                      raise_for_status=lambda: None,
+                                      json=lambda: {"metadata": TEST_CONVERSATION, "name": "test", "thread_id": "test_metadata"}
+                                      if thread_id == "test_metadata"
+                                      else {"name": "test", "thread_id": "test"},
+                                  )
+                          return None

Member

collindutter Aug 16, 2024

Might be better to split this over two mocked requests clients.

tests/unit/drivers/memory/conversation/test_griptape_cloud_conversation_memory_driver.py Outdated

+                  @pytest.fixture(autouse=True)
+                  def _mock_requests(self, mocker):
+                      def get(*args, **kwargs):
+                          if str(args[0]).startswith("https://cloud.griptape.ai/api/threads/"):

Member

collindutter Aug 16, 2024

When would this ever not evaluate to True in the context of this test?

vachillo force-pushed the conv_mem_cloud branch 3 times, most recently from 55e08d4 to b59f6e3 Compare

August 19, 2024 16:41

vachillo requested a review from collindutter

August 19, 2024 16:41


          squash

74a08a3

vachillo force-pushed the conv_mem_cloud branch from b59f6e3 to 74a08a3 Compare

August 19, 2024 17:41

collindutter approved these changes

View reviewed changes

vachillo changed the title ~~Cloud conversation memory driver~~ Add GriptapeCloudConversationMemoryDriver

vachillo merged commit 52c1930 into dev

13 checks passed

vachillo deleted the conv_mem_cloud branch

August 19, 2024 17:56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet