Skip to content

[AI Bundle] Activate token usage output processors #329

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

Jump to bottom
Merged
merged 1 commit into from
Sep 1, 2025
Merged

[AI Bundle] Activate token usage output processors #329

merged 1 commit into from
Sep 1, 2025

Conversation

junaidbinfarooq
Copy link
Contributor

@junaidbinfarooq junaidbinfarooq commented Aug 19, 2025 •
edited
Loading

Q A
Bug fix? no
New feature? yes
Docs? no
Issues Fix #208 Continues #311
License MIT

Changes proposed:

  • Adds support to automatically configure the token usage processors so that they are available to end users (devs) when needed
  • Makes the availability of token usage configurable and enabled by default

@carsonbot carsonbot added AI Bundle Issues & PRs about the AI integration bundle Feature New feature Status: Needs Review labels Aug 19, 2025
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch 2 times, most recently from 0229745 to 971b174 Compare August 19, 2025 12:58
OskarStark
OskarStark reviewed Aug 19, 2025
View reviewed changes
OskarStark
OskarStark reviewed Aug 19, 2025
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Off topic, but should also be then integrated in the profiler 🙌🤓

@junaidbinfarooq
Copy link
Contributor Author

Off topic, but should also be then integrated in the profiler 🙌🤓

Can that be done separately?

@OskarStark
Copy link
Contributor

Yes should be a follow up PR, maybe create an issue to not forget?

OskarStark
OskarStark reviewed Aug 19, 2025
View reviewed changes
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 5a1a420 to 157149b Compare August 19, 2025 15:37
OskarStark
OskarStark reviewed Aug 19, 2025
View reviewed changes
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 157149b to 86d0404 Compare August 19, 2025 17:18
@OskarStark OskarStark changed the title [AI Bundle] Activate token usage output processors in ai bundle [AI Bundle] Activate token usage output processors Aug 20, 2025
@junaidbinfarooq
Copy link
Contributor Author

@chr-hertel
Could you have a look?

OskarStark
OskarStark reviewed Aug 20, 2025
View reviewed changes
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 86d0404 to 69448b0 Compare August 20, 2025 18:14
chr-hertel
chr-hertel reviewed Aug 20, 2025
View reviewed changes
chr-hertel
chr-hertel reviewed Aug 20, 2025
View reviewed changes
chr-hertel
chr-hertel reviewed Aug 20, 2025
View reviewed changes
chr-hertel
chr-hertel requested changes Aug 20, 2025
View reviewed changes
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.

Please remove the indirection via the attribute here. there is no benefit, but only adds complexity to maintain.

We know in the bridge exactly for which platform the output processor is intended, could just register them as service (where does that currently happen?) and check for their existence while building the agent, like you did in AiBundle class.

And please enable and test it with the demo - does it already work for you?

VincentLanglet
VincentLanglet reviewed Aug 25, 2025
View reviewed changes
@junaidbinfarooq
Copy link
Contributor Author

Do you mean we should register such classes as services individually manually?

Yes please - we don't need to decouple via attribute. That also makes it more brittle. More moving parts, more places to break :D let's keep it simple and register the services directly when the option is true programmatic in the AiBundle class

I tried testing the demo app with Ollama. Though everything worked fine, the token usage was obviously empty since the said bridge doesn't have any output processors for token usage yet, which I intended to add as well.

didn't work for me with openai, but not sure i got it right. let's see after the change 👍

@chr-hertel I made the following changes in the latest commit:

  • Removed the attribute to tag token usage output processors
  • Registered the token usage output processors as services manually inside the AI bundle
  • Added token usage information to the documentation

Would it make sense to follow this PR up with another one to add the token usage stuff for demo and probably profiler as well, to keep this PR small?

@chr-hertel
Could you have a look at the latest changes and let me know what you think?

@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 5b03137 to ec76240 Compare August 28, 2025 09:50
VincentLanglet
VincentLanglet reviewed Aug 28, 2025
View reviewed changes
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch 2 times, most recently from d5b4e6b to e66c7a8 Compare August 29, 2025 12:43
chr-hertel
chr-hertel requested changes Aug 31, 2025
View reviewed changes
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.

Please reduce the needed configuration to

track_token_usage: false

See #329 (comment)

And then we're good to merge after me testing it out :) 👍

@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from e66c7a8 to 688ee24 Compare September 1, 2025 09:13
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 688ee24 to 021b133 Compare September 1, 2025 09:26
VincentLanglet
VincentLanglet approved these changes Sep 1, 2025
View reviewed changes
@junaidbinfarooq junaidbinfarooq force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 2f3147b to 44d7b2f Compare September 1, 2025 17:06
OskarStark
OskarStark reviewed Sep 1, 2025
View reviewed changes
@OskarStark
Copy link
Contributor

Looks quite good to me

chr-hertel
chr-hertel reviewed Sep 1, 2025
View reviewed changes
chr-hertel
chr-hertel reviewed Sep 1, 2025
View reviewed changes
@junaidbinfarooq @chr-hertel
feat(ai-bundle): Auto-configure token usage processors
b426948 
- Adds support to automatically configure the token usage processors so that they are available to end users (devs) when needed
- Makes the availability of token usage configurable and disabled by default

feat(ai-bundle): Auto-configure token usage processors
- Adds an example script inside mistral dir to verify the token usage

refactor(ai-bundle): Auto-configure token usage processors
- Removes the attribute to tag token usage output processors
- Registers the token usage output processors as services manually inside ai bundle
- Adds token usage information to the documentation
@chr-hertel chr-hertel force-pushed the feat/activate-token-usage-output-processors-in-ai-bundle branch from 44d7b2f to b426948 Compare September 1, 2025 20:45
@chr-hertel
Copy link
Member

Thank you @junaidbinfarooq.

@chr-hertel chr-hertel merged commit 6fb5e13 into symfony:main Sep 1, 2025
8 checks passed
@chr-hertel
Copy link
Member

Fixed some namespaces and docs on merging + vertexai support

@junaidbinfarooq junaidbinfarooq deleted the feat/activate-token-usage-output-processors-in-ai-bundle branch September 2, 2025 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OskarStark OskarStark OskarStark approved these changes

@chr-hertel chr-hertel chr-hertel requested changes

@Nyholm Nyholm Awaiting requested review from Nyholm Nyholm is a code owner

+1 more reviewer

@VincentLanglet VincentLanglet VincentLanglet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AI Bundle Issues & PRs about the AI integration bundle Feature New feature Status: Needs Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bundle] Activate TokenUsage output processors in the bundle

5 participants

@junaidbinfarooq @OskarStark @VincentLanglet @chr-hertel @carsonbot