fix: forwarded role to Evaluator

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.

[zhaoqizqwang]

RLVRTrainer does not forward the user-provided role to its internal Evaluator.create() call, causing role resolution failures for users whose caller identity cannot auto-resolve, and the required Lambda response format (aggregate_reward_score) is undocumented in the training interface.

[github-actions]

Claude finished @zhaoqizqwang's task in 1m 36s —— View job

---

Code Review: fix: forwarded role to Evaluator

• Read PR diff and changed file
• Verify Evaluator.create accepts role and RoleValidationError import path
• Trace role-resolution flow in train()
• Check for test coverage
• Post review

---

The core fix is correct and worthwhile: forwarding role=role into Evaluator.create(...) means the evaluator is created with the same execution role the trainer already resolved, instead of forcing Evaluator.create to re-resolve from caller identity. The added docstring documenting the required Lambda response format (id, aggregate_reward_score, optional metrics_list) is a nice improvement. The import path (sagemaker.core.helper.iam_role_resolver.RoleValidationError) is valid, and role is a supported keyword on Evaluator.create.

A few points to consider:

1. The try/except RoleValidationError block is likely unreachable (correctness)

rlvr_trainer.py:471 already resolves and validates the role before the Lambda-ARN block:

role=TrainDefaults.get_role(role=self.role, sagemaker_session=sagemaker_session)

TrainDefaults.get_role raises RoleValidationError itself when resolution/validation fails (see defaults.py:69-91), so if a role can't be resolved, execution fails at line 471 — outside your try — with the plain error, never reaching the helpful message you added at lines 505-519.

By the time Evaluator.create(..., role=role, ...) runs, role is an already-validated, non-None ARN. Internally Evaluator.create calls TrainDefaults.get_role(role=role, ...) again (evaluator.py:246) with that explicit, valid role, which will pass validation. So the except RoleValidationError branch effectively won't fire in practice.

If the goal is to surface the "pass role or pre-create the Evaluator" guidance to users, the try/except should wrap the resolution at line 471 (or self.role should be forwarded and resolution deferred to Evaluator.create). As written, the block adds ~14 lines that appear to be dead code. Worth confirming whether this is intended, or trimming it.

2. No test coverage for the changed path (testing)

tests/unit/train/test_rlvr_trainer.py doesn't currently exercise the Lambda-ARN → Evaluator.create branch at all. Since this PR fixes a behavioral bug (role not forwarded), a unit test that mocks Evaluator.create and asserts it's called with role=<resolved-role> would lock in the fix and prevent regression. If the try/except is kept, a test for the re-raised error message would be worthwhile too.

Summary

The one-line substantive fix (adding role=role) is correct — I'd recommend adding a test for it. The main thing to resolve is whether the RoleValidationError try/except is reachable given role is already resolved and validated earlier in train(); if not, either move the handling to the actual failure point or drop it.

Note: the inline-comment tool wasn't available in this environment, so findings are consolidated here rather than posted inline.

[rsareddy0329]

Can we include unit tests as well.

[jam-jee]

No test coverage for the changed path (testing)

Can we add relevant tests.