Skip to content

fix(history): add emit fn to transports that appends to chat history#558

Merged
douglas-reid merged 3 commits intomainfrom
doug/no-really-fix-transports-and-chat-history
Sep 28, 2023
Merged

fix(history): add emit fn to transports that appends to chat history#558
douglas-reid merged 3 commits intomainfrom
doug/no-really-fix-transports-and-chat-history

Conversation

@douglas-reid
Copy link
Copy Markdown
Contributor

Existing transports were not appending messages sent back to users to the ChatHistory. This PR attempts to fix that by introducing a builder for an emit function that will do that directly.

dkolas
dkolas previously approved these changes Sep 27, 2023
Copy link
Copy Markdown
Contributor

@dkolas dkolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this seems like a solid improvement on the current.

Non-blocking question though: Should it be the Transport's job to append output to the history, or could that be factored into the Agent itself?

else:
# Do nothing here; this could be a message we intentionally don't want to respond to (ex. an image or file upload)
pass
self.agent_service.run_agent(self.agent_service.get_default_agent(), context)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the response from run_agent always None? Otherwise it seems like this deletes the call to self.send.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, run_agent doesn't return anything.

Comment thread src/steamship/agents/service/agent_service.py Outdated
@douglas-reid
Copy link
Copy Markdown
Contributor Author

Non-blocking question though: Should it be the Transport's job to append output to the history, or could that be factored into the Agent itself?

I couldn't convince myself that the Agent should be charged with knowing what actually gets sent to the user (as the Transport / wrapping package code could decide to modify the response). For instance, it also doesn't tag the incoming user message. That may seem silly / impossibly nit-picky a distinction -- and not worth separation -- however.

Maybe we start here, and see how we can refactor in a bit?

@douglas-reid douglas-reid force-pushed the doug/no-really-fix-transports-and-chat-history branch from 897d020 to 21da088 Compare September 27, 2023 17:12
from steamship.invocable import PackageService, post


def build_context_appending_emit_func(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@douglas-reid douglas-reid added this pull request to the merge queue Sep 27, 2023
Copy link
Copy Markdown
Contributor

@eob eob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 27, 2023
@douglas-reid douglas-reid added this pull request to the merge queue Sep 27, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 27, 2023
@douglas-reid douglas-reid added this pull request to the merge queue Sep 27, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 27, 2023
@douglas-reid douglas-reid added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit f8c1b2e Sep 28, 2023
@douglas-reid douglas-reid deleted the doug/no-really-fix-transports-and-chat-history branch September 28, 2023 02:17
@dahifi
Copy link
Copy Markdown

dahifi commented Sep 28, 2023

This fixed my issue with the widget transport not remembering things between calls. Thank you Doug.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants