Repository navigation
feat: [Anthropic] add anthropic foundation models support and tests - #1116
Open
vladimir-a-sap wants to merge 32 commits into
Open
vladimir-a-sap wants to merge 32 commits into
vladimir-a-sap wants to merge 32 commits into
Conversation
vladimir-a-sap
changed the title
vladimir-a-sap
marked this pull request as ready for review
October 13, 2026 15:24
vladimir-a-sap
marked this pull request as draft
October 14, 2026 08:47
vladimir-a-sap
marked this pull request as ready for review
October 15, 2026 13:32
vladimir-a-sap
marked this pull request as draft
October 15, 2026 13:33
vladimir-a-sap
marked this pull request as ready for review
October 26, 2026 16:29
ZhongpinWang
requested changes
Oct 27, 2026
Co-authored-by: Charles Dubois <103174266+CharlesDuboisSAP@users.noreply.github.com>
CharlesDuboisSAP
left a comment
CharlesDuboisSAP
left a comment
Contributor
There was a problem hiding this comment.
Don't forget to add [Anthropic] to the title of the PR and add it to the PR lint workflow.
Also you should still add a modelAvailabilityTest for this class to the in ScenarioTest
|
|
||
| @Override | ||
| public void close() { | ||
| // Apache HttpClient lifecycle is managed by Cloud SDK's ApacheHttpClient5Cache |
Contributor
There was a problem hiding this comment.
Could you do a proper implementation.
The feature from Cloud SDK that closes open connections is very dubious at best, and was meant for customers that do not handle connection lifecycle properly. We have had many customers that complain about connections still being open and their application failing to open new ones or being out of memory.
- Please remove this comment as I don't think the Cloud SDK feature is foolproof, and you haven't proved it in a unit test.
- We already have streaming unit tests that check that the stream gets closed. You could replace the stream unit test with a test that does not mock
httpClient, inspired from existing tests, that checks that the stream gets closed. - Remove the warning suppression if possible.
|
|
||
| try { | ||
| if (isStreaming) { | ||
| final var apacheResponse = apacheClient.executeOpen(null, apacheRequest, null); |
Contributor
There was a problem hiding this comment.
Why don't you call execute with our existing ClientResponseHandler given as a parameter, surely that could reduce code duplication?
Comment on lines
+62
to
+65
| @Nonnull final HttpRequest request, @Nonnull final RequestOptions requestOptions) { | ||
| final var apacheClient = ApacheHttpClient5Accessor.getHttpClient(destination); | ||
| final var isStreaming = isStreaming(request); | ||
| final var apacheRequest = toApacheRequest(request, isStreaming); |
Contributor
There was a problem hiding this comment.
Surely there is a better way to execute the request that does not involve translating the request field by field and translating back the response field by field.
Jonas-Isr
reviewed
Nov 6, 2026
Jonas-Isr
left a comment
Jonas-Isr
left a comment
Member
There was a problem hiding this comment.
This review only concerns the ADRs. I will review the code later (as discussed) :)
Co-authored-by: Charles Dubois <103174266+CharlesDuboisSAP@users.noreply.github.com>
Co-authored-by: Jonas-Isr <jonas.israel@sap.com>
vladimir-a-sap
changed the title
… into anthropic-claude-support
This branch has not been deployed
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.
Context
https://github.com/SAP/ai-sdk-java-backlog/issues/449
Adds Anthropic models support (foundation models) and integration tests
Definition of Done