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

Message Splitting to allow multiple message implementations without stacking arguments #25

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

DZunke
Copy link
Contributor

@DZunke DZunke commented Sep 23, 2024

Hey,

i utilize the library to also generate image descriptions by a delivered picture. With the MessageBag this is currently not possible because only textual messages are supported but for image describing one needs to hand over the image to gpt. As with the nice improvements from the weekend for the tooling information i saw that the message stack up with more and more arguments.

So i thought about splitting up the messages into more speaking single objects with an purpose for each. What do you think about the idea to split it more up? As an example there is an image description generator script now.

This MR is a absolute basic. That is why i have kept the Message object as a bridge at first for the specific messages. But with testing, if the idea is fine, i would remove it and rewrite all the code that depends on it.

OskarStark added a commit to OskarStark/llm-chain that referenced this pull request Sep 23, 2024
A simpler implementation of
* php-llm#25

cc @DZunke
@OskarStark OskarStark mentioned this pull request Sep 23, 2024
@OskarStark
Copy link
Contributor

Another approach

@chr-hertel
Copy link
Member

Thanks for driving this - i think we need to tackle that visions support and i also like not growing the message that might become a messy DTO with unclear state, so i'm in with specific DTOs and a structure around it.

two things i'd like to focus on tho:

  1. enabling api calls like defined in the docs of GPT and Claude for vision support
  2. convinience for lib users a.k.a. keeping it simple from a DX point of view

my proposal to follow up on this would be:

  • keeping the Message as some kind of entry point into the DTO land of it
  • enabling multiple use cases for the user message

in the end, from my point of view, both calls should be possible:

$messages[] = Message::ofUser('Hi, my name is Chris');
// and
$messages[] = Message::ofUser('Please describe this image', Content::image($url));
// and also more explicit
$messages[] = Message::ofUser(Content::text('Please describe this image'), Content::image($url))

i know that would potentially lead up with a contract like this

public static function ofUser(string|Content ...$content): UserMessage

but this would be the trade off for having a nicer API

WDYT?

@OskarStark
Copy link
Contributor

public static function ofUser(string|Content ...$content): UserMessage

👍

@DZunke
Copy link
Contributor Author

DZunke commented Sep 23, 2024

public static function ofUser(string|Content ...$content): UserMessage

but this would be the trade off for having a nicer API

WDYT?

I am absolute fine with keeping the Message as an initial entry point. But would you be fine with the idea the message so being a facade for specific implementations of the messages to get rid of the growing Message DTO? That's what i understood.

From my PoV we could so combine DX and keeping cleaner DTOs with a specific purpose. Also with the MessageInterface there would be the direct enablement for more use cases from other models.

I already understood to have a look for the Claude implementation because the image reader looks a bit different in the payload there. Something to thing about when serializing the message 🤔 With my initial idea i would have the direct idea with having a Content Message in the GPT and another one in the Claude space. So the developer can decide based on the utilized model. But with the central entrypoint i see the problem to decide during rendering based on the language model how to render the array.

@chr-hertel
Copy link
Member

But would you be fine with the idea the message so being a facade for specific implementations of the messages to get rid of the growing Message DTO? That's what i understood.

Yes!

Something to thing about when serializing the message

so that's bringing us to the next item here in general - better to scope out tho for the sake of PR size:

ofc the interface enables users to define their own messages, yes. better DX would be limited to built-in ones via Message facade (can't believe i'm favoring this pattern here 😆). the json representation is model specific and therefore we might end up using the serializer for this later with the model and it's version as a serialization context - but step by step

@OskarStark
Copy link
Contributor

IMHO the problem (but I am a bit tired) is only because we are using jsonSerialize() inside the message now. If the message would just be a data holder, we could serialize (with Symfony serializer) the way we need it. OFC only if the message holds all necessary information, so Content::image(string $url, string $filetype......)

@chr-hertel
Copy link
Member

@OskarStark 💯 same idea - but let's go with a follow up PR to switch to serializer for that process

@chr-hertel
Copy link
Member

Nice one, pretty much like that thing now :) left only a few minor things, but mostly as suggestion to just merge.

something's still not right with the pipeline, sorry for that

@DZunke
Copy link
Contributor Author

DZunke commented Sep 25, 2024

something's still not right with the pipeline, sorry for that

No Problem! Thanks for pointing the unused classes out. I am sure PHP CS Fixer is able to remove them in some way automatically. But good to know to have an eye on it.

Thanks for all the good hints and the discussion about a nice interface for the messages, Christopher and Oskar 😊

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

Thank you @DZunke!

@chr-hertel chr-hertel merged commit 615ad93 into php-llm:main Sep 25, 2024
4 checks passed
@DZunke DZunke deleted the message-split branch September 25, 2024 16:08
@chr-hertel
Copy link
Member

one thing i didn't think about is the impact to using/handling/rendering the MessageBag in twig - potential follow up

for now i created php-llm/llm-chain-bundle#22

and patched my example project like this:
image

@chr-hertel chr-hertel mentioned this pull request Jan 5, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants