fix(model): preserve executionConfig in OllamaOptions fromOptions/toBuilder#2011
Merged
chickenlj merged 1 commit intoJul 6, 2026
Conversation
…uilder OllamaOptions.fromOptions(OllamaOptions) and toBuilder() did not copy the executionConfig field, so instances produced via copy(), fromOptions() or toBuilder() silently lost the timeout/retry configuration carried by executionConfig. merge() and toGenerateOptions() already propagate it; align the two copy paths with them and add a regression test covering all three paths (the existing equals()/hashCode() intentionally ignore executionConfig, so the test asserts getExecutionConfig() directly). Closes agentscope-ai#1957
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration-loss bug in the Ollama model options layer: executionConfig (timeouts/retries) is now preserved when copying OllamaOptions via fromOptions(...), toBuilder(), and therefore copy().
Changes:
- Copy
executionConfiginOllamaOptions.fromOptions(OllamaOptions). - Copy
executionConfiginOllamaOptions.toBuilder(). - Add a regression test to ensure
executionConfigsurvivescopy(),fromOptions(...), andtoBuilder().build().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| agentscope-extensions/agentscope-extensions-model/agentscope-extensions-model-ollama/src/main/java/io/agentscope/extensions/model/ollama/options/OllamaOptions.java | Preserves executionConfig across the previously-missing copy paths (fromOptions, toBuilder). |
| agentscope-extensions/agentscope-extensions-model/agentscope-extensions-model-ollama/src/test/java/io/agentscope/extensions/model/ollama/options/OllamaOptionsTest.java | Adds a regression test verifying executionConfig is retained through copy(), fromOptions, and toBuilder() round-trips. |
Comment on lines
+294
to
+296
| // copy() delegates to fromOptions(OllamaOptions) | ||
| OllamaOptions copy = original.copy(); | ||
| assertEquals(executionConfig, copy.getExecutionConfig()); |
Comment on lines
+298
to
+300
| // fromOptions(OllamaOptions) directly | ||
| OllamaOptions fromOptions = OllamaOptions.fromOptions(original); | ||
| assertEquals(executionConfig, fromOptions.getExecutionConfig()); |
Comment on lines
+302
to
+304
| // toBuilder().build() round-trip | ||
| OllamaOptions rebuilt = original.toBuilder().build(); | ||
| assertEquals(executionConfig, rebuilt.getExecutionConfig()); |
5 tasks
zouyx
pushed a commit
to zouyx/agentscope-java
that referenced
this pull request
Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OllamaOptions.fromOptions(OllamaOptions)andtoBuilder()do not copy theexecutionConfigfield, so any instance produced viacopy(),fromOptions()or
toBuilder()silently loses the timeout/retry configuration carried byexecutionConfig.merge(OllamaOptions)andtoGenerateOptions()already propagateexecutionConfig, so the two copy paths were simply missing it. This alignsthem with the rest of the class.
Test
Added
OllamaOptionsTest#testExecutionConfigPreservedOnCopy, which verifies thatexecutionConfigsurvivescopy(),fromOptions()andtoBuilder(). Note thatequals()/hashCode()intentionally ignoreexecutionConfig, so the testasserts
getExecutionConfig()directly.Closes #1957