Skip to content

feat(ollama): add retry template integration to OllamaChatModel #1852

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
Closed
wants to merge 3 commits into from
Closed

feat(ollama): add retry template integration to OllamaChatModel #1852

wants to merge 3 commits into from

Conversation

apappascs
Copy link
Contributor

@apappascs apappascs commented Dec 2, 2024 •
edited
Loading

  • Integrated RetryTemplate for API calls in OllamaChatModel.
  • Ensured consistent retry logic across all model classes.

Run a build and make sure all tests pass prior to submission

./mvnw spring-javaformat:apply
./mvnw clean install -DskipTests -Dmaven.javadoc.skip=true
./mvnw verify -Pintegration-tests -pl spring-ai-core 
./mvnw verify -Pintegration-tests -pl models/spring-ai-ollama
./mvnw verify -Pintegration-tests -pl spring-ai-spring-boot-autoconfigure 
./mvnw javadoc:javadoc -Pjavadoc

@apappascs apappascs marked this pull request as ready for review December 2, 2024 15:15
@ThomasVitale
Copy link
Contributor

Thanks for your contribution!

ThomasVitale
ThomasVitale reviewed Dec 3, 2024
View reviewed changes
models/spring-ai-ollama/src/main/java/org/springframework/ai/ollama/OllamaChatModel.java Outdated
@@ -198,7 +213,8 @@ public Flux<ChatResponse> stream(Prompt prompt) {

observation.parentObservation(contextView.getOrDefault(ObservationThreadLocalAccessor.KEY, null)).start();

Flux<OllamaApi.ChatResponse> ollamaResponse = this.chatApi.streamingChat(request);
Flux<OllamaApi.ChatResponse> ollamaResponse = this.retryTemplate
Copy link
Contributor

Choose a reason for hiding this comment

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

When in streaming mode, we don't use the RetryTemplate as it's not gonna work with the reactive streams. There is pending design on how to configure retries for streaming. You can follow this issue for updates: #1193

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ThomasVitale for pointing this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding some context to this issue: a RetryTemplate is used to add retry capabilities to the blocking model calls. At first, the same strategy was used for the streaming calls. After realising that it wasn't working as expected in a reactive stream, the usage of RetryTemplate streaming calls has been removed. In this issue, we would need to come up with a design for a retry feature to adopt in all streaming implementations across the different model providers.

Its still in use in the code tho: https://github.com/search?q=repo%3Aspring-projects%2Fspring-ai+%22Flux%3CChatCompletionChunk%3E%22+this.retryTemplate+execute&type=code

should I open a PR removing it?

Copy link
Member

Choose a reason for hiding this comment

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

I think we do need a PR to remove that and to create an EPIC issue that reviews all the stream implementations of call the models for retry when streaming.

@apappascs apappascs force-pushed the feature/add-retry-to-ollama-model branch from 3a28241 to 729b807 Compare December 5, 2024 10:19
@apappascs apappascs force-pushed the feature/add-retry-to-ollama-model branch from 729b807 to 55d38fd Compare December 10, 2024 11:55
@apappascs apappascs force-pushed the feature/add-retry-to-ollama-model branch from 55d38fd to bfbc64b Compare January 20, 2025 16:42
@apappascs
Copy link
Contributor Author

@ilayaperumalg @ThomasVitale - If you have some availability in the coming days, I'd appreciate it if you could take a look.

apappascs and others added 2 commits March 2, 2025 21:30
@apappascs
feat(ollama): add retry template integration to OllamaChatModel
f08e9d9 
Signed-off-by: Alexandros Pappas <[email protected]>
@apappascs
refactor
201edde 
Signed-off-by: Alexandros Pappas <[email protected]>
@apappascs apappascs force-pushed the feature/add-retry-to-ollama-model branch from bfbc64b to 201edde Compare March 2, 2025 20:31
@apappascs
code refactor after rebase
8222f78 
Signed-off-by: Alexandros Pappas <[email protected]>
@apappascs
Copy link
Contributor Author

resolves #2372

@markpollack
Copy link
Member

Thanks for your patience!

@markpollack markpollack added this to the 1.1.x milestone Jun 6, 2025
@markpollack markpollack assigned markpollack and unassigned tzolov Jun 6, 2025
@apappascs apappascs mentioned this pull request Jun 12, 2025
Open
@markpollack
Copy link
Member

rebaesd and merged. updated to ollama 0.6.7 as well. modified one test that was supposed to fail, but took a long time to exhause the retry, so disabled retry for that test.

merged in cfbefee

@dev-jonghoonpark
Copy link
Contributor

dev-jonghoonpark commented Jun 22, 2025 •
edited
Loading

Other AutoConfiguration classes located under /auto-configurations/**.
but, OllamaAutoConfiguration is located in /spring-ai-spring-boot-autoconfigure/src/main/java/org/springframework/ai/autoconfigure/ollama/OllamaAutoConfiguration.java
is this intended?

I suspect it might be using the previous directory path because the PR was created about 6 months ago, so I’m leaving a comment about it.

@markpollack

@apappascs
Copy link
Contributor Author

Other AutoConfiguration classes located under /auto-configurations/**. but, OllamaAutoConfiguration is located in /spring-ai-spring-boot-autoconfigure/src/main/java/org/springframework/ai/autoconfigure/ollama/OllamaAutoConfiguration.java is this intended?

I suspect it might be using the previous directory path because the PR was created about 6 months ago, so I’m leaving a comment about it.

@markpollack

looks like you are correct .

@dev-jonghoonpark dev-jonghoonpark mentioned this pull request Jun 23, 2025
Merged
@dev-jonghoonpark
Copy link
Contributor

I have created a PR to address the mentioned part.

@apappascs apappascs mentioned this pull request Jun 23, 2025
Closed
@apappascs
Copy link
Contributor Author

I have created a PR to address the mentioned part.

thank you for the pr @dev-jonghoonpark . it looks like we were working on this at the same time. did you tried to compile the code and run the tests?

./mvnw verify -Pintegration-tests -pl auto-configurations/models/spring-ai-autoconfigure-model-ollama
./mvnw verify -Pintegration-tests -pl models/spring-ai-ollama

the FunctionCallbackResolver is not in the codebase anymore so I had to remove it. But also follow the style of AnthropicChatAutoConfiguration, and others . Do you want to have a look on my PR https://github.com/spring-projects/spring-ai/pull/3648/files ?

I did n't had the time yet to check if this class is still needed as i can see seperate auto configuration classes for api, model, embeddings, etc, like on the org.springframework.ai.model.openai.autoconfigure package.

@dev-jonghoonpark
Copy link
Contributor

dev-jonghoonpark commented Jun 23, 2025 •
edited
Loading

In PR #3645 ,
I have migrated the code changes from legacy OllamaAutoConfiguration to OllamaApiAutoConfiguration and OllamaChatAutoConfiguration.

Currently, I am running the tests, so I have marked it PR a draft.
Testing is taking a long time as it is being performed locally.

I will reopen the PR once the tests are completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markpollack markpollack markpollack left review comments

@ThomasVitale ThomasVitale Awaiting requested review from ThomasVitale

Assignees

@markpollack markpollack

Labels
for: backport-to-1.0.x ollama
Projects
None yet
Milestone
1.1.x
Development

Successfully merging this pull request may close these issues.
5 participants
@apappascs @ThomasVitale @markpollack @dev-jonghoonpark @tzolov