Skip to content

feat(spring-ai-bedrock-converse): Introduce BedrockProxyChatOptions #1760

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(spring-ai-bedrock-converse): Introduce BedrockProxyChatOptions #1760

wants to merge 1 commit into from
+486 −25

Conversation

tzolov
Copy link
Contributor

@tzolov tzolov commented Nov 19, 2024

  • Add new BedrockProxyChatOptions class to replace FunctionCallingOptions
  • Create BedrockProxyChatOptionsBuilder for fluent configuration
  • Update BedrockProxyChatModel and related classes to use new options
  • Add support for additional model-specific parameters via 'additional' map
  • Improve options merging and handling of chat configuration

Related to #1758

@tzolov
feat(spring-ai-bedrock-converse): Introduce BedrockProxyChatOptions f…
d6155c6 
…or more flexible chat configuration

- Add new BedrockProxyChatOptions class to replace FunctionCallingOptions
- Create BedrockProxyChatOptionsBuilder for fluent configuration
- Update BedrockProxyChatModel and related classes to use new options
- Add support for additional model-specific parameters via 'additional' map
- Improve options merging and handling of chat configuration
@tzolov tzolov mentioned this pull request Nov 19, 2024
Closed
12 tasks
ilayaperumalg
ilayaperumalg reviewed Nov 19, 2024
View reviewed changes
...-converse/src/main/java/org/springframework/ai/bedrock/converse/BedrockProxyChatOptions.java
* @author Christian Tzolov
* @since 1.0.0
*/
public class BedrockProxyChatOptions implements FunctionCallingOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why BedrockProxyChatOptions can not implement ChatOptions here as I think we got all of the ChatOptions as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FunctionCallingOptions implements ChatOptions already

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I got deceived by the explicit declaration of ChatOptions in some of the model option classes. I just pushed a minor PR to fix this: #1762

ilayaperumalg
ilayaperumalg reviewed Nov 19, 2024
View reviewed changes
...-converse/src/main/java/org/springframework/ai/bedrock/converse/BedrockProxyChatOptions.java
* @author Christian Tzolov
* @since 1.0.0
*/
public class BedrockProxyChatOptions implements FunctionCallingOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Not specific to this PR, but, do you think it would be good to have a DefaultFunctionCallingOptions as the default implementation for FunctionCallingOptions so that all the Model Chat options can extend? or, any reason why we didn't have the default implementation at the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have something like this called PortableFunctionCallingOptions, but it needs some cleaning regarding the Builder/Object relation ship there.
I started by extending it but then for the sake of the flexibility for the draft I've copied its content.
IMO Before merging this PR we would likely revert to the PortableFunctionCallingOptions parent, unless there are some constrains.

@tzolov tzolov added this to the 1.0.0-M5 milestone Nov 25, 2024
@markpollack markpollack self-assigned this Dec 5, 2024
@markpollack markpollack modified the milestones: 1.0.0-M5, 1.0.0-M6 Dec 21, 2024
@ilayaperumalg ilayaperumalg modified the milestones: 1.0.0-M6, 1.0.0-M7 Feb 12, 2025
@markpollack markpollack modified the milestones: 1.0.0-M7, 1.0.0-RC1 Apr 3, 2025
@markpollack markpollack marked this pull request as ready for review April 4, 2025 15:32
@ilayaperumalg ilayaperumalg modified the milestones: 1.0.0-M7, 1.0.0-RC1 Apr 10, 2025
@ilayaperumalg ilayaperumalg assigned tzolov and unassigned markpollack Apr 10, 2025
@markpollack
Copy link
Member

@tzolov still relevant?

@tzolov
Copy link
Contributor Author

tzolov commented May 10, 2025

Yes it is.
Amongst others it aims to expose the additionalModelRequestFields - Additional inference parameters that the model supports, beyond the base set of inference parameters that Converse and ConverseStream support in the inferenceConfig field. (part of #1758)

But perhaps we can move it for after the GA

@tzolov tzolov modified the milestones: 1.0.0-RC1, 1.0.x May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilayaperumalg ilayaperumalg ilayaperumalg left review comments

Assignees

@tzolov tzolov

Labels
aws Bedrock design enhancement New feature or request status: to-discuss
Projects
None yet
Milestone
1.1.x
Development

Successfully merging this pull request may close these issues.
3 participants
@tzolov @markpollack @ilayaperumalg