-
-
Notifications
You must be signed in to change notification settings - Fork 116
[AI Bundle][Platform] Fix Anthropic platform definition #369
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
VincentLanglet
commented
Aug 29, 2025
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| Docs? | no |
| Issues | Fix #363 |
| License | MIT |
|
@Krystopher88 could you try this and confirm it solves your issue ? |
| HttpClientInterface $httpClient, | ||
| #[\SensitiveParameter] private string $apiKey, | ||
| private string $version = '2023-06-01', | ||
| private ?string $version = 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.
Why this change?
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 is doing
new ModelClient($httpClient, $apiKey, $version)
Which means that, either
- It has to know the default value, so
$version = '2023-06-01',is duplicated - It requires the logic
null === $version ? new ModelClient($httpClient, $apiKey) : new ModelClient($httpClient, $apiKey, $version);
Also, since I needed to pass the platform version in the AiBundle config, I didn't want to pass
$platform['version'] ?? '2023-06-01',
with the version hard coded a third time.
| #[\SensitiveParameter] | ||
| string $apiKey, | ||
| string $version = '2023-06-01', | ||
| ?string $version = 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.
What about moving this parameter at the end instead?
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.
Might be a solution.
But still I'm unsure it a nice pattern to duplicate the default value '2023-06-01' between the factory and the Model, no ?
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 for jumping on that @VincentLanglet!
I'm usually not a huge fan of nullable arguments, and it's true that this is a bit weird in handling and duplicated here.
One thing I noticed when looking at the Anthropics docs again: there are only two version strings at this point 2023-01-01 and the 2023-06-01 we're currently using. and guess what, only one is supported by our implementation here anyways. with 2023-01-01 you get the error "anthropic-version: "2023-01-01" not allowed for this endpoint" error.
So, if you're up for it, let's just clean that up, remove all arguments, the bundle config option and hard code the version in the header: 'anthropic-version' => '2023-06-01' - easier and we don't have to debate the nullable and where not to duplicate stuff 😆
|
Please fix PHPStan, thanks |
|
Thanks for fixing this bug Vincent. |
|
@OskarStark saw my comment? any thoughts on that? |
|
You mean regarding the defaults? Mixed feelings, as if there is a new version, you can only use it with a new release of the lib, like for the models |