Skip to content

Comments

Docs: agents and assistants#2295

Merged
lukegalbraithrussell merged 12 commits intomainfrom
docs-luke-assistants
Nov 1, 2024
Merged

Docs: agents and assistants#2295
lukegalbraithrussell merged 12 commits intomainfrom
docs-luke-assistants

Conversation

@lukegalbraithrussell
Copy link
Contributor

@lukegalbraithrussell lukegalbraithrussell commented Oct 17, 2024

Summary

This adds a page for utilizing assistants/agents. I ended up writing a lot, then removing most of it, and now we're here. I think this might still be too much but I am open to adding/removing/modifying/etcing

I built this off of words from Alissa and Kaz — once we're good to go here, however this ends up, I'll tweak the Python page to align

To get a local version of the site:

  1. git checkout docs-luke-assistants
  2. cd docs
  3. npm i
  4. npm run start

or just read the markdown

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.49%. Comparing base (9bcc680) to head (afc9ef6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2295   +/-   ##
=======================================
  Coverage   92.49%   92.49%           
=======================================
  Files          36       36           
  Lines        7472     7472           
  Branches      651      651           
=======================================
  Hits         6911     6911           
  Misses        553      553           
  Partials        8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@misscoded misscoded left a comment

Choose a reason for hiding this comment

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

Very nice! Left some comments and suggestions, and I echo @seratch's suggestion that perhaps the Reference section can be the home of the table and we can link directly to that instead.

You can store context through the `threadContextStore` property but it must feature `get` and `save` methods.

```js
threadContextStore: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this property either at the top or the bottom of the instantiation: it's not part of the "main" events, and is an addition to make life easier.

@misscoded misscoded mentioned this pull request Oct 17, 2024
2 tasks
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Left a first round of comments, thanks for tackling this!

});
```

You can store context through the `threadContextStore` property but it must feature `get` and `save` methods. If not provided, a `DefaultThreadContextStore` instance is utilized instead, which is a reference implementation that relies on storing and retrieving message metadata as the context changes.
Copy link
Contributor

@filmaj filmaj Oct 18, 2024

Choose a reason for hiding this comment

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

I think we need to define what 'context' means here. It is such a loaded term in programming in general but also just in Bolt: there is a separate advanced Bolt concept "Adding context" that is different - but maybe related? - from the context concept in this doc! I'd probably break out defining context into its own sub-section.

@misscoded how should we reconcile the two ideas of "context" here? Maybe it's the same idea just extended? Not sure, but we probably need to elaborate somewhat.

Another thing: we should describe why developers might need a context store. This was one of the first questions that came up for me when I reviewed this functionality in Bolt earlier this week, and @seratch provided a great answer that I think we should leverage in these docs. The key for me was: while the two assistant_* events do provide the Slack-client context information (that is, where is the end-user situated within the workspace when they interact with the Assistant view, e.g. what channel do they have open side-by-side with the assistant view?), crucially the assistant-thread-message events delivered to the app do not. Therefore, as a developer, if you're trying to build a nice experience that flows across all three assistant events, you likely have to keep track of this context as different combinations of these three assistant events get delivered to your app. The context store, then, is more about filling that context gap when assistant message events get delivered to an app.

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 it's important whenever talking about context as it relates to an Assistant as "thread context" to distinguish it from the event context that is otherwise provided.


```ts
const assistant = new Assistant({
threadContextStore: {

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default one is purely in-memory, right? If so, it will not work well when used with the AwsLambdaReceiver, since each event invocation spins up a fresh Bolt instance, meaning the default thread context store will be empty each time. If so, we might need a callout here to instruct users of the AwsLambdaReceiver to not use the default one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@filmaj the default one uses the bot's first reply's message metadata to remember the latest context data, so you can safely use it for any situation.

Copy link
Contributor

@seratch seratch left a comment

Choose a reason for hiding this comment

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

My final comments: as mentioned in one of the comments, let's merge this once the bolt-js release enabling the metadata setting for the say method is done.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM, left a few comments.


```ts
const assistant = new Assistant({
threadContextStore: {
Copy link
Contributor

Choose a reason for hiding this comment

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

The default one is purely in-memory, right? If so, it will not work well when used with the AwsLambdaReceiver, since each event invocation spins up a fresh Bolt instance, meaning the default thread context store will be empty each time. If so, we might need a callout here to instruct users of the AwsLambdaReceiver to not use the default one.

@lukegalbraithrussell lukegalbraithrussell merged commit 3d7f242 into main Nov 1, 2024
@lukegalbraithrussell lukegalbraithrussell deleted the docs-luke-assistants branch November 1, 2024 19:34
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.

5 participants