-
Notifications
You must be signed in to change notification settings - Fork 324
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
token usage tracking (Issue #268) #339
base: main
Are you sure you want to change the base?
Conversation
|
whoa!! thanks @andrewgcodes! i'll take a more detailed look at this in a bit, but thank you so much for taking the time to do this! |
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.
Looking more, this is a phenomenal PR, and I sincerely appreciate you adding test cases.
However, we're trying super hard to keep the Stagehand interface as minimal as possible to avoid adding unnecessary complexity to the end-user. Having a user write out the code I highlighted feels unnecessarily cumbersome.
I acknowledge the frustration around token usage being a black box, and it's something we're actively fixing before the holiday break ends. We as the Browserbase team need to do better about this.
I'm imagining an interface to act/extract/observe with something like the following:
const result = page.extract(extractOptions) # returns { ...extractResult, _stagehandTokenUsage: TokenUsageObject }
Let's talk more in #stagehand-discussion in the Stagehand public Slack (I see you're part of this already but linking here for posterity)
examples/hackernews.ts
Outdated
const extractUsage = stagehand.getUsage(); | ||
const extractEntry = extractUsage.find( | ||
(entry) => entry.functionName === "extract", | ||
); | ||
if (extractEntry) { | ||
logTokenUsage("HN-ARTICLE-EXTRACT", extractEntry); | ||
} |
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.
this isn't really elegant; ideally the user should be able to have all of this in one line
…ehandTokenUsage property Co-Authored-By: Andrew Gao <[email protected]>
…se _stagehandTokenUsage property" This reverts commit a79b420.
…esponse objects via _stagehandTokenUsage property. new test cases and example
Hey Anirudh, thanks for the quick review and kind words! I'll ping you on
the Slack
|
@@ -53,6 +53,7 @@ export class LLMProvider { | |||
getClient( | |||
modelName: AvailableModel, | |||
clientOptions?: ClientOptions, | |||
stagehand?: any, |
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.
Would definitely try to avoid using any
here
this is amazing! looking for the discussion in Slack. Also I see some Devin in here @andrewgcodes :D |
why
being able to track input/output/total tokens when using stagehand can be helpful for controlling costs, picking which model to use, etc.
See Issue #268 (#268)
what changed
Works for act(), observe(), and extract()
12 files edited, added tests and and example (hackernews.ts) demonstrating how to log tokens used. Please see diff
test plan
i wrote a few test cases and it seems to work but someone on the browserbase team should definitely test as well before merging!