Fix: Address several Bedrock converse streaming issues #4375
Conversation
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
c38c17d to
60da4b9
Compare
|
Updated to set |
| .system(converseRequest.system()) | ||
| .additionalModelRequestFields(converseRequest.additionalModelRequestFields()) | ||
| .toolConfig(converseRequest.toolConfig()) | ||
| .requestMetadata(converseRequest.requestMetadata()) |
There was a problem hiding this comment.
This is recently added via 45baa39.
There was a problem hiding this comment.
Thanks. Dropped commit.
There was a problem hiding this comment.
Actually this is missing. It was added for the ConverseRequest but the mapping to ConverseStreamRequest was missed in that commit.
There was a problem hiding this comment.
@emopti-jrufer oh yeah, thanks for catching that!
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
45e41ea to
60da4b9
Compare
|
@ilayaperumalg Would you like me to create a back port PR off of the 1.0.x branch? Just trying to understand what is need to get this merged in as there are some critical issues this addresses. Most importantly the finish reason and multiple tool requests for a single tool_use stop reason not being processed in a single request. |
|
@emopti-jrufer Thanks for the follow up and your willingness to get these changes into 1.0.x. The first impression of this PR looks really good. We will target to get this merged for 1.1.0-M2. Once merged, we can co-ordinate on the back-porting into 1.0.x.,
While reviewing this PR, we also noticed that concurrency handling needs to be improved across - as a separate issue. Could you help with identifying specific concurrency issues related to this PR (this can help as a base for other models as well)? Thank you! |
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
b747373 to
015b9c2
Compare
|
@ilayaperumalg I have updated the since version to reflect the targeted release. Also noticed I didn’t remove the dead code from With regard to concurrency I am glad to help and assume you are referring to the use of some of the classes in the java.util.concurrent namespace. In the case of this PR I think the issue is more of ensuring the visibility of changes to variables across multiple threads. The |
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
015b9c2 to
b045a1e
Compare
|
@ilayaperumalg Just wanted to check on the status of this PR. Is there anything I can help with as the current version is unusable. |
|
Hi @emopti-jrufer Sorry about the delay. The changes look good. With the PR, I see a test failure on BedrockNovaChatClientIT#toolAnnotationWeatherForecastStreaming for anthropic.claude-3-7-sonnet model. Could you check? |
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
b045a1e to
295e46b
Compare
- Correct finish reason when stop reason is not tool_use - Populate finish reason for non-tool_use cases - Ensure multiple tool calls are output in ChatResponse Closes spring-projectsgh-4374, spring-projectsgh-4126, spring-projectsgh-3251 Signed-off-by: Jared Rufer <[email protected]>
Signed-off-by: Jared Rufer <[email protected]>
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
56ce370 to
bad5d81
Compare
Signed-off-by: Jared Rufer <[email protected]>
emopti-jrufer
force-pushed
the
FIX_4374
branch
from
bad5d81 to
3b3ee5f
Compare
|
@ilayaperumalg I have identified and corrected the issue. While testing ran into service availability issues with Bedrock so I updated the tests to use cross-region inference. A couple tests were already using cross-region references. All tests are passing. |
|
@emopti-jrufer Thanks very much for your contribution and patience while getting this PR reviewed/merged. Rebased the commits, squashed and merged as a897177 |
3 participants
Closes gh-4374, gh-4126, gh-3251