-
-
Notifications
You must be signed in to change notification settings - Fork 54
[Platform] Standardise token usage #311
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?
[Platform] Standardise token usage #311
Conversation
examples/openai/token-metadata.php
Outdated
$result = $agent->call($messages, [ | ||
'max_tokens' => 500, // specific options just for this call | ||
]); | ||
|
||
$metadata = $result->getMetadata(); | ||
if (null === $tokenUsage = $result->getTokenUsage()) { |
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.
Can this happen?
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, if you mean null === $tokenUsage
, when token_usage is marked false in the config, as far as I can see.
Edit: Here it is unnecessary.
final class TokenUsage implements \JsonSerializable | ||
{ | ||
public function __construct( | ||
public ?int $promptTokens = null, |
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.
public ?int $promptTokens = null, | |
public ?int $prompt = null, |
Maybe remove all the Token suffix here? They feel superfluous
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.
No problem.
I would not have kept the suffix if the object were simply named something like Tokens
, but I thought TokenUsage
is a better name. Or perhaps the DTO should be named as Tokens
.
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.
To be discussed with @chr-hertel
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.
I did remove the suffixes BTW.
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.
Thanks, not sure now that this was the right decision 😅
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.
TokenUsage
for the class name and tokens as the suffixes for the property names sounds more logical to me.
Or conversely, Tokens
for the class name and no suffixes for the property names would also be a good option.
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.
TokenUsage and tokens as suffix sound best to me
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.
🤦♂️ 😄
7a34c26
to
d771023
Compare
75e98c8
to
048ac63
Compare
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.
Sorry, but we need to slim this down a bit - in general smaller PRs, that build on top of each other are favorable to me. so let's break it down like you did:
Makes token usage explicit
I like having the value object, we should definitely have that 👍
But I'm not in favor of having thegetTokenUsage()
directly on the result since the result is a high lever abstraction and token usage is quite specific to remote inference of GenAI models - not all models have that. let's keep it in the metadata please.
Adds support to automatically register token usage extractors so that the token usage information is available in the result when a model is invoked
That's a bit misleading since token usage was already sitting on an extension point =>OutputProcessor
and your design decision to use that extension point to add another extension point doesn't really resonate with me. why is the new extension point needed? it feels like we're just adding them because we can.
Makes the token usage auto-registration configurable
For me this is too early, i don't think that a use-case, do you? how often do we see the case for someone to bring their own token usage extractor.
all in all, let's start here please with the TokenUsage value object in the metadata class - that's an easy step we see the same picture 👍
edit: another PR could be to fix that issue #208 by registering the corresponding output processor based on the config setting
048ac63
to
1f1e6be
Compare
Hmm, I initially thought about keeping it in metadata, but having it inside the Result object also made sense since an API should send us this information. I, so far, have seen such information in the responses of whatever APIs I tried, but perhaps you are right, and other models out there do not provide such information.
Nope, the idea, instead, was to introduce an extension point specific to token usage extraction and exposure. This would also simplify for bridges to their own implentations of the extractor interface if needed.
So far, in this repository, I have found three such extractors (currently TokenOutputProcessor classes), including the new Google Vertex AI bridge, and all three of them perform the extraction differently, albeit not entirely. As you also rightly pointed out above, token usage is something specific to GenAI models, and not all of them have it. Not all of them will expose the data in a similar fashion, either. |
@OskarStark @chr-hertel |
@@ -70,8 +72,11 @@ public function testItAddsRemainingTokensToMetadata() | |||
$processor->processOutput($output); | |||
|
|||
$metadata = $output->result->getMetadata(); | |||
$tokenUsage = $metadata->get('token_usage'); |
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 about having a dedicated setTokenUsage/getTokenUsage on the metadata object?
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.
Not sure about this.
Metadata object already has a lot of methods, and each of them is generic in nature as far as I can see, and doesn't concern any specific model characteristic.
e33ba4a
to
069b4c3
Compare
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.
almost done i'd say - thanks already!
- Makes token usage explicit - Adds support to automatically register token usage extractors so that the token usage information is available in the result when a model is invoked - Makes the token usage auto-registration configurable
This reverts commit 1f1e6be.
- Makes token usage explicit by adding a dedicated dto for token information - Populates the token usage dto inside different token output processors and then adds it to metadata object
- Code refactor
069b4c3
to
21976bb
Compare
I am working on autoconfiguring token usage processors inside the AI bundle and based that branch on this one. |
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.
Good to merge from my end - @OskarStark?
Changes proposed:
The PR aims to standardize the token usage extraction and its availability in the AI bundle. Essentially, the following is done: