-
Notifications
You must be signed in to change notification settings - Fork 221
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.
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
feat/added bedrock streaming #314
base: main
Are you sure you want to change the base?
feat/added bedrock streaming #314
Conversation
@lloydhamilton Hi, can you make sure tests pass locally too please, I will review |
No probs, will get it fixed. I know what the issue is. Test passed locally on mine as I have a default AWS profile set up. Will mock those env vars. |
hey @fm1320 I have updated the tests. Could you verify that the tests passed? |
Hi @lloydhamilton , any more updates or is this ready? It generally looks good I just want to test a few more things when its complete |
Hey @fm1320 no more updates on my end for this PR. Do let me know how the tests go. I should be available to address any changes. |
What does this PR do?
Added streaming functionality for Bedrock API integration. Followed as closely as possible the coding styles written in
openai_client.py
andollama_client.py
..converse_stream
endpoint when streaming is true.streaming = True
isapi_kwargs
.Since the
stream
parameter is not a parameter recognised by Bedrock, I had to addapi_kwargs.pop("stream")
when evaluatingapi_kwargs
prior to bedrock API call.Related issue:
#283
Add tests that extends the tests written in #289
Before submitting