Skip to content
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

fix: use timestamp passed by request for user message created date #1503

Merged
merged 7 commits into from
Jul 4, 2024

Conversation

goetzrobin
Copy link
Contributor

we pass the timestamp through until we reach the agent's step function this function calls Message.dict_to_message(), where we pass the timestamp as argument for created_at, which is then persisted into the DB

Please describe the purpose of this pull request.
Is it to add a new feature? Is it to fix a bug?

How to test
How can we test your PR during review? What commands should we run? What outcomes should we expect?
After merging the dev portal changes and creating a new agent duplicated messages should not show up anymore

Have you tested this PR?
Have you tested the latest commit on the PR? If so please provide outputs from your tests.

Related issues or PRs
Please link any related GitHub issues or PRs.

Is your PR over 500 lines of code?
If so, please break up your PR into multiple smaller PRs so that we can review them quickly, or provide justification for its length.

Additional context
Add any other context or screenshots about the PR here.

we pass the timestamp through until we reach the agent's step function
this function calls Message.dict_to_message(), where we pass the timestamp as
argument for created_at, which is then persisted into the DB
the sort expression should not change even if we reverse the order of the items
a date before should still be a date before the initial sort takes care of
everything we need already
@cpacker cpacker requested review from cpacker and sarahwooders July 4, 2024 18:29
@cpacker
Copy link
Collaborator

cpacker commented Jul 4, 2024

Pending tests LGTM.

Though I think at a later point we may want to revisit the concept of allowing the user/client to specify the timestamp to be stored server-side since it feels like a security risk. Instead maybe we can return the timestamp (and other necessary metadata) in the response of the POST call, which should effectively allow the client (eg chat UI) to get a server-verified timestamp immediately after POST.

Thoughts @goetzrobin ?

@goetzrobin
Copy link
Contributor Author

@cpacker I agree. I like that or maybe have the first streamed event return metadata like an id & timestamp? I do think moving forward we definitely want to have this server driven.

@@ -314,19 +314,13 @@ def get_all_cursor(
# cursor logic: filter records based on before/after ID
if after:
after_value = getattr(self.get(id=after), order_by)
if reverse: # if reverse, then we want to get records that are less than the after_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably keep reverse in, right? Was this just removed for debugging purposes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@goetzrobin I reverted the change on this file - I'm assuming that the diff isn't actually needed in this PR?

memgpt/server/server.py Outdated Show resolved Hide resolved
@cpacker
Copy link
Collaborator

cpacker commented Jul 4, 2024

@cpacker I agree. I like that or maybe have the first streamed event return metadata like an id & timestamp? I do think moving forward we definitely want to have this server driven.

Gotcha. Let's merge in this PR for now, but deprecate the ability to pass in timestamp via the API call in a subsequent PR: #1505

@cpacker cpacker merged commit 811e5b2 into letta-ai:main Jul 4, 2024
9 of 11 checks passed
@cpacker cpacker mentioned this pull request Jul 4, 2024
mattzh72 pushed a commit that referenced this pull request Oct 9, 2024
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.

2 participants