-
-
Notifications
You must be signed in to change notification settings - Fork 29
[AIBundle] Wire & configure services explicitly #163
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
Conversation
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 for working on this! Will need to give it another read + test, but some things already caught my eye.
570a0a3
to
bda9a1f
Compare
cd4c881
to
1c19fa1
Compare
Converted from name-based to index-based args to address #77 too Included in the same PR not to need 2 rounds of testing |
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 so much for this! Tested it with the demo - and that looks good 👍
src/ai-bundle/src/AIBundle.php
Outdated
->setArguments([ | ||
$platform['api_key'], | ||
new Reference('http_client', ContainerInterface::NULL_ON_INVALID_REFERENCE), | ||
new Reference('ai.platform.contract.default'), |
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.
That's causing the error with the audio bot - it needs the Whisper contract extension, that is not part of the default, see here:
$contract ?? Contract::create(new AudioNormalizer()), |
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.
Feels a bit counter-intuitive: I thought OpenAI was meant to be the default contract and now it seems we need a custom contract for it 🤔
Anyway, done – the audio demo works now
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.
The Contract is for the entire Platform - including the model Whisper, but the default is currently only for embeddings and LLM completion.
Whisper doesn't qualify to be in the default Platform contract for me, but still is part of the OpenAI Platform.
Thank you @valtzu. |
Missed a rebase here, main is broken now. 🫣 Fix in #176 |
This PR was merged into the main branch. Discussion ---------- [AIBundle] Fix dependency injection | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Docs? | no | License | MIT Fix DI config after #163 / #154. Should've rebased before merge – now we're getting a bunch of errors on `main`: ``` Symfony\AI\Agent\Toolbox\Toolbox::__construct(): Argument #1 ($tools) must be of type Traversable|array, Symfony\AI\Agent\Toolbox\ToolFactory\ReflectionToolFactory given ``` etc Commits ------- 3c6290e [AIBundle] Fix dependency injection config after #163
Don't autowire or autoconfigure anything in AI Bundle