Skip to content

Use Inference Profiles on AWS Bedrock Models #506

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 7, 2025
Merged

Use Inference Profiles on AWS Bedrock Models #506

merged 6 commits into from
Aug 7, 2025

Conversation

aozherelyeva
Copy link
Contributor

Fixes #488

Type of the change

  • New feature
  • Bug fix
  • Documentation fix
  • Tests improvement

Checklist for all pull requests

  • The pull request has a description of the proposed change
  • I read the Contributing Guidelines before opening the pull request
  • The pull request uses develop as the base branch
  • [] Tests for the changes have been added
  • [] All new and existing tests passed
Additional steps for pull requests adding a new feature
  • An issue describing the proposed change exists
  • The pull request includes a link to the issue
  • The change was discussed and approved in the issue
  • Docs have been added / updated

@aozherelyeva aozherelyeva requested review from sdubov and devcrocod July 30, 2025 16:49
@aozherelyeva
Copy link
Contributor Author

Heavy tests aren't fixed yet, but I'm on my way to do it.

@aozherelyeva aozherelyeva force-pushed the zarechneva/aws-bedrock-llm-profiles-fix branch from 1b2bbf2 to fe0f6e4 Compare July 30, 2025 17:16
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 30, 2025 •
edited
Loading

Qodana for JVM

385 new problems were found

Inspection name Severity Problems
Check Kotlin and Java source code coverage 🔶 Warning 380
Vulnerable imported dependency 🔶 Warning 4
String concatenation that can be converted to string template ◽️ Notice 1 
@@ Code coverage @@
+ 68% total lines covered
8823 lines analyzed, 6019 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

@aozherelyeva aozherelyeva force-pushed the zarechneva/aws-bedrock-llm-profiles-fix branch from fe0f6e4 to b86fa5b Compare August 1, 2025 07:37
devcrocod
devcrocod reviewed Aug 1, 2025
View reviewed changes
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should look into how models are provided in AWS

As far as I understand, the structure is:
Region → Model Family → Model Name

We could wrap this in enums or classes to avoid using raw strings. It would also simplify our code by removing string-based matching

@aozherelyeva aozherelyeva force-pushed the zarechneva/aws-bedrock-llm-profiles-fix branch 3 times, most recently from 2d1ada9 to fb92132 Compare August 5, 2025 11:03
samDobsonDev
samDobsonDev reviewed Aug 5, 2025
View reviewed changes
Copy link

@samDobsonDev samDobsonDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I have very little knowledge of the wider scope that this code might affect, but from a usage standpoint, it looks like it should do the trick :)

@aozherelyeva aozherelyeva force-pushed the zarechneva/aws-bedrock-llm-profiles-fix branch from fb92132 to fb0c10a Compare August 5, 2025 12:09
@aozherelyeva aozherelyeva requested a review from Rizzen August 5, 2025 13:16
devcrocod
devcrocod approved these changes Aug 7, 2025
View reviewed changes
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, PR looks good, but please take a look at my questions and comments

I think in this case it would be better to create an issue and a separate PR for fixing the Bedrock client

...eatures-memory/src/jvmMain/kotlin/ai/koog/agents/memory/storage/Aes256GCMStorageEncryptor.kt
@@ -1,7 +1,7 @@
package ai.koog.agents.memory.storage

import java.security.SecureRandom
import java.util.*
import java.util.Base64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good practice would be to use the Kotlin api where possible. Although in this case it doesn’t change anything, since kotlin simply wraps Java Base64. This approach will reduce potential issues if the code ever needs to be moved to common

...r-bedrock-client/src/jvmMain/kotlin/ai/koog/prompt/executor/clients/bedrock/BedrockModels.kt
*
* Represents all available AWS regions where Bedrock service is supported.
*/
@Serializable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pay attention to the behavior of the Serializable enum
https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/json.md#decoding-enums-in-a-case-insensitive-manner

@aozherelyeva aozherelyeva force-pushed the zarechneva/aws-bedrock-llm-profiles-fix branch from 32f9cfa to 51ec4dc Compare August 7, 2025 13:34
@aozherelyeva aozherelyeva merged commit 76bc3a6 into develop Aug 7, 2025
5 checks passed
@aozherelyeva aozherelyeva deleted the zarechneva/aws-bedrock-llm-profiles-fix branch August 7, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devcrocod devcrocod devcrocod approved these changes

@sdubov sdubov Awaiting requested review from sdubov

@Rizzen Rizzen Awaiting requested review from Rizzen

+1 more reviewer

@samDobsonDev samDobsonDev samDobsonDev left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for Inference Profiles on AWS Bedrock Models
3 participants
@aozherelyeva @devcrocod @samDobsonDev