-
Notifications
You must be signed in to change notification settings - Fork 399
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
Docs: agents and assistants #2295
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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: { |
There was a problem hiding this comment.
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.
Co-authored-by: Alissa Renz <arenz@slack-corp.com>
There was a problem hiding this 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!
docs/content/basic/app-assistant.md
Outdated
}); | ||
``` | ||
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/content/basic/app-assistant.md
Outdated
|
||
```ts | ||
const assistant = new Assistant({ | ||
threadContextStore: { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
docs/content/basic/app-assistant.md
Outdated
|
||
```ts | ||
const assistant = new Assistant({ | ||
threadContextStore: { |
There was a problem hiding this comment.
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.
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:
git checkout docs-luke-assistants
cd docs
npm i
npm run start
or just read the markdown
Requirements (place an
x
in each[ ]
)