evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…

[mohamedzeidan2021]

…DELS

LLMAsJudgeEvaluator.evaluator_model was validated against a hardcoded _ALLOWED_EVALUATOR_MODELS dict (model → regions) in sagemaker/train/constants.py. This PR replaces that with two-step validation against authoritative, service-maintained sources, and removes the dict.

Problem

1. Maintenance toil. Every Bedrock judge-model add/deprecation required hand-editing the dict and cutting an SDK release — and the same list is triplicated across the SDK, Studio UI, and SageMaker Agent Skills.
2. Stale list → deep runtime failures. When a judge model reaches end of life the dict still lists it, so it passes client-side validation, the eval job spins up, and only the in-container Bedrock CreateEvaluationJob call fails ("model version has reached end of life"). Compute is wasted and the error surfaces deep inside a running job instead of failing fast.

This is real today: the supported list still advertises claude-3-5-sonnet-20240620, claude-3-5-haiku, claude-3-5-sonnet-v2, and claude-3-7-sonnet, all of which return ResourceNotFoundException from Bedrock in us-west-2.

Solution — two-step validation

Step 1 — construction (is it a judge-capable model?): fetch the service-maintained list at s3://jumpstart-cache-prod-/fmhMetadata/supported-llmaj-judge-models.json (source of truth for supported judge models) and fail fast if evaluator_model isn't in it.

Step 2 — evaluate() (is it still in service?): the supported list is a superset that can still include end-of-life models, so we call bedrock:GetFoundationModel and fail fast if the model is unavailable in the region or past its endOfLifeTime. The lookup is gated on the caller's IAM permission via a new non-raising caller_can_perform() helper that reuses the existing iam:SimulatePrincipalPolicy caller-check pattern (verify_evaluation_caller_permissions).

Graceful degradation everywhere: we never block on "can't tell." If a source can't be read (missing permission, unreadable list, transient error), the SDK logs an actionable warning with a link to the supported models and continues.

User-facing messaging

Fail-fast errors (block construction / submission):

• Not a supported judge model (Step 1):
  ▎ evaluator_model '' is not a supported LLM-as-Judge model in region ''. Choose one of the supported judge models. See https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported
• Past end-of-life (Step 2):
  ▎ evaluator_model '' has reached end of life in region '' (end-of-life ) and can no longer be used as a judge. Choose a judge model that is in service. See https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported
• Unavailable in region (Step 2):
  ▎ evaluator_model '' is not available in region ''. It may be unsupported in this region or have reached end of life. Choose a judge model that is in service in this region. See https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported

Warnings (degrade, do not block):

• Caller's role is missing the permission (Step 2):
  ▎ Your IAM role does not include the bedrock:GetFoundationModel permission, so the SDK can't check whether the evaluator model '' is still in service or has reached end of life. The evaluation will still run, but it may fail if this model has been retired. Add bedrock:GetFoundationModel to your role to enable this check. See the list of supported judge models: https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported
• Permission can't be confirmed — identity isn't a role, or iam:SimulatePrincipalPolicy unavailable (Step 2):
  ▎ The SDK couldn't confirm your IAM role includes the bedrock:GetFoundationModel permission, so it can't check whether the evaluator model '' is still in service or has reached end of life. The evaluation will still run, but it may fail if this model has been retired. See the list of supported judge models: https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported
• Transient Bedrock error while checking lifecycle (Step 2):
  ▎ The SDK couldn't verify whether the evaluator model '' is still in service right now (a temporary error occurred). The evaluation will still run, but it may fail if this model has been retired. See the list of supported judge models: https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported
• Supported-models list unreadable (Step 1):
  ▎ The SDK couldn't retrieve the list of supported judge models, so it can't confirm '' is a valid judge model. The evaluation will still run, but it may fail if the model isn't supported. See the list of supported judge models: https://docs.aws.amazon.com/bedrock/latest/userguide/evaluation-judge.html#evaluation-judge-supported

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Claude finished @mohamedzeidan2021's task in 3m 13s —— View job

---

Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

Thanks for tackling the stale-list problem — the two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound, the graceful-degradation-everywhere policy is applied consistently, the triplicated dict removal is clean, and the new paths are well tested. A few things worth a look before merge.

Note: the inline-comment tool isn't available to me in this run, so findings are below with file:line references instead of anchored review comments.

---

1. (medium) The bedrock:GetFoundationModel permission gate can silently skip the EOL check for callers who do hold the permission

sagemaker-train/.../llm_as_judge_evaluator.py:345

allowed = caller_can_perform(["bedrock:GetFoundationModel"], self.sagemaker_session)

bedrock:GetFoundationModel is a resource-scoped action, and caller_can_perform simulates it without ResourceArns — its own docstring warns that "resource-scoped actions can come back implicitDeny even for a caller who holds them" (iam_role_resolver.py:772-775). So any caller whose grant is scoped to specific foundation-model ARNs (rather than * / AmazonBedrockFullAccess) gets allowed is False → the SDK warns and skips the exact EOL check this PR adds, silently defeating step 2 for the security-conscious users most likely to scope their policies.

Meanwhile, a genuine lack of permission is already handled downstream: get_foundation_model would raise AccessDeniedException, which falls through the ResourceNotFoundException/ValidationException branch into the warn-and-continue path at lines 388-397. So the gate mostly buys false negatives plus 3 extra STS/IAM calls (GetCallerIdentity, GetRole, SimulatePrincipalPolicy) per evaluate().

Suggestion: drop the pre-gate and call get_foundation_model directly, mapping AccessDenied* to the "couldn't confirm permission / can't verify" warning. Same UX, no false negatives, fewer API calls. If you keep the gate, simulate with the proper foundation-model ResourceArns so scoped policies resolve correctly.

2. (medium) Construction now performs synchronous S3 network I/O inside a pydantic validator

sagemaker-train/.../llm_as_judge_evaluator.py:292

_validate_evaluator_model does an S3 GetObject against jumpstart-cache-prod-<region> on every LLMAsJudgeEvaluator(...) construction. This changes construction from a cheap in-memory op into a network- and credential-dependent one:

