-
Notifications
You must be signed in to change notification settings - Fork 114
Updates to make the library applicable for wider range of use cases, and more stable. #644
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
base: main
Are you sure you want to change the base?
Conversation
|
/gemini review |
|
@csells , how does it look? |
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.
Code Review
This pull request introduces several significant and beneficial changes to make the library more flexible and robust. Key changes include removing the enforced ChatMessageRole, renaming ChatMessage to Message, and using constants for JSON keys to reduce message size. The test coverage has been significantly improved, including a new test for the example, which is excellent. My review focuses on some opportunities to improve code clarity and leverage more modern Dart features, such as using const constructors for immutable classes and refactoring switch statements/expressions for better readability. Overall, this is a solid pull request that improves the library's design and usability.
|
Options for name for the class ChatMessage:
|
|
I think you left one option:
Can I ask what problem is solved by renaming it? |
Sure! This library is intended to serve very wide range of client applications, interacting with AI. 'Chat' is implying type of interaction, where history is preserved, and UI looks as timeline. But, there can be many other types. And, I am sure we do not know these types as they do not exist yet. Does it help? |
|
That sounds abstract. My understanding of the goal of this package was to concretely provide base types for chat history, which would eliminate the need to provide mapping layers between GenUI, dartantic and other packages like the Flutter AI Toolkit. What concrete scenarios do you have in mind? |
For example, interaction with some history-less agent without chat-like UI. |
|
To which agent are you referring that provides such a history-based API without it being conversational? I'm unfamiliar with any that work like that. |
|
|
||
| /// A message in a conversation between a user and a model. | ||
| @immutable | ||
| class Message { |
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.
Is this class intended to be implemented? If not, let's mark it using the base modifier.
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.
Made it final to avoid issues with backward compatibility. If we ever change our mind, it will be easy to remove it or replace final with base.
| /// Creates a new message, taking a text string as separate parameter. | ||
| Message( | ||
| String text, { | ||
| List<Part> parts = const [], |
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 if one of the parts is a TextPart? Is it OK to have two text parts?
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.
yes, added documentation
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.
The documentation is on a different constructor. This constructor should explain what happens if one passes both text, and one or more TextParts.
| @immutable | ||
| abstract class Part { | ||
| /// Creates a new part. | ||
| const Part(); |
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.
Why do we need a constructor for creating an empty part?
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.
Because we need parent constructor for all descendants, but implicit constructor is not constant. It is good to redefine it to make constant.
| final Map<String, dynamic> metadata; | ||
|
|
||
| /// Gets the text content of the message by concatenating all text parts. | ||
| String get text => parts.whereType<TextPart>().map((p) => p.text).join(); |
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 I have parts "happy", "new", "year", "world", will the concatenated text be "happynewyearworld"?
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.
yes, but luckily the text parts include whitespace, so we're good.
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.
Yes, these text parts are created to support streaming from LLM. And, LLM will produce the parts with spaces and paragraph marks:
"Happy", " new", " year,", " world!\n"
(yes, happy New Year!) I said above not 'history-based' but 'history-less', but it is not very important. As I said before, we want to cover wide range of stories, published and not published, past and future. It does not matter if such agent exist or no. The landscape of AI-related tools and patterns and practices changes every week. If a story is theoretically possible, and it may benefit from this library, I do not want naming to create confusion. And yes, as you noticed, it sounds abstract, and it should be abstract, to allow as many concrete implementations, as possible. A good side effect of this renaming is that now the name 'ChatMessage' is used in example, and it is good that this name is available for client applications to incapsulate whatever envelope information is needed. Did I miss something? |
Yes, I am cutting off some opinions of this library that have big chances to be different in different use cases. In particular I am separating message body and message envelope: the body should stay in the library (because body has the same structure in most cases), while envelope should be defined in the client application (because content of envelope has big chances to differ from case to case, and thus there is no point to have it in library). Does it help? |
Wraps the example data parsing and component creation in a try-catch block to capture and rethrow errors with additional context (the item name). This makes it easier to identify which catalog item has invalid example data.
| /// | ||
| /// If `parts` or `metadata` is not provided, an empty collections are used. | ||
| /// | ||
| /// If there is no parts of type [TextPart], the [text] property |
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.
grammar: "are no parts"
| /// If there is no parts of type [TextPart], the [text] property | ||
| /// will be empty. | ||
| /// | ||
| /// If there are many parts of type [TextPart], the [text] property |
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.
"many" sounds ambiguous. Let's say "more than one".
| /// | ||
| /// If there are many parts of type [TextPart], the [text] property | ||
| /// will be a concatenation of all of them. | ||
| /// Many text parts is convenient to have to support |
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.
Separate paragraphs with one empty line.
grammar: "convenient to have to support" doesn't sound right. Suggestion: "Use multiple text parts to support..."
| /// If there are many parts of type [TextPart], the [text] property | ||
| /// will be a concatenation of all of them. | ||
| /// Many text parts is convenient to have to support | ||
| /// streaming of the message. |
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 does "streaming of the message" mean? Standard Dart Streams are asynchronous constructs, but parts are provided as one synchronous List.
| /// streaming of the message. | ||
| const Message({this.parts = const [], this.metadata = const {}}); | ||
|
|
||
| /// Creates text message. |
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.
grammar: "Creates a text message"
| break; | ||
| @override | ||
| Part convert( | ||
| Map<String, Object?> input, { |
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.
nit: let's give input a proper name (all arguments are "inputs" 😄 ). Perhaps partJson?
| /// Base class for message content parts. | ||
| @immutable | ||
| abstract class Part { | ||
| abstract base class Part { |
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.
With a little more understanding of how this works, I think this needs substantially more documentation. In particular, if we want to support custom parts, we should document how to properly create them. The docs should explain about operator==, hashCode, and the JsonToPartConverter. The developer would want to know that all these things need to be covered in order to build a robust custom part implementation.
|
|
||
| /// The default MIME type for binary data. | ||
| static const defaultMimeType = 'application/octet-stream'; | ||
| /// Converts the part to a JSON-compatible map. |
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.
The map needs more than JSON compatibility. The returned value must be a JSON object, containing certain keys, such as "type". Let's document that.
| /// A text part of a message. | ||
| @immutable | ||
| class TextPart extends Part { | ||
| base class TextPart extends Part { |
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.
It might be safer to make this final. Inheritance complicates equality, hash codes, and serialization. For example, it would be too easy to attempt to call super.toJson() from a subclass, and forget to overwrite the part "type". So a HeadingTextPart may accidentally end up getting deserialized as a plain TextPart.
There's not much code to reuse anyway. The developer could just copy this code and make the necessary changes.
This comment applies to other concrete Part classes below.
| description: | ||
| name: characters | ||
| sha256: f71061c654a3380576a52b451dd5532377954cf9dbd272a78fc8479606670803 | ||
| sha256: faf38497bda5ead2a8c7615f4f7939df04333478bf32e4173fcb06d428b5716b |
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.
Did you need to update pubspec.lock to make your code changes to work? Or was this by accident?
|
My 2c on the |
Contributes to #607
dynamicwithObject?Partto make the class extendable.collectionfor deep collection comparison