fix: use IAM roles for bedrock access#578
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
|
@jakelorocco you are tagged as a reviewer because I changed core.py to only call post-processing if there are no exceptions. The status quo continues to give us pain. |
|
I think that sounds good. I will review once the changes are done. We will need to make sure the finally block / telemetry stuff doesn't cause issues with this change. |
|
Looks like there's another draft PR open to more comprehensively fix the post process issue: #580 (comment). I will take a closer look tomorrow. |
|
Hey folks - thank you so much for adding support for the BedRock non-mantle endpoints through the LiteLLM backend! I'm currently working on integrating Mellea with a fork of IBM's FactReasoner. I made a couple additional (local) edits to this PR in order to get it to work for me, which include:
Let me know if these changes would be valuable to contribue back to the main Mellea repo, I'm happy to either open a PR from my own fork (which would be branched off of this PR), or wait till this PR is merged to open mine, or any other way to contribute the changes that makes sense with your workflow; I'm just trying to avoid forking on top of a fork! (the edits are all in the Once again, thank for getting the LiteLLM Bedrock backend working! |
|
@leothomas, I think all these changes sound good. I will check in with @nrfulton today and see if we can get this base PR merged so that your PRs can be opened. I can't test this myself. The |
|
Hey there! I pulled @nrfulton 's branch into my own forks and then added the edits mentioned above. Here's the branch if you want to check it out: https://github.com/leothomas/mellea/tree/bedrock_aws_access_key_id @jakelorocco I'll wait for y'all to merge this PR to open one from my branch! |
|
Also, here's the LiteLLM fork with LogProbs: https://github.com/leothomas/litellm/tree/feature/logprobs I still have to write tests and all that good stuff before opening a PR against LiteLLM |
…ea into bedrock_aws_access_key_id
Assisted-by: Claude Code Signed-off-by: Nathan Fulton <gitcommit@nfulton.org>
|
I changed up the auto-selected reviewers to match the reviewers assigned to #849 . FYI @ajbozarth and @jakelorocco: TL;DR: same state as #849. This is the OG PR for that work. Switched here because maintainer commit rights were disabled for the #849 branch and there were a number of failing pre-commit hooks + a merge required to proceed. I think this is good to go if tests pass. Please give a once over since there's been a fair amount of changes since this PR was first opened. The main design decisions:
FYI, all tests pass on my local machine with |
|
Thanks for following up on this and getting it over the finish line @nrfulton - again please let me know if I can support in any way! |
Conflicts: uv.lock - resolved with uv sync
| hf_model_name="openai/gpt-oss-20b", # OpenAI GPT-OSS 20B | ||
| ollama_name="gpt-oss:20b", # Ollama | ||
| bedrock_name="openai.gpt-oss-20b", | ||
| bedrock_litellm_name="bedrock/converse/openai.gpt-oss-120b-1:0", |
| ) | ||
|
|
||
|
|
||
| def _assert_bedrock_auth() -> None: |
There was a problem hiding this comment.
Should we be calling this on the OpenAI path as well? Does the OpenAI SDK work with the creds path?
| def _assert_region(region: str | None) -> None: | ||
| resolved_region = ( | ||
| region | ||
| or os.environ.get("AWS_REGION_NAME") | ||
| or os.environ.get("AWS_DEFAULT_REGION") | ||
| or os.environ.get("AWS_REGION") | ||
| ) | ||
| assert resolved_region is not None, ( | ||
| "you must specify a region: pass `region` explicitly or set AWS_REGION_NAME, AWS_DEFAULT_REGION, or AWS_REGION." | ||
| ) |
There was a problem hiding this comment.
Do the OpenAI and LiteLLM SDKs check for these same env vars?
| Use this instead of `create_bedrock_openai_backend` when you need auth with an AWS_ACCESS_KEY_ID. | ||
| """ | ||
| _assert_bedrock_auth() | ||
| _assert_region(region) |
There was a problem hiding this comment.
If region is passed in, this assertion could succeed but the region isn't passed into the actual LiteLLM Backend init.
| # TODO litellm doesn't even appear to use this...? | ||
| backend._base_url = None # type: ignore |
There was a problem hiding this comment.
Can we remove this? or add a comment why setting the _base_url to None is necessary?
|
|
||
| def create_bedrock_mantle_backend( | ||
| def create_bedrock_litellm_backend( | ||
| model_id: ModelIdentifier | str, region: str | None = None, num_retries: int = 15 |
There was a problem hiding this comment.
15 seems like a high default for retries. I did a quick search and it seems like Litellm uses exponential waits. I couldn't find a number, but that seems like this might resolve in long hangs.
| # Extract top-level Ollama params that must not be forwarded into `options`. | ||
| logprobs = model_opts.pop("logprobs", None) | ||
| top_logprobs = model_opts.pop("top_logprobs", None) | ||
| MelleaLogger.get_logger().info(f"Tools for call: {tools.keys()}") |
There was a problem hiding this comment.
This should be moved back to the indented block above.
There was a problem hiding this comment.
It might be nice to add at least one qualitative test for the litellm code path.
|
|
||
| m = MelleaSession( | ||
| backend=create_bedrock_mantle_backend( | ||
| backend=create_bedrock_openai_backend( |
There was a problem hiding this comment.
This should be the litellm path given the import above.
| [Bedrock Model Access](https://us-east-1.console.aws.amazon.com/bedrock/home#/model-access), | ||
| or pass a different `region` to `create_bedrock_mantle_backend`. | ||
| or pass a different `region` to `create_bedrock_litellm_backend`. | ||
|
|
There was a problem hiding this comment.
This got caught in the rename sweep — the troubleshooting tip points to create_bedrock_litellm_backend, but the "Model X is not supported in region" error only comes from create_bedrock_openai_backend (the one that validates via list_mantle_models). The litellm path skips that check entirely.
| or os.environ.get("AWS_REGION") | ||
| ) | ||
| assert resolved_region is not None, ( | ||
| "you must specify a region: pass `region` explicitly or set AWS_REGION_NAME, AWS_DEFAULT_REGION, or AWS_REGION." |
There was a problem hiding this comment.
assert resolved_region is not None is stripped silently under python -O. For config validation (as opposed to invariant checks) the idiomatic pattern is if resolved_region is None: raise SomeError(...). Same applies to the assert model_name != "" a few lines below.
| import os | ||
|
|
||
| import boto3 | ||
| import botocore.exceptions |
There was a problem hiding this comment.
boto3 is in the litellm optional extras, not the base mellea deps. Making it a top-level import means from mellea.backends.bedrock import create_bedrock_openai_backend — which worked fine with a plain pip install mellea before — now raises ModuleNotFoundError on a base install. Scoping the import inside _assert_bedrock_auth (where it's actually needed) would fix the regression without losing anything.
|
|
||
|
|
||
| def create_bedrock_openai_backend( | ||
| model_id: ModelIdentifier | str, region: str | None = None |
There was a problem hiding this comment.
create_bedrock_mantle_backend was the documented public entry point before this PR. Removing it without a deprecation alias is a breaking change for anyone importing it directly. A two-line shim with DeprecationWarning would cover existing users.
planetf1
left a comment
There was a problem hiding this comment.
A few things separate from the existing thread. Inline comments above cover a misnamed function in the troubleshooting section, a boto3 import regression for base-install users, assert-as-config-validation (which gets stripped under -O), and the missing deprecation alias.
One more that cannot be pinned inline — docs/docs/integrations/bedrock.md line 45 still has a stray prose reference to create_bedrock_mantle_backend that was not updated by the rename.
Misc PR
Type of PR
Description
Testing