Skip to content
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

improved ToString implementations overridden in a previous PR #44

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KrzysztofCwalina
Copy link
Collaborator

No description provided.

@KrzysztofCwalina KrzysztofCwalina marked this pull request as ready for review June 11, 2024 22:30
public override string ToString()
{
if (Kind == ChatMessageContentPartKind.Text) {
return String.IsNullOrWhiteSpace(Text) ? "<empty>" : Text;
Copy link
Collaborator

@joseharriaga joseharriaga Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if modifying the output to change an empty string to "<empty>" could be a problem. For example, imagine using chat completions with function calling and a user writing a function that can return an empty string as a valid result. If the user is not aware of this, I think they could unknowingly start sending "" in their requests, which would be incorrect. It's a little bit of an edge case, but it makes me think that we should consider a different solution. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we prefer to simply return the empty string as-is, I am ok with that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of something like the following?

What if we introduce a new class called ChatMessageContent that looks like this:

public class ChatMessageContent
{
    public IReadOnlyList<ChatMessageContentPart> Parts { get; }
    
    // It returns a concatenation of the Text property of all the Parts. 
    // We could also call it "FullText" to be clearer. 
    // It could also be a method called "GetAllText()", etc. 
    public string Text => ...
}

Then, we change the ChatCompletion class so that its Content property is now of type ChatMessageContent instead of IReadOnlyList<ChatMessageContentPart>.

Developers can then use it like this:

ChatCompletion chatCompletion = client.CompleteChat(new UserChatMessage("Say hello"));
Console.WriteLine($"[ASSISTANT]: {chatCompletion.Content.Text}");

What I like about an approach like this one:

  1. It can return empty or null. This is not a problem because it's not bound by the ToString guidelines.
  2. Its behavior feels more consistent: If there are zero parts with Kind == Text, it will intuitively return null instead of returning something else just because it has to return something according to the ToString guidelines.
  3. It feels a little more future-proof to me. For example, if in the future we realize we need something with different behavior, we can EBN this one and simply introduce a new property with the new behavior. On the other hand, we cannot really EBN ToString and ToString feels like it would also be "stickier" (that is: I think it would be harder to teach people to stop using it after we originally told them to use it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind introducing a type like you proposed. I even like it. It has many benefits. Having said that, I think Text property returning null is not great. I think this type would have to override ToString (and have roughly the same implementation as this one). In .NET, ToString returns "best effort" textual representation. Yes, it cannot return null, but that's as far as the promise of what it does goes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I think it's important to be able to return both empty and null is because they can both have special meanings. For example, when the model wants to call a function, it returns a message that can have null content. Null content here indicates that the model is not willing to respond until it receives the value returned by the functions it wants to call.

If a user were writing UI, they wouldn't want to display an empty speech bubble for this message that has null content. How can they know they should skip this message? Should they check message.Content.Count > 0 first and only call ToString() if it succeeds? Personally I think checking for message.Content.Text != null and then printing message.Content.Text sounds more intuitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants