-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Implement ChatSteps and ChatStep to show/hide intermediate steps #6617
base: main
Are you sure you want to change the base?
Conversation
I'm overall very much for making progress reporting easy. The first feedback I have is
|
I like the idea of a
I'm not 100% sure that's neceessary since it's a matter of implementing |
The reason i think its important with a strategy for Context managers is that its much easier to use a framework if it follows some general principløs. It's much harder if suddenly one component can be used as a Context manager and the rest of the components cannot. |
Can you think of other components that context managers could be useful for? I imagine indicators, boolean, Card/Accordion. Also, maybe disabled/loading? |
Cleaner layout: Screen.Recording.2024-05-14.at.7.23.52.PM.mov |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6617 +/- ##
==========================================
+ Coverage 81.52% 81.63% +0.11%
==========================================
Files 318 324 +6
Lines 46776 47220 +444
==========================================
+ Hits 38133 38547 +414
- Misses 8643 8673 +30 ☔ View full report in Codecov by Sentry. |
Okay this is ready for review! See https://github.com/holoviz/panel/blob/1fbaac662d8e33887eebac7e215fa441e04c091b/examples/reference/chat/ChatStep.ipynb for usage. I'm not sure if |
Added some new methods while implementing holoviz/lumen#560. Need to change the docs, and also add tests, which I realized I didn't commit it and accidentally removed it locally :( Screen.Recording.2024-05-22.at.11.01.27.PM.mov |
ready for review now Screen.Recording.2024-05-23.at.2.23.24.PM.mov |
panel/chat/feed.py
Outdated
self.stream(steps, user=user, avatar=avatar) | ||
return steps | ||
|
||
def attach_step(self, objects: str | list[str] | None = None, **step_params) -> ChatStep: |
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.
What happens right now if you .stream()
when the last message contains ChatSteps
could that not simply be used to attach a new step?
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.
If this can't be removed I'd suggest renaming maybe, maybe add_step
or stream_step
?
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.
append_step
?
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 decided against adding the functionality to stream
since it can get quite complex due to the kwargs value
accepting ChatMessage
types and also message
.
Instead, I compressed the two functions into append_step
, where it accepts a steps
kwarg which can be new
, append
, or a ChatSteps
type.
Edit: see notebook for updated usage.
Closes #6598
The relationship:
ChatInterface > ChatFeed > Steps or Step
Steps > Step
Easily available to use by:
instance.step(**kwargs)
->instance.stream(ChatStep(**kwargs))
instance.steps(**kwargs)
->instance.stream(ChatSteps(**kwargs))
steps.step(**kwargs)
->steps.append(step)
ChatStep
(s) methods mimicChatFeed
methods:step.stream()
step.stream_title()
These context managers are optional and the equivalent of:
Screen.Recording.2024-04-01.at.6.00.35.PM.mov