Skip to content

feat: Add Raw Response Metadata Handling - GPT Token Usage Example #270

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

DZunke
Copy link
Contributor

@DZunke DZunke commented Apr 8, 2025

Currently it is not really possible to get into details of the interaction between user and model because the HTTPResponse is hidden behind the response converter and is then forgotten forever. So the only possibility currently to get details from the interaction would be a custom response and a custom response converter.

As a draft and to collect your ideas and other input i want to throw this stone into the lake with having the HTTPResponse also transported into the direction of the output processors. Could this give a bit more flexibility? What do you think?

Please have in mind this is only a draft to discuss. I thought about having this also possible if a request fails with an error status code in the future so the output processors could handle in a separation of convern way if there should be an exception thrown. Or do we maybe need an additional layer on top of the response converters itself?

For sure this is the simplest use case with the text response in mind. Maybe for the streamed response it would need a more "recording" approach with an output processor. But would surely be possible.

@OskarStark OskarStark marked this pull request as draft April 8, 2025 11:55
@OskarStark
Copy link
Contributor

Looks great, I would only vote vor having a Metadata object instead of an array

@DZunke
Copy link
Contributor Author

DZunke commented Apr 8, 2025

Looks great, I would only vote vor having a Metadata object instead of an array

Thanks ... 👍🏼 ... i thought about extending the collection, if it is generally fine, with a key value (as object) storage and search by name, etc. so a bit of comfort in the metadata then. Just was unsure if this kind of "transport" is in the interest of the library or if someone has other restructurings in mind that would make it "easier" or more complicated.

@chr-hertel
Copy link
Member

chr-hertel commented Apr 9, 2025

Yeees, we need that - thanks for kicking it off! 😍
I also like the two levels here with a more structured metadata object and the raw response.

And I would even prefer to have them lower. Basically in the ResponseInterface directly:

namespace PhpLlm\LlmChain\Model\Response;

use PhpLlm\LlmChain\Model\Response\Metadata;
use Symfony\Contracts\HttpClient\ResponseInterface as SymfonyHttpResponse;

interface ResponseInterface
{
    /**
     * @return string|iterable<mixed>|object|null
     */
    public function getContent(): string|iterable|object|null;

    /**
     * Returns a key-value set based on provider specific response meta data.
     */
    public function getMetadata(): Metadata;

    /**
     * Return the original API response.
     */
    public function getRawResponse(): SymfonyHttpResponse;
}

I know that this would shift the responsibility from the OutputProcessor to every ResponseConverter, but maybe we can shift that into more clever abstractions/helper/trait/younameit if there are redundancies.

@4lxndr @tryvin that would also help with your use-cases, right?

@tryvin
Copy link

tryvin commented Apr 9, 2025

@chr-hertel Yes, it does! Thanks for letting me know!

I've been so busy dealing with external providers that I didn't have the time to do what I promised in the issue.

But I will keep an eye on this PR

@DZunke DZunke closed this Apr 10, 2025
@DZunke DZunke reopened this Apr 10, 2025
@chr-hertel
Copy link
Member

While working on #280 and with that fake PipelineResponse I was wondering if we should rather use terminology like Input/Output on this layer already/as well - instead of Request/Response - what do you think?

@DZunke
Copy link
Contributor Author

DZunke commented Apr 16, 2025

Would this not very confusing? Then there would be two different "Output" named layers within the data flow.

@chr-hertel
Copy link
Member

Yeah true, having that terminology twice could be annoying 🤔

@OskarStark
Copy link
Contributor

ModelResponse? ModelOutput? To be more specific regarding the layer? Just thinking out loud

@DZunke DZunke force-pushed the token-recording branch 2 times, most recently from cce5ae2 to 5b7ef9a Compare April 21, 2025 12:45
@chr-hertel
Copy link
Member

Nevermind the pipeline if it's only the commit thingy - need to dig deeper why that is so flaky

@chr-hertel
Copy link
Member

from an OOP point of view - why choose traits over an abstract base implementation of the response? isn't it that all response classes have the two traits?

with having the interface people could still decide to do their own - or we could do both - having a base class that uses both traits. not sure if i miss the downside here

@DZunke
Copy link
Contributor Author

DZunke commented Apr 24, 2025

from an OOP point of view - why choose traits over an abstract base implementation of the response? isn't it that all response classes have the two traits?

with having the interface people could still decide to do their own - or we could do both - having a base class that uses both traits. not sure if i miss the downside here

Mhm. Yeah. First i wanted to have a most flexible approach but after revisiting it i have implemented an abstract base response to extend from if needed. But i also kept the traits as a handy helpers, if this is fine? See AsyncResponse where i have not extended from the BaseResponse but utilized the MetadataAwareTrait as it has it's own raw symfony response usage.

What do you think?

@DZunke DZunke changed the title Draft: A first throw of metadata parsing from LLM interaction feat: Add Raw Response Metadata Handling - GPT Token Usage Example Apr 24, 2025
@DZunke DZunke marked this pull request as ready for review April 24, 2025 07:14
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.

Thanks a ton for taking care about this @DZunke - that feature was overdue!

@chr-hertel chr-hertel merged commit 5a06b3f into php-llm:main Apr 25, 2025
7 checks passed
@OskarStark
Copy link
Contributor

Nice one 👏

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

Successfully merging this pull request may close these issues.

4 participants