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.

Sign up for GitHub

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
+18 −2
Open

improved ToString implementations overridden in a previous PR #44

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35d86a5
improved ToString implementations overriden in a previous PR
KrzysztofCwalina Jun 11, 2024
9f620b3
Update src/Custom/Chat/ChatCompletion.cs
KrzysztofCwalina Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/Custom/Chat/ChatCompletion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Text;

namespace OpenAI.Chat;

Expand Down Expand Up @@ -65,5 +66,19 @@ public partial class ChatCompletion
/// Returns text representation of the first part of the first choice.
/// </summary>
/// <returns></returns>
public override string ToString() => Content[0].Text;
public override string ToString()
{
IReadOnlyList<ChatMessageContentPart> content = Content;
if (content.Count == 1)
{
return content[0].ToString();
}

StringBuilder sb = new();
foreach (var part in content)
{
sb.AppendLine(part.ToString());
}
return sb.ToString();
}
KrzysztofCwalina marked this conversation as resolved.
Show resolved Hide resolved
}
8 changes: 7 additions & 1 deletion src/Custom/Chat/ChatMessageContentPart.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,13 @@ public static ChatMessageContentPart CreateImageMessageContentPart(BinaryData im
/// Returns text representation of this part.
/// </summary>
/// <returns></returns>
public override string ToString() => Text;
public override string ToString()
{
if (Kind == ChatMessageContentPartKind.Text) {
return String.IsNullOrWhiteSpace(Text) ? "<empty>" : Text;
Copy link
Collaborator

@joseharriaga joseharriaga Jun 12, 2024 •
edited
Loading

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.

}
return $"<{Kind.ToString().ToLowerInvariant()}>";
}

/// <summary>
/// Implicitly creates a new <see cref="ChatMessageContentPart"/> instance from an item of plain text.
Expand Down
Loading