-
Notifications
You must be signed in to change notification settings - Fork 288
Add 'agent: BoltAgent' listener argument #1437
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
Changes from 9 commits
fadc1a4
10c56e9
e6e456a
d0752cb
c332128
1a73ac4
7722f3e
c7e0089
5335855
cf5ef98
724ea5f
0492aa0
3a85cd5
f32af71
29ffbb8
c2a590d
f10f94d
721b634
0195ee2
3d7f305
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,9 @@ venv/ | |
| .venv* | ||
| .env/ | ||
|
|
||
| # claude | ||
| .claude/*.local.json | ||
|
|
||
| # codecov / coverage | ||
| .coverage | ||
| cov_* | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| from .agent import BoltAgent | ||
|
|
||
| __all__ = [ | ||
| "BoltAgent", | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||||||||
| from typing import Optional | ||||||||||
|
|
||||||||||
| from slack_sdk import WebClient | ||||||||||
| from slack_sdk.web.chat_stream import ChatStream | ||||||||||
|
|
||||||||||
|
|
||||||||||
| class BoltAgent: | ||||||||||
| """Agent listener argument for building AI-powered Slack agents. | ||||||||||
|
|
||||||||||
| Experimental: | ||||||||||
| This API is experimental and may change in future releases. | ||||||||||
|
Comment on lines
+10
to
+11
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: We're using an "Experimental" warning while we developer this feature. Rather than working on a long-standing branch, we'd like to merge into |
||||||||||
|
|
||||||||||
| FIXME: chat_stream() only works when thread_ts is available (DMs and threaded replies). | ||||||||||
| It does not work on channel messages because ts is not provided to BoltAgent yet. | ||||||||||
|
Comment on lines
+13
to
+14
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Important callout. I'd like to add |
||||||||||
|
|
||||||||||
| @app.event("app_mention") | ||||||||||
| def handle_mention(agent): | ||||||||||
| stream = agent.chat_stream() | ||||||||||
| stream.append(markdown_text="Hello!") | ||||||||||
| stream.stop() | ||||||||||
| """ | ||||||||||
|
|
||||||||||
| def __init__( | ||||||||||
| self, | ||||||||||
| *, | ||||||||||
| client: WebClient, | ||||||||||
| channel_id: Optional[str] = None, | ||||||||||
| thread_ts: Optional[str] = None, | ||||||||||
| team_id: Optional[str] = None, | ||||||||||
| user_id: Optional[str] = None, | ||||||||||
| ): | ||||||||||
| self._client = client | ||||||||||
| self._channel_id = channel_id | ||||||||||
| self._thread_ts = thread_ts | ||||||||||
| self._team_id = team_id | ||||||||||
| self._user_id = user_id | ||||||||||
|
|
||||||||||
| def chat_stream( | ||||||||||
| self, | ||||||||||
| *, | ||||||||||
| channel: Optional[str] = None, | ||||||||||
| thread_ts: Optional[str] = None, | ||||||||||
| recipient_team_id: Optional[str] = None, | ||||||||||
| recipient_user_id: Optional[str] = None, | ||||||||||
| **kwargs, | ||||||||||
| ) -> ChatStream: | ||||||||||
| """Creates a ChatStream with defaults from event context. | ||||||||||
|
|
||||||||||
| Each call creates a new instance. Create multiple for parallel streams. | ||||||||||
|
|
||||||||||
| Args: | ||||||||||
| channel: Channel ID. Defaults to the channel from the event context. | ||||||||||
| thread_ts: Thread timestamp. Defaults to the thread_ts from the event context. | ||||||||||
| recipient_team_id: Team ID of the recipient. Defaults to the team from the event context. | ||||||||||
| recipient_user_id: User ID of the recipient. Defaults to the user from the event context. | ||||||||||
| **kwargs: Additional arguments passed to ``WebClient.chat_stream()``. | ||||||||||
|
|
||||||||||
| Returns: | ||||||||||
| A new ``ChatStream`` instance. | ||||||||||
| """ | ||||||||||
| resolved_channel = channel or self._channel_id | ||||||||||
| resolved_thread_ts = thread_ts or self._thread_ts | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering here if falling back to the class instances Like if a developer only passes one of these parameters and are unaware of the the fallback values they could end up chat streaming to the wrong location?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 i agree with @WilliamBergamin!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @WilliamBergamin and @srtaalej for your thorough review! I really appreciate that you're actually taking the time to think through the logic, rather than glance over the source code. 🙇🏻 I agree, a mismatch of arguments could lead to bugs that are difficult to diagnose. An all-or-nothing approach would be more fitting, where the developer either relies on the default values set by the context or provides all of the arguments. If a partial set of arguments are provided, then we can raise an error like @srtaalej suggested. Commit 724ea5f disallows partial overrides of the arguments and I've added some tests around it. 🚀
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🐣 ramble: I'm following this discussion curious! I agree the confusion of unknown defaults isn't good, but find extra effort in gathering the details I'm not changing... At the moment the Not a blocker at all for me but I want to share thought!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zimeg Totally fair point! My best suggestion at moving forward is that it's easier to loosen constraints than tighten them later. So, if we start with the "all or nothing" approach above, then we can loosen it when we come up against your advanced use-cases. 🤔 |
||||||||||
| if resolved_channel is None: | ||||||||||
| raise ValueError( | ||||||||||
| "channel is required: provide it as an argument or ensure channel_id is set in the event context" | ||||||||||
| ) | ||||||||||
| if resolved_thread_ts is None: | ||||||||||
| raise ValueError( | ||||||||||
| "thread_ts is required: provide it as an argument or ensure thread_ts is set in the event context" | ||||||||||
| ) | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
🪓 thought(non-blocking): Perhaps against above comments too, but I have small preference to pass missing values to the API for errors from upstream. I realize though that catching error earlier might be preferred, and we have validation logic for similar parameters too!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
| return self._client.chat_stream( | ||||||||||
| channel=resolved_channel, | ||||||||||
| thread_ts=resolved_thread_ts, | ||||||||||
| recipient_team_id=recipient_team_id or self._team_id, | ||||||||||
| recipient_user_id=recipient_user_id or self._user_id, | ||||||||||
| **kwargs, | ||||||||||
| ) | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| from typing import Optional | ||
|
|
||
| from slack_sdk.web.async_client import AsyncWebClient | ||
| from slack_sdk.web.async_chat_stream import AsyncChatStream | ||
|
|
||
|
|
||
| class AsyncBoltAgent: | ||
| """Async agent listener argument for building AI-powered Slack agents. | ||
|
|
||
| Experimental: | ||
| This API is experimental and may change in future releases. | ||
|
|
||
| @app.event("app_mention") | ||
| async def handle_mention(agent): | ||
| stream = await agent.chat_stream() | ||
| await stream.append(markdown_text="Hello!") | ||
| await stream.stop() | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| client: AsyncWebClient, | ||
| channel_id: Optional[str] = None, | ||
| thread_ts: Optional[str] = None, | ||
| team_id: Optional[str] = None, | ||
| user_id: Optional[str] = None, | ||
| ): | ||
| self._client = client | ||
| self._channel_id = channel_id | ||
| self._thread_ts = thread_ts | ||
| self._team_id = team_id | ||
| self._user_id = user_id | ||
|
|
||
| async def chat_stream( | ||
| self, | ||
| *, | ||
| channel: Optional[str] = None, | ||
| thread_ts: Optional[str] = None, | ||
| recipient_team_id: Optional[str] = None, | ||
| recipient_user_id: Optional[str] = None, | ||
| **kwargs, | ||
| ) -> AsyncChatStream: | ||
| """Creates an AsyncChatStream with defaults from event context. | ||
|
|
||
| Each call creates a new instance. Create multiple for parallel streams. | ||
|
|
||
| Args: | ||
| channel: Channel ID. Defaults to the channel from the event context. | ||
| thread_ts: Thread timestamp. Defaults to the thread_ts from the event context. | ||
| recipient_team_id: Team ID of the recipient. Defaults to the team from the event context. | ||
| recipient_user_id: User ID of the recipient. Defaults to the user from the event context. | ||
| **kwargs: Additional arguments passed to ``AsyncWebClient.chat_stream()``. | ||
|
|
||
| Returns: | ||
| A new ``AsyncChatStream`` instance. | ||
| """ | ||
| resolved_channel = channel or self._channel_id | ||
| resolved_thread_ts = thread_ts or self._thread_ts | ||
| if resolved_channel is None: | ||
| raise ValueError( | ||
| "channel is required: provide it as an argument or ensure channel_id is set in the event context" | ||
| ) | ||
| if resolved_thread_ts is None: | ||
| raise ValueError( | ||
| "thread_ts is required: provide it as an argument or ensure thread_ts is set in the event context" | ||
| ) | ||
| return await self._client.chat_stream( | ||
| channel=resolved_channel, | ||
| thread_ts=resolved_thread_ts, | ||
| recipient_team_id=recipient_team_id or self._team_id, | ||
| recipient_user_id=recipient_user_id or self._user_id, | ||
| **kwargs, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from typing import Optional | ||
| from typing import TYPE_CHECKING, Optional | ||
|
|
||
| from slack_sdk.web.async_client import AsyncWebClient | ||
|
|
||
|
|
@@ -15,6 +15,9 @@ | |
| from slack_bolt.context.set_title.async_set_title import AsyncSetTitle | ||
| from slack_bolt.util.utils import create_copy | ||
|
|
||
| if TYPE_CHECKING: | ||
| from slack_bolt.agent.async_agent import AsyncBoltAgent | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL: TYPE_CHECKING, it seems awesome and maybe we can use it elsewhere to improve our types checking for async stuff IIUC this is to ensure that we
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was my main motivation to add it. At first, I felt like it was the wrong approach, but it seems be correct and necessary with the Async modules. |
||
|
|
||
| class AsyncBoltContext(BaseContext): | ||
| """Context object associated with a request from Slack.""" | ||
|
|
@@ -187,6 +190,34 @@ async def handle_button_clicks(context): | |
| self["fail"] = AsyncFail(client=self.client, function_execution_id=self.function_execution_id) | ||
| return self["fail"] | ||
|
|
||
| @property | ||
| def agent(self) -> "AsyncBoltAgent": | ||
| """`agent` listener argument for building AI-powered Slack agents. | ||
|
|
||
| Experimental: | ||
| This API is experimental and may change in future releases. | ||
|
|
||
| @app.event("app_mention") | ||
| async def handle_mention(agent): | ||
| stream = await agent.chat_stream() | ||
| await stream.append(markdown_text="Hello!") | ||
| await stream.stop() | ||
|
|
||
| Returns: | ||
| `AsyncBoltAgent` instance | ||
| """ | ||
| if "agent" not in self: | ||
| from slack_bolt.agent.async_agent import AsyncBoltAgent | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👁️🗨️ question: Perhaps similar to above discussion, but I'm wondering if we can comment inline a quick note for this moreso dynamic import? I notice similar pattern in the following file and would be tempted to move this without thinking too much...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➕
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this @zimeg. I agree, I'd probably be the developer who "refactored" the code to readability only to introduce an unintended side effect. Commit c2a590d adds a comment. I'm not 100% certain that we'll keep the |
||
|
|
||
| self["agent"] = AsyncBoltAgent( | ||
| client=self.client, | ||
| channel_id=self.channel_id, | ||
| thread_ts=self.thread_ts, | ||
| team_id=self.team_id, | ||
| user_id=self.user_id, | ||
| ) | ||
| return self["agent"] | ||
|
|
||
| @property | ||
| def set_title(self) -> Optional[AsyncSetTitle]: | ||
| return self.get("set_title") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ class BaseContext(dict): | |
| "set_status", | ||
| "set_title", | ||
| "set_suggested_prompts", | ||
| "agent", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪬 quibble: I'm wanting this to be alphabetical so much ahhaha!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌚 question: I'm curious if This is unclear to me since the functions and values above seem to map well to details included with an incoming request, but I'd find it odd to access streamer = context.agent.chat_stream()It might just be unfamiliar to me! I'm also unsure if the same message context can be gathered from the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zimeg Fantastic comment and catch! 🎣 I agree, we don't want the I wish I would have noticed this sooner because the refactor removed a lot of code and simplified the implementation and moves all of the agent construction to a single place (kwargs injection) 😅 Commit f10f94d removes |
||
| ] | ||
| # Note that these items are not copyable, so when you add new items to this list, | ||
| # you must modify ThreadListenerRunner/AsyncioListenerRunner's _build_lazy_request method to pass the values. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ def build_async_required_kwargs( | |
| error: Optional[Exception] = None, # for error handlers | ||
| next_keys_required: bool = True, # False for listeners / middleware / error handlers | ||
| ) -> Dict[str, Any]: | ||
| all_available_args = { | ||
| all_available_args: Dict[str, Any] = { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: This fixed a linter warning |
||
| "logger": logger, | ||
| "client": request.context.client, | ||
| "req": request, | ||
|
|
@@ -83,6 +83,10 @@ def build_async_required_kwargs( | |
| if k not in all_available_args: | ||
| all_available_args[k] = v | ||
|
|
||
| # Defer agent creation to avoid constructing AsyncBoltAgent on every request | ||
| if "agent" in required_arg_names or "args" in required_arg_names: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noticed this but why would we want to initialize this if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WilliamBergamin thanks for catching this! The However, I agree we should only construct the agent when It also simplifies our implementation further. 🧹 ✨
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit 721b634 removes the |
||
| all_available_args["agent"] = request.context.agent | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clever 💯 I like this and wonder if we should follow this pattern for other keyword arguments 🚀 What do you think about including a warning here that informs developers that the agent is an experimental feature subject to change? From what I can tell we should be able to do this by taking advantage of the FutureWarning like this import warnings
class ExperimentalWarning(FutureWarning):
"""Warning for features that are still in experimental phase."""
pass
warnings.warn(
"agent is experimental and may change in future versions.",
category=ExperimentalWarning,
stacklevel=2
)IIUC every time a handler processes a request and uses the "agent" kwargs, this warning would be printed, this might be a bit annoying but it would be clear
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea, @WilliamBergamin! I didn't know about Commit cf5ef98 adds the above suggestion and implements it. I added you as a co-author because I took your code verbatim. 👌🏻
It looks like there is a Warning Filter and the default is to only print the first occurrence. Personally, I'd be okay with printing on every occurrence since it's an experimental feature, but my implementation is using the default.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🙂 btw, I pulled this idea from the |
||
|
|
||
| if len(required_arg_names) > 0: | ||
| # To support instance/class methods in a class for listeners/middleware, | ||
| # check if the first argument is either self or cls | ||
|
|
@@ -102,7 +106,7 @@ def build_async_required_kwargs( | |
| for name in required_arg_names: | ||
| if name == "args": | ||
| if isinstance(request, AsyncBoltRequest): | ||
| kwargs[name] = AsyncArgs(**all_available_args) # type: ignore[arg-type] | ||
| kwargs[name] = AsyncArgs(**all_available_args) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: The above fix allows us to remove this type ignore |
||
| else: | ||
| logger.warning(f"Unknown Request object type detected ({type(request)})") | ||
|
|
||
|
|
||

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.
🤔 Maybe we should wait until the feature is GA before exporting it here?
Developers should still be able to import the class directly with something like
from slack_bolt.agent import BoltAgentThere 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.
🤔 If possible, I'd prefer to export the
BoltAgentnow.ExperimentalWarningmakes it clear that the developer is using something that's unstable and at their own risk.👉🏻 That said, I trust you decision over ours since you're more experienced with Bolt Python. If it makes you feel more comfortable waiting until the feature is GA, just let me know.