Implement AWS Bedrock Support#233
Conversation
|
@microsoft-github-policy-service agree company="Aplyca Tecnología SAS" |
|
Hello @jennifermarsman, I hope you're doing well. I wanted to check if there's anything else I should do to get it reviewed or assigned. I’d really appreciate any guidance. Thanks in advance! |
|
Hi @bsanchez-aplyca - I made some changes to your PR but can't push to your branch. I made a new branch of same name here - can you pull these changes in? (Can you allow edits from maintainers in the PR?) There is one more bug I am trying to run down so I think almost there. Thanks! https://github.com/microsoft/NLWeb/tree/implement-aws-bedrock-support |
|
Hello @chelseacarter29 — the "Allow edits by maintainers" option didn’t show up on my side since the fork was made under an organization. But I’ve pulled in the changes from your branch, so everything should be up to date now. Thanks a lot for the help! |
|
@chelseacarter29, @rvguha I was wondering if there is any traction with the PR review for this set of features, adding AWS Bedrock Support. The PR was added on June 25, and I know it takes significant effort to keep it up to date. Is there a policy towards rejecting them? Could you or the rest of the NLWeb team clarify if these contributions will be considered? |
|
Hi @bsanchez-aplyca — thanks for this contribution, AWS Bedrock support would be great to have. The repo has been restructured since this PR was opened — Happy to help if you run into any issues with the rebase. |
rvguha
left a comment
There was a problem hiding this comment.
Hi @bsanchez-aplyca — thanks for the contribution, AWS Bedrock support is something we'd like to have. I've done a detailed review and have some feedback before we can merge this.
Blocking Issues
1. Runtime pip install in embedding.py — please remove
The _ensure_package_installed() function runs subprocess.check_call([sys.executable, "-m", "pip", "install", ...]) at request time. This is a security and reliability hazard in production — dependencies should be installed at deploy time via requirements.txt, not at runtime. Please remove the _ensure_package_installed function, the _embedding_provider_packages dict, and the related sys/subprocess imports from embedding.py.
2. Synchronous boto3 call inside async method
AWSBedrockProvider.get_completion() is declared async but calls client.invoke_model() synchronously, which blocks the event loop. Please wrap it with asyncio.run_in_executor():
import asyncio
loop = asyncio.get_event_loop()
response = await loop.run_in_executor(
None, lambda: client.invoke_model(modelId=model, body=json.dumps(body))
)3. Anthropic message format is incorrect
In _build_model_body, the Anthropic branch constructs:
{"role": "system", "content": {"type": "text", "text": ...}}The Anthropic Messages API does not accept "role": "system" as a message. The system prompt must be a top-level system parameter. This would fail at runtime against Bedrock's Anthropic models. Please restructure to:
{
"anthropic_version": "bedrock-2023-05-31",
"system": "Provide a valid JSON response matching this schema: ...",
"messages": [
{"role": "user", "content": [{"type": "text", "text": prompt}]}
],
"max_tokens": max_tokens,
"temperature": temperature,
}4. Non-standard credential handling
Packing access_key_id:secret_access_key into a single AWS_BEDROCK_API_KEY env var is fragile and breaks AWS conventions. boto3 natively reads AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_DEFAULT_REGION (as well as credential profiles and instance roles). Please use the standard AWS env vars and let boto3 handle credential resolution. This also eliminates the colon-parsing logic and the risk of keys containing :.
5. api_version_env repurposed for region
Both config_llm.yaml and config_embedding.yaml store the AWS region in the api_version_env field. This is a semantic mismatch that will confuse future contributors. If the config schema doesn't have a region field, let's discuss adding one rather than overloading an existing field.
Minor Issues
- Copy-paste docstring:
aws_bedrock_embedding.pysays"OpenAI embedding implementation"— please update. - Scope creep in
.env.template: The PR adds env vars for Hugging Face (HF_TOKEN), Cloudflare AutoRAG, andNLWEB_LOGGING_PROFILEthat are unrelated to Bedrock. Please split those out into a separate PR. - Duplicate client initialization: The embedding provider and LLM provider each create their own boto3
bedrock-runtimeclient with slightly different patterns. Consider sharing a single client factory. - Batch embedding is sequential:
batch_get_embeddingsjust loops over single calls. This is fine as a starting point but worth a TODO comment. - Trailing whitespace in a few places (e.g., after
return resultinembedding.py).
Rebase Required
As I mentioned in my earlier comment, the repo has been restructured since this PR was opened. Please rebase against current main and update the file paths accordingly. Happy to help if you run into issues.
Looking forward to the updated PR!
…t LLMProviderConfig interface
…called before LLM; additional refactoring changes.
…features for llm and embeddings for Bedrock - Updated AWS Bedrock environment variables to .env.template - Refactored aws_bedrock_embedding.py to support other AWS Bedrock embeddings models than titan. - Created aws_bedrock_client.py for shared Bedrock client management. - Updated config files to remove deprecated API key references - Enhanced setup documentation for AWS Bedrock integration.
8809b2d to
21cfbda
Compare
|
Hello @rvguha, thanks for your time and your feedback, I just pushed the fixes and some other updates in order to support the major of fundational models in AWS bedrock. I did too the rebase. Best regards. |
|
@bsanchez-aplyca — Thank you so much for the incredible work on this update! I really appreciate the time and effort you put into the rebase and addressing all the feedback. The changes look fantastic — the shared client, the A few small things I noticed that would be great to clean up before we merge:
None of these are major — really close to being ready. Thanks again for the excellent contribution! 🙏 |
|
Hello @rvguha, solved, and really thanks for your time :D |
rvguha
left a comment
There was a problem hiding this comment.
@bsanchez-aplyca — This looks great, approving! Thank you so much for your patience and persistence with this PR over the past several months, and for being so responsive to all the feedback. The code is clean and well-structured, and AWS Bedrock support is a really valuable addition to NLWeb. Much appreciated! 🎉
Hi all,
In this pull request we have added the support for AWS Bedrock and its main foundation models for text and embeddings.
Best regards.