-
-
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?
Changes from all commits
7236b90
fad800f
265a789
51175f2
b4ca107
5a8a346
30ff8d0
9eff899
ebd3925
93bfa1e
94bc5a9
b774f6d
fd18c26
609fa1b
ca4d961
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,15 +13,10 @@ | |
|
|
||
| use AsyncAws\BedrockRuntime\BedrockRuntimeClient; | ||
| use AsyncAws\BedrockRuntime\Input\InvokeModelRequest; | ||
| use AsyncAws\BedrockRuntime\Result\InvokeModelResponse; | ||
| use Symfony\AI\Platform\Bridge\Anthropic\Claude; | ||
| use Symfony\AI\Platform\Bridge\Bedrock\RawBedrockResult; | ||
| use Symfony\AI\Platform\Exception\RuntimeException; | ||
| use Symfony\AI\Platform\Model; | ||
| use Symfony\AI\Platform\ModelClientInterface; | ||
| use Symfony\AI\Platform\Result\TextResult; | ||
| use Symfony\AI\Platform\Result\ToolCall; | ||
| use Symfony\AI\Platform\Result\ToolCallResult; | ||
|
|
||
| /** | ||
| * @author Björn Altmann | ||
|
|
@@ -60,31 +55,6 @@ public function request(Model $model, array|string $payload, array $options = [] | |
| return new RawBedrockResult($this->bedrockRuntimeClient->invokeModel(new InvokeModelRequest($request))); | ||
| } | ||
|
|
||
| public function convert(InvokeModelResponse $bedrockResponse): ToolCallResult|TextResult | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why removing this? |
||
| { | ||
| $data = json_decode($bedrockResponse->getBody(), true, 512, \JSON_THROW_ON_ERROR); | ||
|
|
||
| if (!isset($data['content']) || [] === $data['content']) { | ||
| throw new RuntimeException('Response does not contain any content.'); | ||
| } | ||
|
|
||
| if (!isset($data['content'][0]['text']) && !isset($data['content'][0]['type'])) { | ||
| throw new RuntimeException('Response content does not contain any text or type.'); | ||
| } | ||
|
|
||
| $toolCalls = []; | ||
| foreach ($data['content'] as $content) { | ||
| if ('tool_use' === $content['type']) { | ||
| $toolCalls[] = new ToolCall($content['id'], $content['name'], $content['input']); | ||
| } | ||
| } | ||
| if ([] !== $toolCalls) { | ||
| return new ToolCallResult(...$toolCalls); | ||
| } | ||
|
|
||
| return new TextResult($data['content'][0]['text']); | ||
| } | ||
|
|
||
| private function getModelId(Model $model): string | ||
| { | ||
| $configuredRegion = $this->bedrockRuntimeClient->getConfiguration()->get('region'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,6 +288,26 @@ public function __construct(array $additionalModels = []) | |
| Capability::OUTPUT_TEXT, | ||
| ], | ||
| ], | ||
| '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, | ||
| ], | ||
| ], | ||
|
Comment on lines
+291
to
+310
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert, I extracted it to: |
||
| ]; | ||
|
|
||
| $this->models = array_merge($defaultModels, $additionalModels); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ | |
| final class PlatformFactory | ||
| { | ||
| public static function create( | ||
| BedrockRuntimeClient $bedrockRuntimeClient = new BedrockRuntimeClient(), | ||
| ?BedrockRuntimeClient $bedrockRuntimeClient = null, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh - not sure about this one. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { |
||
| ModelCatalogInterface $modelCatalog = new ModelCatalog(), | ||
| ?Contract $contract = null, | ||
| ?EventDispatcherInterface $eventDispatcher = null, | ||
|
|
@@ -42,6 +42,10 @@ public static function create( | |
| throw new RuntimeException('For using the Bedrock platform, the async-aws/bedrock-runtime package is required. Try running "composer require async-aws/bedrock-runtime".'); | ||
| } | ||
|
|
||
| if (null === $bedrockRuntimeClient) { | ||
| $bedrockRuntimeClient = new BedrockRuntimeClient(); | ||
| } | ||
|
|
||
| return new Platform( | ||
| [ | ||
| new ClaudeModelClient($bedrockRuntimeClient), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\AI\Platform\Bridge\Bedrock\Tests\Anthropic; | ||
|
|
||
| use AsyncAws\BedrockRuntime\BedrockRuntimeClient; | ||
| use AsyncAws\BedrockRuntime\Input\InvokeModelRequest; | ||
| use AsyncAws\BedrockRuntime\Result\InvokeModelResponse; | ||
| use AsyncAws\Core\Configuration; | ||
| use PHPUnit\Framework\MockObject\MockObject; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Symfony\AI\Platform\Bridge\Anthropic\Claude; | ||
| use Symfony\AI\Platform\Bridge\Bedrock\Anthropic\ClaudeModelClient; | ||
| use Symfony\AI\Platform\Bridge\Bedrock\RawBedrockResult; | ||
|
|
||
| final class ClaudeModelClientTest extends TestCase | ||
| { | ||
| private const VERSION = '2023-05-31'; | ||
|
|
||
| private MockObject&BedrockRuntimeClient $bedrockClient; | ||
| private ClaudeModelClient $modelClient; | ||
| private Claude $model; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| $this->model = new Claude('claude-sonnet-4-5-20250929'); | ||
| $this->bedrockClient = $this->getMockBuilder(BedrockRuntimeClient::class) | ||
| ->setConstructorArgs([ | ||
| Configuration::create([Configuration::OPTION_REGION => Configuration::DEFAULT_REGION]), | ||
| ]) | ||
| ->onlyMethods(['invokeModel']) | ||
| ->getMock(); | ||
| } | ||
|
|
||
| public function testPassesModelId() | ||
| { | ||
| $this->bedrockClient->expects($this->once()) | ||
| ->method('invokeModel') | ||
| ->with($this->callback(function ($arg) { | ||
| $this->assertInstanceOf(InvokeModelRequest::class, $arg); | ||
| $this->assertEquals('us.anthropic.claude-sonnet-4-5-20250929-v1:0', $arg->getModelId()); | ||
| $this->assertEquals('application/json', $arg->getContentType()); | ||
| $this->assertTrue(json_validate($arg->getBody())); | ||
|
|
||
| return true; | ||
| })) | ||
| ->willReturn($this->createMock(InvokeModelResponse::class)); | ||
|
|
||
| $this->modelClient = new ClaudeModelClient($this->bedrockClient, self::VERSION); | ||
|
|
||
| $response = $this->modelClient->request($this->model, ['message' => 'test']); | ||
| $this->assertInstanceOf(RawBedrockResult::class, $response); | ||
| } | ||
|
|
||
| public function testUnsetsModelName() | ||
| { | ||
| $this->bedrockClient->expects($this->once()) | ||
| ->method('invokeModel') | ||
| ->with($this->callback(function ($arg) { | ||
| $this->assertInstanceOf(InvokeModelRequest::class, $arg); | ||
| $this->assertEquals('application/json', $arg->getContentType()); | ||
| $this->assertTrue(json_validate($arg->getBody())); | ||
|
|
||
| $body = json_decode($arg->getBody(), true); | ||
| $this->assertArrayNotHasKey('model', $body); | ||
|
|
||
| return true; | ||
| })) | ||
| ->willReturn($this->createMock(InvokeModelResponse::class)); | ||
|
|
||
| $this->modelClient = new ClaudeModelClient($this->bedrockClient, self::VERSION); | ||
|
|
||
| $response = $this->modelClient->request($this->model, ['message' => 'test', 'model' => 'claude']); | ||
| $this->assertInstanceOf(RawBedrockResult::class, $response); | ||
| } | ||
|
|
||
| public function testSetsAnthropicVersion() | ||
| { | ||
| $this->bedrockClient->expects($this->once()) | ||
| ->method('invokeModel') | ||
| ->with($this->callback(function ($arg) { | ||
| $this->assertInstanceOf(InvokeModelRequest::class, $arg); | ||
| $this->assertEquals('application/json', $arg->getContentType()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertSame where possible please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a rector rule that could automate (or at least detect) this:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created an issue |
||
| $this->assertTrue(json_validate($arg->getBody())); | ||
|
|
||
| $body = json_decode($arg->getBody(), true); | ||
| $this->assertEquals('bedrock-'.self::VERSION, $body['anthropic_version']); | ||
|
|
||
| return true; | ||
| })) | ||
| ->willReturn($this->createMock(InvokeModelResponse::class)); | ||
|
|
||
| $this->modelClient = new ClaudeModelClient($this->bedrockClient, self::VERSION); | ||
|
|
||
| $response = $this->modelClient->request($this->model, ['message' => 'test']); | ||
| $this->assertInstanceOf(RawBedrockResult::class, $response); | ||
| } | ||
|
|
||
| public function testSetsToolOptionsIfToolsEnabled() | ||
| { | ||
| $this->bedrockClient->expects($this->once()) | ||
| ->method('invokeModel') | ||
| ->with($this->callback(function ($arg) { | ||
| $this->assertInstanceOf(InvokeModelRequest::class, $arg); | ||
| $this->assertEquals('application/json', $arg->getContentType()); | ||
| $this->assertTrue(json_validate($arg->getBody())); | ||
|
|
||
| $body = json_decode($arg->getBody(), true); | ||
| $this->assertEquals(['type' => 'auto'], $body['tool_choice']); | ||
|
|
||
| return true; | ||
| })) | ||
| ->willReturn($this->createMock(InvokeModelResponse::class)); | ||
|
|
||
| $this->modelClient = new ClaudeModelClient($this->bedrockClient, self::VERSION); | ||
|
|
||
| $options = [ | ||
| 'tools' => ['Tool'], | ||
| ]; | ||
|
|
||
| $response = $this->modelClient->request($this->model, ['message' => 'test'], $options); | ||
| $this->assertInstanceOf(RawBedrockResult::class, $response); | ||
| } | ||
| } | ||
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
?
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