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

WIP: Pawel/eng 651 threads management #29

Merged
merged 8 commits into from
Jan 5, 2024

Conversation

PawelMorawian
Copy link

No description provided.

Copy link

linear bot commented Dec 28, 2023

ENG-651 Update the Thread management in the sdk

Right now the client.thread decorator is only saving a thread_id in the context.

We need to:

  • create a proper thread object when using the decorator
  • allow devs to send custom tags/metadata/… to the thread through the decorator
  • properly queue the thread upsert into the event processor
  • add a way to get the thread object client.get_current_thread instead of the current client.get_current_thread_id
  • option: document a way to update the thread after having done client.get_current_thread
    • we might simply want to use client.api.update_thread
    • another option is that we decide to send a thread update at the end of the thread with block/decorated function

@PawelMorawian PawelMorawian force-pushed the pawel/eng-651-threads-management branch from d27295e to b0b3619 Compare December 28, 2023 15:04
@PawelMorawian PawelMorawian changed the title WIP: Pawel/eng 651 threads management Pawel/eng 651 threads management Dec 28, 2023
):
self.client = client
if thread_id is None:
thread_id = str(uuid.uuid4())
self.thread_id = thread_id
active_thread_id_var.set(thread_id)
self.thread = Thread(id=thread_id)
needs_upsert = kwargs != {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a fan of the needs_upsert concept. Plus this check is always false because it is going to do a reference comparison right?

Copy link
Author

Choose a reason for hiding this comment

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

"is" keyword is for reference comparison.

>>> k = {}
>>> k == {}
True
>>> k is {}
False
>>> k = {"foo": "bar"}
>>> k == {}
False

Copy link
Author

Choose a reason for hiding this comment

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

I started by doing a hash comparision and only update on exit when the hash is different but decided against it because this solution seems simpler.

Do you have any preferences on how to do this ?

async def save(self):
thread = active_thread_var.get()
thread_data = thread.to_dict()
thread_data.pop("steps", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we keep the steps in the thread class?

Copy link
Author

@PawelMorawian PawelMorawian Dec 28, 2023

Choose a reason for hiding this comment

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

I guess it is to programatically browser the thread's steps
I have no strong opinion about this

@PawelMorawian PawelMorawian force-pushed the pawel/eng-651-threads-management branch 2 times, most recently from be5b87c to 5a18e5f Compare December 28, 2023 17:16
@PawelMorawian PawelMorawian force-pushed the pawel/eng-651-threads-management branch 2 times, most recently from 8ec22c9 to 5a9e466 Compare December 29, 2023 10:28
@@ -34,6 +36,7 @@ def __init__(
self.metadata = metadata
self.tags = tags
self.user = user
self.needs_upsert = bool(metadata or tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think user should trigger an uspert

thread_data = {
"id": thread_data["id"],
"metadata": thread_data.get("metadata"),
"tags": thread_data.get("tags"),
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, why we do not account for user?

@PawelMorawian PawelMorawian force-pushed the pawel/eng-651-threads-management branch from 5a9e466 to a9c0826 Compare December 29, 2023 15:07
@PawelMorawian PawelMorawian force-pushed the pawel/eng-651-threads-management branch from a9c0826 to c33ddcd Compare December 29, 2023 15:15
@willydouhard
Copy link
Contributor

Can you provide an example of what is now possible ? Should we also add this example in the docs?

@PawelMorawian PawelMorawian changed the title Pawel/eng 651 threads management WIP: Pawel/eng 651 threads management Jan 3, 2024
@willydouhard willydouhard merged commit 7a72fc6 into main Jan 5, 2024
2 checks passed
@willydouhard willydouhard deleted the pawel/eng-651-threads-management branch January 5, 2024 14:19
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