Skip to content

feat: implement Bedrock and model clients for Claude, Llama, and Nova #312

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

Merged
merged 12 commits into from
May 26, 2025

Conversation

bjalt
Copy link
Contributor

@bjalt bjalt commented May 17, 2025

  • Test for Prompt Converters
  • Documentation for limits of invokeModel with tool calling using Nova models
  • Documentation for configuration should be part of the bundle
  • Image support for Claude and Nova using Bedrock
  • Remove local model validation
  • Example usage for Bedrock

chr-hertel added a commit that referenced this pull request May 18, 2025
@bjalt bjalt requested a review from OskarStark May 19, 2025 11:01
@bjalt bjalt marked this pull request as ready for review May 19, 2025 11:01
@bjalt
Copy link
Contributor Author

bjalt commented May 19, 2025

There is some code duplication in Anthropic and Bedrock. For the sake of keeping this PR small I left it in.

Please be also aware that I enabled Claude Modle image support to support. This was needed to use the same model definition for using Bedrock Claude with images. I tested the image implementation for Platform Anthropic with Sonnet 3.7.

@bjalt
Copy link
Contributor Author

bjalt commented May 19, 2025

Fixes #307

@chr-hertel
Copy link
Member

First of all thanks for working on this - I think it's a great addition and a very valid case in challenging the current extension points of the Platform layer - good timing for that large refactoring in #316 - but i guess we should get this merged before the other one.

also the redundancy can be tackled as a follow up - that's currently a matter how the response structured is handled.

please have a look at the phpstan failure, but nevermind the commit message check - just a reminder for me while merging.

i will give it a detailed look right away - thanks again! 👍

@chr-hertel
Copy link
Member

Don't have access to the models on AWS atm, but will trust on your tests here - can you please add some examples though?

@bjalt
Copy link
Contributor Author

bjalt commented May 19, 2025

@chr-hertel

please have a look at the phpstan failure, but nevermind the commit message check - just a reminder for me while merging.

I just looked at the phpstan issue and they should not be there. It runs locally and the code is guarded with an if statement limiting the possible type that can be handled.
Do you utilize any kind of caching in the actions? Can you trigger the qa run again before I set up the GitHub action in my repo to reproduce the issue?

Found the issue, I was just looking at my state not the merge result 🙈

@bjalt
Copy link
Contributor Author

bjalt commented May 20, 2025

@chr-hertel I fixed the ci issues and added some examples.

@bjalt bjalt requested a review from chr-hertel May 20, 2025 07:05
@chr-hertel
Copy link
Member

needs a rebase, but other than that i'd like to merge this - thanks already! :)

@bjalt
Copy link
Contributor Author

bjalt commented May 26, 2025

@chr-hertel Rebased and resolved conflicts in the composer.json

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.

Thanks for you work @bjalt - will continue to get rid off redundencies 👍

@chr-hertel chr-hertel merged commit e6385cc into php-llm:main May 26, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants