-
Notifications
You must be signed in to change notification settings - Fork 850
feat: add task_display_mode option to the start of chat streams #1820
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: add task_display_mode option to the start of chat streams #1820
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-ai-apps-thinking-steps #1820 +/- ##
==============================================================
Coverage ? 83.96%
==============================================================
Files ? 116
Lines ? 13212
Branches ? 0
==============================================================
Hits ? 11094
Misses ? 2118
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| streaming to channels. | ||
| recipient_user_id: The encoded ID of the user to receive the streaming text. Required when streaming to channels. | ||
| task_display_mode: Specifies how tasks are displayed in the message. A "timeline" displays individual tasks | ||
| interleaved with text and "plan" displays all tasks together. |
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
WilliamBergamin
left a comment
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.
Looking good 💯 left one commend one comment on enums and error message from the backend
| recipient_team_id: Optional[str] = None, | ||
| recipient_user_id: Optional[str] = None, | ||
| chunks: Optional[Sequence[Union[Dict, Chunk]]] = None, | ||
| task_display_mode: Optional[str] = None, # timeline, plan |
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.
(take it or leave it) we could potentially use enums here, but if it does not fall in line with existing patterns we shouldn't do it
Edit: actually maybe it would be worth having the web API return a proper error message with the possible options if an invalid value is passed
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.
Agree, I think it's good to expect the API return an error message with the the available options when an invalid option value is passed.
The display modes are aiming to support what's common in LLM UIs, so I imagine there will be more in the future that Slack will want to support. It would be nice for developers to have access to the new modes without upgrading the SDK version (I'm assuming an enum would require an SDK upgrade to support the new options?)
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.
@WilliamBergamin @mwbrooks Oh dang, the tradeoffs are fascinating! 😳
I think I agree letting the API error for invalid values might be best for ongoing maintenance and I hope for now this comment is quick enough to jump to for fast reference. I'll update some notes to avoid using enums in adjacent changes too 📚
Prior art for me was found just above:
python-slack-sdk/slack_sdk/web/client.py
Line 2765 in efb7ba2
| parse: Optional[str] = None, # none, full |
mwbrooks
left a comment
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.
✅ Thanks for adding the task_display_mode to the chat_startStream and chat_stream helper methods!
🧪 Local testing works for both task_display_mode="plan" and task_display_mode="timeline".
🧠 Small aside, but seeing the same code displayed in both plan and timeline has helped me better understand the UX differences between these two. I wonder if it would help other developers as well (e.g. in our documentation)? .cc @lukegalbraithrussell
Co-authored-by: Michael Brooks <[email protected]>
|
@srtaalej @WilliamBergamin @mwbrooks Fascinating discussion again! Let's hold off on enums for these upcoming versions but keep documentation current 📚 ✨
We might have the kind Block Kit Builder to use for showing this quick in examples, but I'm hoping the sample "examples" repos can be useful too! Let's merge this 🐞 🔍 👻 |
Summary
This PR adds the
task_display_modeoption to the start of chat streams.Testing
Use the following to start a stream with either a "plan" or "timeline" task display mode:
Category
/docs(Documents)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.