Skip to content

Conversation

@ThomasVitale
Copy link
Contributor

When calling the OpenAiChatModel with ChatOptions, they are always ignored. Options are considered only when using OpenAiChatOptions or FunctionCallingOptions.

This seems a regression after the recent refactoring of FunctionCallingOptions. I checked all the model implementation and it's only OpenAI having this error.

When calling the OpenAiChatModel with ChatOptions, they are always ignored. Options are considered only when using OpenAiChatOptions or FunctionCallingOptions.

Signed-off-by: Thomas Vitale <[email protected]>
FunctionCallingOptions.class, OpenAiChatOptions.class);
}
else if (prompt.getOptions() instanceof OpenAiChatOptions) {
else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same implementation as all the other model integrations. The problem was only in OpenAI.

@tzolov
Copy link
Contributor

tzolov commented Nov 27, 2024

I've noticed this issue last week. Initially the FunctionCallingOptions where NOT extending from ChatOptions.
So the above implementation was making sense. But since we made FunctionCallingOptions extends ChatOptions this obviously a bogus behavior and IMO we need to fix for all models.

@tzolov tzolov self-assigned this Nov 27, 2024
@tzolov tzolov added chat options bug Something isn't working labels Nov 27, 2024
@tzolov tzolov added this to the 1.0.0-M5 milestone Nov 27, 2024
@ThomasVitale
Copy link
Contributor Author

@tzolov I haven't seen the bug in the other chat implementations since they all already have an "else" statement that makes sure the portable options are mapped correctly. Can we get the bug fixed in OpenAI with this PR and then have the refactoring to improve the logic in a separate issue, now that FunctionCallingOptions extends ChatOptions?

@tzolov
Copy link
Contributor

tzolov commented Nov 28, 2024

@ThomasVitale i guess you are right. I was concerned about the order, but it correct (e.g. from more specific to the generic type)
Thanks for the fix!

@tzolov tzolov merged commit d0a0c87 into spring-projects:main Nov 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working chat options

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants