-
-
Notifications
You must be signed in to change notification settings - Fork 148
[AI Bundle][Platform] Allow configure Bedrock platform via bundle config #1206
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?
Conversation
|
It seems like to run tests successfully for ai-bundle link call would be needed (at least it helped for me locally). Is it possible or should i search for another approach? could someone help me out with those ? PS big thanks for quick merge of previous issue, it was my first one contribution, so pardon me for silly questions)) |
0fb4ea9 to
6b5ad59
Compare
chr-hertel
left a comment
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 @uerka for picking up here 👍
src/ai-bundle/src/AiBundle.php
Outdated
| if (!ContainerBuilder::willBeAvailable('symfony/ai-bedrock-platform', BedrockFactory::class, ['symfony/ai-bundle'])) { | ||
| throw new RuntimeException('Bedrock platform configuration requires "symfony/ai-bedrock-platform" package. Try running "composer require symfony/ai-bedrock-platform".'); | ||
| } |
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.
needs a check for [] === $type['bedrock'] or to be moved into foreach
see #1214 for similar
…hrough response conversion)
8ff4030 to
fd18c26
Compare
|
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
docs/bundles/ai-bundle.rst
Outdated
| model: 'text-to-speech' | ||
| tools: false | ||
| nova: | ||
| platform: 'ai.platform.bedrock_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.
| platform: 'ai.platform.bedrock_default | |
| platform: 'ai.platform.bedrock_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.
guess it was about missing quote mark, fixed)
|
I noticed your tests are failing cause of similar reasons as in my PR: #1139 Any idea how to fix these type of issues? Do we need to add new method parameters as last to avoid breaking BC? |
| { | ||
| public static function create( | ||
| BedrockRuntimeClient $bedrockRuntimeClient = new BedrockRuntimeClient(), | ||
| ?BedrockRuntimeClient $bedrockRuntimeClient = 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.
is this change needed?
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.
tbh - not sure about this one.
When its nullable we can pass null as first argument in this line that feels like a better idea than creating BedrockRuntimeClient in the config.
i moved creating client later in code, so i dont really see the problem of having it nullable from now - except of failing tests atm.
i dont mind to have it as required argument and always require it for configuring bedrock platform, but seems like creating client on the fly should work also fine (as soon as creds can be resolved through env vars).
would appreciate your input on 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.
understood - let's keep it like that, but go with explicit null check below:
if (null === $bedrockRuntimeClient) {| throw new RuntimeException('Bedrock platform configuration requires "symfony/ai-bedrock-platform" package. Try running "composer require symfony/ai-bedrock-platform".'); | ||
| } | ||
|
|
||
| $platformId = 'ai.platform.bedrock_'.$name; |
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.
@chr-hertel should we go with
| $platformId = 'ai.platform.bedrock_'.$name; | |
| $platformId = 'ai.platform.bedrock.'.$name; |
?
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, would be more consistent - see azure
| return new RawBedrockResult($this->bedrockRuntimeClient->invokeModel(new InvokeModelRequest($request))); | ||
| } | ||
|
|
||
| public function convert(InvokeModelResponse $bedrockResponse): ToolCallResult|TextResult |
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.
why removing this?
| ->method('invokeModel') | ||
| ->with($this->callback(function ($arg) { | ||
| $this->assertInstanceOf(InvokeModelRequest::class, $arg); | ||
| $this->assertEquals('application/json', $arg->getContentType()); |
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.
assertSame where possible please
| 'claude-opus-4-5-20251101' => [ | ||
| 'class' => Claude::class, | ||
| 'capabilities' => [ | ||
| Capability::INPUT_MESSAGES, | ||
| Capability::INPUT_IMAGE, | ||
| Capability::OUTPUT_TEXT, | ||
| Capability::OUTPUT_STREAMING, | ||
| Capability::TOOL_CALLING, | ||
| ], | ||
| ], | ||
| 'claude-sonnet-4-5-20250929' => [ | ||
| 'class' => Claude::class, | ||
| 'capabilities' => [ | ||
| Capability::INPUT_MESSAGES, | ||
| Capability::INPUT_IMAGE, | ||
| Capability::OUTPUT_TEXT, | ||
| Capability::OUTPUT_STREAMING, | ||
| Capability::TOOL_CALLING, | ||
| ], | ||
| ], |
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.
Please revert, I extracted it to:
Currently it seems not possible to define bedrock as a platform.
Added configuration for this platform with support of multiple instances (for example per AWS region). Added tests for model clients. Changed Bedrock's PlatformFactory to have nullable BedrockRuntimeClient
So now the AWS bedrock can be configured together with other platforms as follows:
Also fixed some smaller bugs for Nova model (exception of sending model name).