-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Add to_display to BaseMessage #5523
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5523 +/- ##
=======================================
Coverage 78.74% 78.74%
=======================================
Files 167 167
Lines 10025 10030 +5
=======================================
+ Hits 7894 7898 +4
- Misses 2131 2132 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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.
Generally speaking, it is better for the consumer of the messages to decide what to display. E.g., you can create a filter or lambda condition from the consumer side to selectively display a subset of messages. The producer of the messages may not know who is going to consume it. For example, a producer that produces the messages but the messages were writen to disk, and read again through a separate system. The to_display
flag is just going to be ignored and won't be useful.
Rather than the indicator for display or not display, I think it's better to introduce attributes that indicate what the message is, timestamp, is it an inner thought message, etc., and the consumer can decide based on these attribtues.
the consumer should know what the message is via its type and content. I think is_inner_message field would be confusing because other messages have inner_messages. Moreover, I'm not sure what constitutes an "inner_message", because the use case I have these really are not inner messages but just not meant for consumption by a UI. If you have other names than to_display that would make that clear, that's fine. I'm not sure I follow consumer-producer logic, I can see arguments for both sides making sense. The agent/interface distinction is blurry for most applications/implementations. |
I still think a metadata field is still the right way to do this vs adding many other fields. Elegance is one thing, but being limited at the message layer is a blocking factor. |
For comparison, this is Langchain messages https://python.langchain.com/docs/concepts/messages/ |
@ekzhu what about is_inner_team_message instead of to_display ? Let me know what we need to add a boolean field to basemessage. |
I agree with Eric that this field imposes something that clients may not choose to follow, which is confusing. IMO, if the application needs to filter based on some extra fields then I am open to an unstructured metadata property bag |
Ah! metadata bag! I think this is at least better than to_display. Still, can we discuss the problem a bit more, e.g., what is preventing using other types of filters? I just want us to be more aware of the reason that drives the decision. |
what can go wrong with adding metadata to messages? model_usage is arguable metadata already. |
Add to_display field to BaseMessage to allow control over which messages to show in Console or other applications.
Modified Console to rely on to_display and filter messages. This can be removed
resolves part of #5385
studio could also rely on this, but up to @victordibia