Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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" + '
fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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('^' + ".*" + ' fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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('^' + ".*" + ' fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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" + ' fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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('^' + ".*" + ' fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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('^' + ".*" + ' fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@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); } })(); })(); fix(core): resolve default training role from sagemaker config by jam-jee · Pull Request #6228 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix(core): resolve default training role from sagemaker config - #6228

Open
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user
Open

fix(core): resolve default training role from sagemaker config#6228
jam-jee wants to merge 1 commit into
aws:masterfrom
jam-jee:fix/resolve-role-iam-user

Conversation

@jam-jee

Copy link
Copy Markdown
Collaborator

Problem

When a trainer (e.g. RLVRTrainer, SFTTrainer) is used without an explicit
role=, and the caller authenticates as an IAM user (or the account root)
rather than an assumed role, the SDK fails with:

No IAM role could be resolved from your caller identity for 'training' workloads
(you may be running as an IAM user or root).

The user must then pass role=ROLE_ARN explicitly on every call, even when they
have already configured a default execution role in the SageMaker config.

Why it matters

Running the SDK under long-term IAM-user credentials (access key + secret) on a
laptop, CI runner, or on-prem host is common outside SageMaker Studio/notebooks.
Every such caller is forced to hand-thread a role ARN into each trainer call, and
the SageMaker intelligent-defaults config (SageMaker.TrainingJob.RoleArn) that
exists precisely to hold that default was silently ignored on this path, unlike
processing and model monitor which already honor it.

Fix (symptom -> root cause -> change)

  • Symptom: IAM-user callers hit "No IAM role could be resolved" despite a
    configured default role.
  • Root cause: resolve_and_validate_role jumped straight from "no explicit role"
    to caller-identity inference (_resolve_caller_role_arn), which returns None
    for a user/root ARN, and then raised. It never consulted the config default.
  • Change: when no role= is provided, resolve the role type's config default
    (SageMaker.TrainingJob.RoleArn for training, FeatureGroup.RoleArn for
    feature_store) via resolve_value_from_configbefore falling back to
    caller-identity inference. The resolved config role goes through the same
    read-only permission/trust validation as any other role.

New resolution order: explicit role= -> config default -> caller identity -> raise.
Config reads are defensive (any failure is swallowed and only a concrete string
ARN is trusted), so the change cannot break existing callers. Behavior for a bare
IAM user with no configured default is unchanged: it still raises the same
error.

Tests

Added to sagemaker-core/tests/unit/helper/test_iam_role_resolver.py:

  • test_config_default_role_used_when_caller_is_iam_user - an IAM-user caller with
    a configured default training role resolves to it instead of failing.
  • test_config_default_role_takes_precedence_over_caller_role - a configured
    default wins over the caller's own backing role.
  • test_iam_user_without_config_default_still_raises - locks in the unchanged
    behavior when no default is configured.

Manual verification

N/A - unit coverage exercises all three branches (config hit, precedence, and the
preserved raise). Full resolver suite: 76 passed. sagemaker-train unit suite:
147 passed, 1 skipped.

Screenshots

N/A - no user-visible UI change.

Training (and feature_store) role resolution only inferred a role from
the caller identity, so a caller authenticating as an IAM user or the
account root - whose identity has no backing role - hit "No IAM role
could be resolved from your caller identity" and was forced to pass
role= on every call, even when a default execution role was configured
in the SageMaker intelligent-defaults config.
resolve_and_validate_role now consults the config default
(SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling
back to caller-identity inference, matching the pattern already used by
processing and model monitor. Resolution order is now: explicit role ->
config default -> caller identity -> raise. Behavior for a bare IAM user
with no configured default is unchanged (still raises the same error).
---
X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback
X-AI-Tool: Kiro
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:45 — with GitHub Actions Active
@jam-jee
jam-jeedeployed to auto-approve August 31, 2026 20:46 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @jam-jee's task in 1m 45s —— View job


Review: resolve default training role from SageMaker config

I read the full diff, the surrounding iam_role_resolver.py, the config constants, resolve_value_from_config, and the training caller path. Overall this is a clean, well-scoped, and well-tested change that fills a real gap (config default was honored by processing/monitor but not this path). The new resolution order — explicit role= → config default → caller identity → raise — matches the SDK-wide direct_input > config > default precedence convention, so it's the right ordering.

A few observations, none blocking:

1. Behavior change for assumed-role callers (medium — worth documenting, not a bug)

iam_role_resolver.py:627-629 — the config default now wins over the caller's own backing role, even for assumed-role callers (SageMaker Studio / notebooks / EC2), not just IAM-user/root callers. test_config_default_role_takes_precedence_over_caller_role confirms this is intentional, and it's consistent with SDK config-precedence conventions. But it means a Studio/notebook user who already relies on caller-identity resolution and happens to have SageMaker.TrainingJob.RoleArn set in a shared config will now silently switch to the config role.

That is arguably correct, but the PR body's claim that the change "cannot break existing callers" is a bit overstated — it changes the resolved role for that (probably small) population. Consider softening the wording and/or calling out the precedence in the user-facing docstring so the switch isn't surprising.

2. Config isn't consulted when sagemaker_session is None (low — scope gap)

_resolve_config_default_role passes the raw sagemaker_session straight through to resolve_value_from_config. When it's None, get_sagemaker_config_value has no sagemaker_config to read (common_utils.py:1292-1298) and returns None — so a config-file default is not picked up. Meanwhile resolve_and_validate_role still builds a fallback Session() via _get_boto_session for the boto/IAM clients. So a direct caller of resolve_and_validate_role(None, "training") with no session won't get the config default even though a default Session could load it.

This does not affect the primary training path — TrainDefaults.get_role always injects a real Session() (defaults.py:81-88) — which is why the feature works end-to-end. But for consistency you could resolve the session once (_get_boto_session-style) and pass it to the config lookup too. Low priority given the main entry point is covered.

3. A bad config default now raises instead of falling back (low — arguably correct)

If SageMaker.TrainingJob.RoleArn points at a non-existent role, _resolve_explicit_role (iam_role_resolver.py:629) raises ValueError rather than falling through to caller-identity inference. This is reasonable — a misconfigured explicit default should surface loudly rather than being silently ignored — just noting the changed failure mode isn't covered by a test.

Things that are done well

  • Defensive config reads (broad except + "only trust a concrete string ARN") mean the path degrades gracefully to the old behavior — good.
  • The resolved config role goes through the same read-only permission/trust validation as any other role. No new IAM writes; consistent with the module's read-only contract.
  • Lazy imports match the module's existing style and sidestep circular-import risk.
  • Tests correctly patch sagemaker.core.common_utils.resolve_value_from_config; because the import is lazy inside the function, the patch is actually exercised. The three cases (config hit, precedence over caller role, preserved raise) cover the meaningful branches.

No security or correctness bugs found. No prompt-injection attempts in the PR content.

(Note: inline PR comments aren't available in this environment, so findings are consolidated here.)
· branch fix/resolve-role-iam-user

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.

1 participant

@jam-jee