-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
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 72.30% 81.67% +9.37%
==========================================
Files 322 326 +4
Lines 47663 47944 +281
==========================================
+ Hits 34463 39160 +4697
+ Misses 13200 8784 -4416 ☔ 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 |
Sorry for the delay in reviewing this. The functionality here looks great but now that I'm actually playing around with this I'm finding it pretty confusing. It's not helped by the fact that the examples in the original comment haven't been updated so I'm guessing at the proper way to use this. I'll have to spend some more time with it to form a better opinion on what the API should be but I'm fairly convinced that this isn't mergeable in the current state. |
I think the main thing I'm struggling with is that I don't quite understand the point of the |
Okay, I've made some changes here:
I'm quite happy with this approach, but may have missed something, e.g. maybe you should be able to append to an existing set of steps even if it's not the last message. If that's something we need to support we can revisit. For now I'll merge to unblock some of the other work going on. |
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