Improve chat client entity with structured output integration tests

[nicolaskrier]

Description

• Use MistralAiChatOptions instead of DefaultChatOptions to fix chatClientEntityWithStructuredOutput test,
• Polish MistralAiChatModelIT integration tests.

Explanations

I have noticed a failing integration for Mistral AI on the latest integration tests.
The test is failing due to this assertion that is not true. The ChatOptions is not an instance of MistralAiChatOptions here so nativeStructuredOutputUsed flag is not set to true.

ChatOptions used to be nullable and a vendor specific implementation like MistralAiChatOptions was created in the end when it was null. ChatOptions isn't nullable anymore and DefaultChatOptions is used by default. Consequently, the caller is forced to provide the ChatOptions specific to the vendor if it wants to benefit from its behavior.

I think it is acceptable to do so here to fix this integration test.

Related work

The non-nullability behavior has been introduced with #5403.

[sdeleuze]

@nicolaskrier After a second look, I am wondering if we can avoid this kind of breakage without changing the code on user site, could you please share the error you see on integration tests?

[ilayaperumalg]

@sdeleuze Here is the integration test failure:

Error: Tests run: 16, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 22.10 s <<< FAILURE! -- in org.springframework.ai.mistralai.MistralAiChatModelIT
Error: org.springframework.ai.mistralai.MistralAiChatModelIT.chatClientEntityWithStructuredOutput -- Time elapsed: 0.552 s <<< FAILURE!
org.opentest4j.AssertionFailedError:
[Native structured output should be used with ResponseFormat.Type.JSON_SCHEMA]
Expecting value to be true but was false
at org.springframework.ai.mistralai.MistralAiChatModelIT.chatClientEntityWithStructuredOutput(MistralAiChatModelIT.java:542)

You can find the complete test run here

[nicolaskrier]

@nicolaskrier After a second look, I am wondering if we can avoid this kind of breakage without changing the code on user site, could you please share the error you see on integration tests?

@sdeleuze:

• I have updated the description with better explanations. I hope it is clearer now.
• I am not sure if we should modify the test or the actual behavior. Providing by default a non-vendor specific ChatOptions is debatable. As a Spring AI user, I will expect this behavior at least to be documented (maybe in Prompt class Javadoc?) and highlighted as a breaking change for Spring AI 2 migration.
• It is not handy to investigate these modifications without a Mistrai AI API key that is why I have implemented an integration test similar to Mistral AI's one for Ollama in a second commit of this pull request. I hope it will help you.

@ilayaperumalg: Thank you for quoting the test failure, I should have done it initially.

[sdeleuze]

@nicolaskrier Thanks for adding the Ollama test, indeed that helps a lot.

Looks like indeed a side effect of #5403 due to ChatModelCallAdvisor#augmentWithFormatInstructions storing the output schema in the prompt option only if it is an instance of StructuredOutputChatOptions.

IMO it is not acceptable to force users to do an explicit .chatOptions(OllamaChatOptions.builder().build()) since it should provide the same behavior than not invoking it.

[sdeleuze]

I think the proper way to fix those integration tests is to refine #5403 by updating the processing of the model options to still convert to OllamaChatOptions (or similar more specific model options) via ModelOptionsUtils.copyToTarget, but to do it when they are not instances of OllamaChatOptions, not when they are null.

[nicolaskrier]

I had another idea in mind before seeing your suggestion. I added a third commit containing my proposal. There are still several unit tests failing but chatClientEntityWithStructuredOutput integration tests for Mistral AI and Ollama are passing on my computer. What do you think about this approach?

If you prefer yours, I propose that you create another PR with your approach and then I will modify this PR to polish MistralAiChatModelIT integration tests and to add chatClientEntityWithStructuredOutput integration test to OllamaChatModelIT because I will be less available the coming days.

[sdeleuze]

I think the approach you proposed will still induce side effects given current Spring AI design, so maybe yeah please polish this PR without the third commit (it is ok to force push) and I will create another PR to fix the regression I introduced with #5403 and merge both PRs.

[nicolaskrier]

Thank you for having taken time to review my proposal. I think the incoming changes are quite impactful, so I appreciate you spotting the side effects early on.
I will take a closer look at your PR, and in the meantime I have made the adjustments we discussed.

[sdeleuze]

Merged in main via 617c290, I have chosen to revert #5403 as at prompt level, the options should in fact remain nullable, otherwise too much side effects.

I will monitor CI runs of OllamaChatModelIT as it works in IDEA but fails on command line for reasons I am for now unable to explain (I cleaned the compilation cache and tried various other things).

[sdeleuze]

So with @ilayaperumalg we confirm that in IDEA, OllamaChatModelIT#chatClientEntityWithStructuredOutput works because the options is an instance of OllamaChatOptions but on command line, it fails because it is a DefaultChatOptions! We are trying to figure out why.

[nicolaskrier]

This is weird! I have seen #5441 planned for M4. You may consider disabling this test until this issue is resolved if it becomes a blocking point.

[sdeleuze]

Seems like this was due to the command documented to run integration tests in https://github.com/spring-projects/spring-ai?tab=readme-ov-file#building missing the -am flag (to build also dependent modules in the Maven Reactor) and the -Dfailsafe.failIfNoSpecifiedTests=falseone (to not fail due to the test specified not being in the dependent modules). Took me some time to figure out as the test running was the correct one but with an old implementation.

I will confirm tomorrow with @ilayaperumalg and @ericbottard, and update the README accordingly if confirmed.