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

feat(product-assistant): a human in the loop for the taxonomy agent #26767

Merged
merged 77 commits into from
Dec 17, 2024

Conversation

skoob13
Copy link
Contributor

@skoob13 skoob13 commented Dec 9, 2024

Problem

If the request is ambiguous or the taxonomy doesn't provide a clear picture, we should ask the user to clarify their request.

Demo
2024-12-12 19 47 34

Changes

  • Add a checkpointer implementation on Django for LangGraph.
  • Threads are now stored in the database (+ checkpoints, checkpoint writes, and checkpoint blobs).
  • Implement a human in the loop for the taxonomy agent.
  • Refactor how we restore a conversation since human messages can now appear in any order.
  • Move the assistant to its endpoint.
  • Use Pydantic for the state to simplify further state serialization.
  • Add more tests.

Further work:

  • Improve serialization of checkpoints. I've decided to split these PRs because this appears to be more complicated than I expected.
  • Improve frontend UI, so users can easily search and add specific events to a prompt.
  • Add eval tests.

Does this work well for both Cloud and self-hosted?

No

How did you test this code?

Manual testing, Storybook, more unit tests.


class Conversation(UUIDModel):
user = models.ForeignKey(User, on_delete=models.CASCADE)
team = models.ForeignKey(Team, on_delete=models.CASCADE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, I think that there should be a ref to the project because property values can differ. @Twixes what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We can't really have a meaningful analysis shared between environments because of data differing, so team-level (aka environment-level) is the right way here

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

10 snapshot changes in total. 0 added, 10 modified, 0 deleted:

  • chromium: 0 added, 10 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@skoob13 skoob13 requested a review from Twixes December 12, 2024 18:59
@skoob13 skoob13 marked this pull request as ready for review December 12, 2024 18:59
@skoob13
Copy link
Contributor Author

skoob13 commented Dec 16, 2024

The CI fails because there is a bug in migrations tests, which I've fixed in #26928.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

A mammoth PR so I might have missed something, but overall looks good, with a few comments!


class Conversation(UUIDModel):
user = models.ForeignKey(User, on_delete=models.CASCADE)
team = models.ForeignKey(Team, on_delete=models.CASCADE)
Copy link
Member

Choose a reason for hiding this comment

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

We can't really have a meaningful analysis shared between environments because of data differing, so team-level (aka environment-level) is the right way here

frontend/src/types.ts Show resolved Hide resolved
ee/urls.py Outdated Show resolved Hide resolved
ee/urls.py Outdated Show resolved Hide resolved
ee/hogai/utils/types.py Outdated Show resolved Hide resolved
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@skoob13 skoob13 requested a review from Twixes December 17, 2024 11:50
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Let's get this in

Copy link
Member

@Twixes Twixes Dec 17, 2024

Choose a reason for hiding this comment

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

Not important to shipping, but could be useful in terms of maintainability: Would it make sense to split this out into its own PyPI package, langgraph_checkpoint_django? This part seems pretty much abstract, independent from PostHog specifics, could be useful for others building on Django too

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Let's get this in

Twixes

This comment was marked as duplicate.

@Twixes
Copy link
Member

Twixes commented Dec 17, 2024

dashboard.cy.ts has failed 4 times, so something seems to be up 🤔 @skoob13

@skoob13 skoob13 merged commit 738fa46 into master Dec 17, 2024
96 checks passed
@skoob13 skoob13 deleted the feat/agent-human-in-the-loop branch December 17, 2024 16:28
Copy link

sentry-io bot commented Dec 19, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ IntegrityError: duplicate key value violates unique constraint "unique_checkpoint_write" /api/environments/{parent_lookup_team_id}/conve... View Issue
  • ‼️ IntegrityError: duplicate key value violates unique constraint "unique_checkpoint_write" ee.hogai.django_checkpoint.checkpointer in put_... View Issue
  • ‼️ IntegrityError: null value in column "checkpoint_ns" of relation "ee_conversationcheckpointblob" violates not-nul... ee.hogai.django_checkpoint.checkpointer in put View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants