Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… by mohamedzeidan2021 · Pull Request #6217 · aws/sagemaker-python-sdk · GitHub
Skip to content

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO… - #6217

Open
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding
Open

evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MO…#6217
mohamedzeidan2021 wants to merge 4 commits into
aws:masterfrom
mohamedzeidan2021:llaj-hardcoding

Conversation

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

…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):

Warnings (degrade, do not block):

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

…DELS
dict (model -> regions) in sagemaker/train/constants.py. That list is
triplicated across clients and goes stale: when a judge model reaches end
of life it still passes client-side validation, so the eval job spins up
and only fails deep inside the in-container Bedrock CreateEvaluationJob
call, wasting compute and surfacing a poor error.
Replace it with two-step validation against authoritative sources:
- Construction: fetch the service-maintained supported-judge-models list at
s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json
and fail fast if evaluator_model is not a supported judge model.
- evaluate(): 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 mirrors the existing SimulatePrincipalPolicy caller-check
pattern (verify_evaluation_caller_permissions).
Both steps degrade gracefully instead of blocking: if a source can't be read
(missing bedrock:GetFoundationModel permission, unreadable list, or a
transient error) the SDK logs an actionable warning with a link to the
supported models and continues.
- Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py
- Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py
- Add unit tests for both validation steps and caller_can_perform
@github-actions

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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 withoutResourceArns — 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-modelResourceArns 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 everyLLMAsJudgeEvaluator(...) 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

github-actionsBot commented Aug 27, 2026

Copy link
Copy Markdown

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)
...
iferror_codein ("ResourceNotFoundException", "ValidationException"):
raiseValueError("... 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_idsS3Downloader.read_file)

Because sagemaker_session is always populated (the @validator('sagemaker_session', always=True) default-session creator in base_evaluator.py:419), everyLLMAsJudgeEvaluator(...) 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

jam-jee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

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

github-actionsBot commented Aug 28, 2026

Copy link
Copy Markdown

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") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... 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 mocksendOfLifeTime 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 (GetFoundationModelFoundationModelDetailsModelLifecycle). 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, everyLLMAsJudgeEvaluator(...) 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) andResourceNotFound 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

… and self-provisioning
The retired-model lifecycle assertion required the runner to hold
bedrock:GetFoundationModel; CI identities that lack it correctly degrade
(warn, don't block), so the hard-raise assertion failed there.
- Rename test_retired_model_fails_lifecycle_check ->
test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both
outcomes under the ambient identity (raise if permitted, warn if not).
- Refactor the restricted-role fixture into a policy-parameterized
_assumed_role_session() context manager; the positive bedrock-permission test
now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic
enforce) and the negative provisions one that lacks it (deterministic
degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole.
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job


Review: replace hardcoded _ALLOWED_EVALUATOR_MODELS with service-backed validation

  • Read the PR diff
  • Verify Bedrock GetFoundationModel returns endOfLifeTime (EOL branch reachability)
  • Examine validators, evaluate(), and helpers in context
  • Assess correctness, tests, performance
  • Post review

The two-step design (construction = "is it a judge model", evaluate() = "is it still in service") is sound and the graceful-degradation-everywhere policy is applied consistently. Prior review follow-ups landed: the caller_can_perform pre-gate is gone, GetFoundationModel is called directly, the degradation blocks are collapsed, and _fetch_supported_judge_model_ids distinguishes "unreadable" from "unexpected shape" at debug. The integ tests jam-jee asked for (JS-hub positive/negative + Bedrock permission positive/negative) are present.

One finding rises to blocker-level because it means the PR's flagship guard never fires in production. Findings below with file:line (the inline-comment tool isn't available in this run).


1. (high — confirmed) The endOfLifeTime EOL branch is dead code, and LEGACY models slip through

llm_as_judge_evaluator.py:398-408

lifecycle=details.get("modelLifecycle", {}) ...
end_of_life=lifecycle.get("endOfLifeTime") ...
ifisinstance(end_of_life, datetime) andend_of_life<=datetime.now(timezone.utc):
raiseValueError("... has reached end of life ...")

The Bedrock GetFoundationModel response's modelDetails.modelLifecycle (ModelLifecycle) exposes only a status enum — ACTIVE | LEGACY. It does not carry an endOfLifeTime timestamp. So lifecycle.get("endOfLifeTime") is always None, the isinstance(end_of_life, datetime) guard is never true, and:

  • The headline "has reached end of life" user-facing message (prominently advertised in the PR description and docstring at :345) is unreachable against real Bedrock.
  • Retirement is detected only via ResourceNotFoundException (full removal), which emits the different "is not available in region" message.
  • There is a real functional gap in between: a model that is deprecated but still resolvable returns status: "LEGACY" with a successful response, so this method returns cleanly and the SDK submits the job with no warning — exactly the "stale list → deep runtime failure" scenario the PR set out to prevent, since a LEGACY judge can be retired mid-flight or rejected by CreateEvaluationJob.

Strong corroboration inside this PR: the retired-model integ test asserts match="not available in region" (test_llmaj_model_validation.py:466, 624), not"reached end of life" — i.e. your own EOL scenario relies on ResourceNotFound, not on endOfLifeTime. The only coverage of the EOL branch is a unit test that mocksendOfLifeTime into the response (test_llm_as_judge_evaluator.py:1079-1084), which by construction cannot catch this schema mismatch.

Suggested direction — pick one and align the docstring/PR copy:

  • If LEGACY should block: check lifecycle.get("status") == "LEGACY" (that's the real signal) and reword the message to "is deprecated / scheduled for retirement".
  • If LEGACY should only warn: warn on LEGACY, keep the hard fail for ResourceNotFound, and drop the endOfLifeTime branch plus the "reached end of life" message entirely so the code and the advertised UX match.
  • Either way, add a test that feeds the actual response shape ({"modelDetails": {"modelLifecycle": {"status": "LEGACY"}}}, no endOfLifeTime) so the intended behavior is pinned to the real schema.

2. (medium — verify) ValidationException is mapped to a hard fail; inference-profile IDs would be rejected

llm_as_judge_evaluator.py:368-374

GetFoundationModel resolves foundation-model ids only. Mapping ValidationException → hard ValueError "not available in region" assumes every entry in supported-llmaj-judge-models.json is a plain FM id. If the list ever carries a cross-region inference-profile id (us.anthropic.claude-…), the call returns ValidationException and a model that passed step 1 is hard-rejected at submit time — a behavior regression vs. the old membership check. GetFoundationModel is a metadata lookup so plain FM ids are fine (the positive integ test confirms anthropic.claude-haiku-4-5-... resolves), which mitigates this, but please confirm the list is guaranteed FM-id-only. If not, treat ValidationException as warn-and-continue (it can mean "malformed identifier" rather than "retired") or resolve the profile to its base id first. (Raised in prior reviews; still open.)

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

llm_as_judge_evaluator.py:305

Since sagemaker_session is always populated (base-class always=True validator), everyLLMAsJudgeEvaluator(...) issues an S3 read against jumpstart-cache-prod-<region>. Fine functionally (degrades on failure), but it turns construction into a network/credential-dependent op — noticeable for tuning sweeps that instantiate repeatedly. An lru_cache keyed by region would avoid the re-fetch. Also: GovCloud/China partitions use different jumpstart-cache-prod-<region> bucket naming, so they always degrade and the WARNING at :308 fires on every construction there — consider debug/warn-once for the unreadable case while keeping the fail-fast ValueError as the actionable signal.

4. (low) Integ test hardcodes account 729646638167

test_llmaj_model_validation.py:376-378

DATASET_S3_URI / S3_OUTPUT_PATH / MLFLOW_ARN embed a specific account. Even as format-only fixtures this couples the test to one account and reads oddly against the repo's "no hardcoded account IDs" convention (AGENTS.md). Prefer a synthetic placeholder or derive from sts.get_caller_identity() / default_bucket(). Also, test_retired_model_* assumes claude-3-5-sonnet-20240620-v1:0 is simultaneously still in the supported list (passes step 1) and ResourceNotFound in us-west-2 (fails step 2) — the day the service list drops it, _build_evaluator(...) raises ValidationError and the test breaks confusingly. Worth a comment documenting that fragility.

5. (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 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. The one real gap feeds finding #1 — no test exercises the actualGetFoundationModel response shape (status-only, no endOfLifeTime), so the dead EOL branch and the LEGACY gap are invisible to CI.

Given the fail-open policy nothing here corrupts data, but #1 means the feature's headline capability (fail-fast on end-of-life judge models) does not actually work against Bedrock today — I'd want that resolved before merge. No prompt-injection attempts were found in the PR content.
· branch llaj-hardcoding

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mohamedzeidan2021@jam-jee