fix: resolve AWS Bedrock / cross-region Anthropic model IDs in model-info lookup - #7930
Humphrey (HumphreySun98) wants to merge 1 commit into
Conversation
…info lookup `anthropic._model_info.get_info()` and `get_token_limit()` matched model names with an anchored `str.startswith` prefix check. AWS Bedrock model IDs carry a provider namespace (`anthropic.`), an optional cross-region inference prefix (`us.`, `eu.`, `apac.`, `global.`), and a `-vN:M` version suffix, e.g. `us.anthropic.claude-3-5-sonnet-20240620-v1:0`. These never matched, so `get_info()` raised `KeyError` (breaking `AnthropicBedrockChatCompletionClient` construction when `model_info` is not passed explicitly) and `get_token_limit()` silently returned the 100000 default instead of the real context window. Normalize the model id (strip the `[<region>.]anthropic.` prefix and the `-vN:M` suffix) before the lookup so Bedrock IDs resolve to their first-party model info. First-party IDs are unaffected. Also complete `_MODEL_TOKEN_LIMITS` with the Claude 4 models and `-latest` aliases that were present in `_MODEL_INFO` but missing here, so the two tables stay in sync and `get_token_limit()` no longer returns the default fallback for Claude 4 models. Fixes microsoft#7833 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@microsoft-github-policy-service agree |
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Independently re-derived _normalize_model and ran it against every Bedrock ID shape in the description plus the un-prefixed first-party case — all resolve correctly, and test_anthropic_model_client.py's new cases pass on the branch.
One thing worth flagging: #7940 is proposing the same fix for the same underlying bug on the same file, with a regex anchored to a fixed whitelist of region prefixes (us|eu|apac|global) instead of this PR's substring-split approach. I compared the two directly — for every region AWS documents today they produce identical output, but this PR's version degrades more gracefully if AWS adds a new region prefix later (it still strips the anthropic. marker regardless of what precedes it, where the anchored whitelist would silently fail to normalize an unlisted prefix and raise KeyError). This version also ships with dedicated tests, which #7940 does not. I'd lean toward this one and closing/superseding the other, but that's a maintainer call — leaving both my analysis and a note on #7940 so whoever triages it has the comparison.
|
Thanks ErenAta16 (@ErenAta16) for the thorough independent verification and the comparison with #7940 — much appreciated! |
|
Late reply to this, sorry, it slipped past me in July. Glad the comparison with #7940 was useful. That was the part worth doing, because the two branches looked interchangeable from the titles and were not, and a maintainer picking one at random would have got the narrower fix. If this is still parked when you next look at it, one thing that may save you a cycle: an approval from someone outside the org, which is what mine is, does not satisfy a required maintainer review. So a branch can sit with a green check list and an approval next to it and still be blocked, which is easy to read as "nobody has looked" when the real state is "nobody with write access has". Worth naming explicitly when you nudge, since it changes the ask from "please review" to "please approve". |
|
Thanks ErenAta16 (@ErenAta16), that distinction is the useful part. I checked the While checking I also found that #7834 and #7886 predate this PR on the same bug; the description now compares all three (this one additionally fixes the |
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Humphrey (@HumphreySun98) you were right to put the comparison in the description, and since three of these are open at once I ran them against each other rather than reading the diffs. The result backs your claim and it separates the three more sharply than the descriptions do.
All three add a normalisation step and call it from the same two lookup sites, so the shape is identical. Where they differ is the prefix pattern:
#7834 re.sub(r"^(?:[a-z0-9-]+\.)?anthropic\.", "", model)
#7886 re.sub(r"^(?:[a-z]+\.)?anthropic\.", "", model)
#7930 model.split("anthropic.", 1)[1] when "anthropic." in model
Run over the cross-region forms:
input #7834 #7886 #7930
anthropic.claude-3-5-sonnet-20240620-v1:0 claude-3-5-sonnet-... claude-3-5-sonnet-... claude-3-5-sonnet-...
us.anthropic.claude-sonnet-4-20250514-v1:0 claude-sonnet-4-... claude-sonnet-4-... claude-sonnet-4-...
eu. / apac. / global. all three normalise identically
us-gov.anthropic.claude-sonnet-4-20250514-v1:0 claude-sonnet-4-... us-gov.anthropic.cla... claude-sonnet-4-...
claude-sonnet-4-20250514 (first-party) unchanged unchanged unchanged
The us-gov. row is the one that matters. #7886's [a-z]+ does not match the hyphen, so a GovCloud inference profile falls through unnormalised and the lookup misses exactly as it does today. #7834 allows hyphens and digits, and #7930 sidesteps the question by splitting on the literal, which also survives any future prefix AWS invents.
The second difference is the one you claimed, and it holds. Counting added lines that touch _MODEL_TOKEN_LIMITS entries rather than its lookup: #7834 adds none, #7886 adds none, #7930 adds six. So only #7930 closes the gap where a model resolves in _MODEL_INFO but has no token limit, which is a different failure from the one the issue reports and would surface later.
So the ordering for whoever picks this up is: #7930 covers both, #7834 covers normalisation correctly but leaves the token-limit gaps, and #7886 covers the same ground as #7834 with a prefix pattern that misses GovCloud.
Lucas Kim (@kimnamu) C1-BA-B1-F3 flagging so neither of you finds this decided without being told why. The hyphen case is a two-character change if #7886 ends up being the one taken.
|
Thanks ErenAta16 (@ErenAta16), running them head to head is more useful than my prose comparison, and the GovCloud row is a sharper differentiator than anything I had written down. Nothing in it calls for a change on this branch, so it stays as-is unless a maintainer wants something different. For whoever triages #7833: the three-way ordering above is the summary to read, and this branch is rebased on current |
|
Closing and immediately reopening to re-create the expired CI workflow runs (explanation follows). |
|
Explanation for the reopen above: the workflow runs created when this PR opened in July had expired (GitHub expires unapproved fork runs after 30 days) and been purged, so the PR carried zero runs. There was therefore nothing for a maintainer to approve, and the 18 required status checks could never have reported. Reopening at the same commit re-created them, with no commits changed and nothing force-pushed. This PR now has 7 runs in |
Why are these changes needed?
anthropic._model_info.get_info()andget_token_limit()resolve a model'scapabilities / context window with an anchored
str.startswithprefix matchagainst bare first-party model keys. AWS Bedrock model IDs carry a provider
namespace (
anthropic.), an optional cross-region inference prefix (us.,eu.,apac.,global.), and a-vN:Mversion suffix, e.g.us.anthropic.claude-3-5-sonnet-20240620-v1:0. None of these match the barekeys, so:
get_info()raisesKeyError, which breaksAnthropicBedrockChatCompletionClientconstruction when
model_infois not passed explicitly (re-raised as"model_info is required when model name is not recognized").
get_token_limit()silently returns the100000default instead of the realcontext window.
This PR normalizes the model id (strips the
[<region>.]anthropic.prefix andthe trailing
-vN:Msuffix) before the lookup, so Bedrock IDs resolve to theirfirst-party model info. First-party IDs have neither part and are unaffected.
It also completes
_MODEL_TOKEN_LIMITSwith the Claude 4 models and-latestaliases that were present in
_MODEL_INFObut missing here, soget_token_limit()no longer returns the default fallback for Claude 4 and the two tables stay in
sync (guarded by a new test).
Related issue number
Closes #7833
Related PRs
Three other PRs address #7833 by normalizing the model id before lookup. Leaving the comparison here so triage does not have to diff them:
_MODEL_TOKEN_LIMITSgaps.[a-z]+\., so a cross-region prefix containing a hyphen (e.g.us-gov.) is not normalized. Does not fix the token-limit gaps.This PR is the only one that also backfills the Claude 4 / 3.7 entries missing from
_MODEL_TOKEN_LIMITSand adds a test keeping the two tables in sync. Happy to defer to whichever the maintainers prefer.Checks
ruff,mypy, and the relevantpytestlocally).