• adds latency to every instantiation;
• other unit tests that construct the evaluator without patching S3Downloader.read_file will now attempt a real network call each time (they'll degrade with a warning, but slow/flaky in CI);
• step-1 validation only runs when sagemaker_session is explicitly passed — if the session is resolved lazily (e.g. via TrainDefaults.get_sagemaker_session), values.get('sagemaker_session') is None and the check always degrades silently.

Consider caching the per-region list (module-level or lru) and/or documenting that construction now touches S3.

3. (low) Step-1 relies on an assumed external JSON schema, failing open if it's wrong

llm_as_judge_evaluator.py:247-262

_fetch_supported_judge_model_ids assumes {"supported_judge_models": [{"model_id": "..."}]}. If the real file's shape ever differs, the parse yields None → warn → continue, so the SDK silently stops validating any model while appearing to. That's the safe failure direction, but given the whole PR is motivated by a stale/wrong list, a brief note (or a log at debug distinguishing "unreadable" from "unexpected shape") would help future debugging.

4. (nit) Duplicated degradation block

llm_as_judge_evaluator.py:388-397 and 398-407 are byte-for-byte identical warning bodies (except ClientError non-matching branch vs except Exception). Both are reachable, just collapsible. Harmless.

---

Nothing here is a hard blocker — the degradation policy means everything fails open. Finding #1 is the one I'd most want addressed, since it can quietly neutralize the new EOL guard for scoped-permission callers. Tests look thorough for the paths that exist; the main untested gap is that evaluate() actually invokes _check_evaluator_model_lifecycle, but that's minor.
· branch llaj-hardcoding

[github-actions]

Claude finished @mohamedzeidan2021's task in 3m 28s —— View job

---

Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

• Read the PR diff (/tmp/pr.diff)
• Examine validators, evaluate(), and helpers in context
• Confirm which prior review findings were addressed
• Assess correctness, tests, performance
• Post review

Note: the inline-comment tool isn't available in this run, so findings are below with file:line references.

The design is sound and the two-step split (construction = "is it a judge model", evaluate() = "is it still in service") is applied consistently, with graceful degradation throughout. The prior review's main concerns look resolved in the latest commit: the caller_can_perform pre-gate is gone and get_foundation_model is now called directly (prior #1), the two degradation blocks are collapsed into a single except Exception with distinct messages (prior #4), and _fetch_supported_judge_model_ids now logs debug distinguishing "unreadable" from "unexpected shape" (prior #3). A few things remain worth a look.

---

1. (medium) get_foundation_model doesn't accept cross-region inference-profile IDs — step 2 can falsely reject a valid judge model

llm_as_judge_evaluator.py:361 / :368

response = client.get_foundation_model(modelIdentifier=self.evaluator_model)
...
if error_code in ("ResourceNotFoundException", "ValidationException"):
    raise ValueError("... is not available in region ...")

bedrock:GetFoundationModel resolves foundation-model IDs/ARNs only. If evaluator_model is ever an inference-profile-style ID (e.g. us.anthropic.claude-..., which some newer models require for on-demand access in a region), Bedrock returns ValidationException/ResourceNotFoundException, and this code maps that to a hard fail-fast "not available in region" — blocking a model that is actually valid. This is a behavior change from the old dict membership check.

Worth confirming: are all entries in supported-llmaj-judge-models.json guaranteed to be plain FM ids (never profile ids)? If not, step 2 should resolve the profile to its base model id before calling get_foundation_model, or treat ValidationException more conservatively (warn rather than block), since a ValidationException can mean "malformed/unsupported identifier" rather than "retired model".

2. (medium) Construction now performs synchronous S3 GetObject inside a pydantic validator

llm_as_judge_evaluator.py:305 (_fetch_supported_judge_model_ids → S3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), every LLMAsJudgeEvaluator(...) now issues an S3 read against jumpstart-cache-prod-<region>. Construction went from a cheap in-memory op to a network- and credential-dependent one on the hot path. Consider a module-level / lru_cache per-region cache of the fetched set so repeated constructions (and tuning sweeps) don't re-fetch, and/or document that construction now touches S3.

3. (low) WARNING-level noise on every degrade

llm_as_judge_evaluator.py:308

The "couldn't retrieve the list of supported judge models" path logs at WARNING on every construction where the list can't be read — which includes any caller lacking read access to the JumpStart cache bucket, and GovCloud/China partitions where the jumpstart-cache-prod-<region> bucket naming differs (so it always degrades there). For those users this fires on every instantiation. Consider debug (or warn-once) for the unreadable-list case to avoid steady-state noise, keeping the fail-fast ValueError as the actionable signal.

4. (nit) Stale comment at the evaluate() call site

llm_as_judge_evaluator.py:1016

# ... Gated on caller permissions — warns and continues if it can't be verified. — the pre-gate was removed; step 2 no longer gates on a permission simulation, it calls Bedrock directly and degrades on AccessDenied. Reword to avoid implying a pre-check that no longer exists (the method docstring at :336-341 already describes the new behavior correctly).

---

Tests: thorough for the new paths — supported/unsupported/degrade routes for step 1, and active / future-EOL / past-EOL / not-found / access-denied / transient for step 2, plus a sentinel test that evaluate() invokes the check. Minor gap: no end-to-end test that a past-EOL ValueError actually propagates out of evaluate() (the invocation test patches the method with a sentinel), but that's low priority.

Nothing here is a hard blocker given the fail-open policy; #1 is the one I'd most want confirmed, since it can turn a valid judge model into a hard construction/submission failure.
· branch llaj-hardcoding

[jam-jee]

Overall changes LGTM , thanks.

• Can we pls also add integ tests (positive and negative) for validating a model against JS hub. (No need to add separate integ tests for E2E eval jobs.)
• Positive and negative integ tests for bedrock role.

[github-actions]

Claude finished @mohamedzeidan2021's task in 3m 31s —— View job

---

Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

• Read PR diff
• Examine changed source in context (validators, evaluate(), helpers)
• Assess correctness, tests, performance, prior-review follow-ups
• Post review findings

The two-step design is sound and the graceful-degradation policy is applied consistently. I confirmed the plumbing: S3Downloader.read_file(s3_uri=…, sagemaker_session=…) matches the signature at sagemaker-core/.../s3/client.py:166, _get_boto_session exists at iam_role_resolver.py:241, and field ordering is fine (base-class sagemaker_session has an always=True validator at base_evaluator.py:419, so it's populated in values before the subclass evaluator_model validator runs). The integ tests jam-jee asked for are present (positive/negative for the JS-hub list check + Bedrock lifecycle). A few things worth a look before merge.

Note: the inline-comment tool isn't available in this run, so findings are below with file:line references.

---

1. (medium — please verify) The endOfLifeTime EOL branch may be dead code — Bedrock GetFoundationModel may not return that field

llm_as_judge_evaluator.py:398-408

lifecycle = details.get("modelLifecycle", {}) ...
end_of_life = lifecycle.get("endOfLifeTime") ...
if isinstance(end_of_life, datetime) and end_of_life <= datetime.now(timezone.utc):
    raise ValueError("... has reached end of life ...")

As far as I can tell, the Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum (ACTIVE / LEGACY) — it does not carry an endOfLifeTime timestamp. If that's correct, this branch never fires against real Bedrock, the headline "has reached end of life" user-facing message is unreachable, and retirement is detected only via the ResourceNotFoundException path (which emits the different "is not available in region" message).

Strong internal corroboration: your own retired-model integ test (test_llmaj_model_validation.py:436) asserts match="not available in region", not "reached end of life" — i.e. the test for the exact stale/EOL scenario this PR targets relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocks endOfLifeTime into the response (:869), which can't catch a schema mismatch.

I couldn't run tooling in this sandbox to confirm the botocore shape — please verify against the installed botocore (GetFoundationModel → FoundationModelDetails → ModelLifecycle). If endOfLifeTime isn't a real field, either drop the branch (and reword the docstring/PR description, which prominently advertise the EOL message) or source the timestamp from wherever it actually lives.

2. (medium) Inference-profile IDs are hard-rejected by step 2

llm_as_judge_evaluator.py:361,368-374

bedrock:GetFoundationModel resolves foundation-model ids only. If any entry in supported-llmaj-judge-models.json is (or becomes) a cross-region inference-profile id (e.g. us.anthropic.claude-…, which some newer models require for on-demand use), GetFoundationModel returns ValidationException/ResourceNotFoundException, which this code maps to a hard fail-fast "not available in region" — blocking a model that passed step 1 and is actually valid. Worth confirming all list entries are guaranteed to be plain FM ids; if not, resolve the profile to its base id before the call, or treat ValidationException as warn-and-continue (a ValidationException can mean "malformed identifier" rather than "retired"). (Flagged in the prior review; still open.)

3. (low) Construction still performs synchronous S3 GetObject on the hot path

llm_as_judge_evaluator.py:305

Because sagemaker_session is always populated, every LLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. This is fine functionally (degrades on failure) but turns construction into a network/credential-dependent op, which matters for tuning sweeps that instantiate repeatedly. Consider an lru_cache keyed by region so repeated constructions don't re-fetch. Also note GovCloud/China partitions where the jumpstart-cache-prod-<region> bucket naming differs will always degrade — the WARNING at :308 then fires on every construction; debug (or warn-once) would cut steady-state noise while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test brittleness / hardcoded account

test_llmaj_model_validation.py:363-365, 375, 436

• DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN hardcode account 729646638167. Even though they're described as format-only, this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention — prefer a fixture/default_bucket() or a clearly-synthetic placeholder.
• test_retired_model_fails_lifecycle_check assumes claude-3-5-sonnet-20240620 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2). The moment the service list drops it, construction raises ValidationError at _build_evaluator(...) and the test breaks in a confusing way. Consider asserting the two steps more independently or documenting the assumption's fragility.

5. (nit) Stale comment at the evaluate() call site

llm_as_judge_evaluator.py (the block above self._check_evaluator_model_lifecycle(region))

# ... Gated on caller permissions — warns and continues if it can't be verified. — the permission pre-gate was removed; step 2 now calls Bedrock directly and degrades on AccessDenied. Reword to avoid implying a pre-check that no longer exists (the method docstring at :336-341 already describes the current behavior).

---

Tests: thorough for the paths that exist (step-1 supported/unsupported/degrade routes; step-2 active/future-EOL/past-EOL/not-found/access-denied/transient; a sentinel test that evaluate() invokes the check). The main gap is the one feeding finding #1 — no coverage that exercises the real API shape for the EOL branch.

Nothing here is a hard blocker given the fail-open policy. #1 is the one I'd most want confirmed, since it may mean the PR's flagship EOL guard never actually triggers in production. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding