fix: propagate user-supplied tags to created resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources

[rohangujarathi]

Tags passed to the trainer and evaluator classes were accepted but never reached the resources they create.

• Trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL): the serverless train() path built its tag list from the JumpStart tags only and discarded self.tags. MultiTurnRLTrainer additionally computed tags and then never passed them to Job.create().
• BaseTrainer._train_serverful_smtj() did not forward tags to ModelTrainer.from_recipe().
• BaseEvaluator had no tags field at all; add one and merge it into the pipeline tags.
• MultiTurnRLEvaluator passed only the evaluation discovery tag to CreatePipeline and dropped customer tags.

The create APIs are typed List[Tag] and validated by pydantic, which coerces plain key/value dicts and accepts Tag objects, so the merged tag lists are passed through as-is. MultiTurnRLEvaluator is the exception: it calls CreatePipeline through raw boto3 and also renders tags into the pipeline definition, both of which require the capitalized Key/Value form, so its tags are normalized locally.

Testing
13 new unit tests: 2 for trainer tag merging, 2 for BaseEvaluator.tags reaching the pipeline execution, 7 for the MultiTurnRLEvaluator casing conversion (including the raw boto3 CreatePipeline payload shape), and 2 for _create_evaluation_pipeline accepting either casing.

Verified against a pristine master checkout in an identical environment: 39 pre-existing sagemaker-train failures on both sides, identical test-name sets, zero new.

Issue #, if available:

Description of changes:

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

[github-actions]

Claude finished @rohangujarathi's task in 5m 27s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

This is a solid, well-scoped fix — the intent (accept-but-drop → accept-and-forward) is clear and consistently applied, and each change is backed by a test. A few things worth a look before merge.

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

Correctness

1. Inconsistent tag key-casing between evaluator paths (worth flagging) — sagemaker-train/src/sagemaker/train/evaluate/multi_turn_rl_evaluator.py:690-691

BaseEvaluator now exposes a single tags: Optional[List[TagsDict]] field, but the two code paths that consume it require different key casing:

• BaseEvaluator._start_execution() (base_evaluator.py:963) routes tags through EvaluationPipelineExecution.start → Pipeline.create, where pydantic coerces the lowercase {"key":..., "value":...} form (the convention used everywhere else in this PR, and what _get_jumpstart_tags/add_jumpstart_model_info_tags emit).
• MultiTurnRLEvaluator._start_mtrl_execution() uses raw boto3 create_pipeline, which requires the capitalized{"Key":..., "Value":...} form. It seeds pipeline_tags with {"Key": ..., "Value": "true"} and extends with self.tags as-is (also rendered capitalized by the pipeline template at mtrl_pipeline_templates.py:133-134).

Net effect: a user who passes the documented lowercase tag form (as the new test_create_pipeline_propagates_user_tags and the SFT trainer test both do) to an MTRL evaluator will send {"key":...,"value":...} to boto3 create_pipeline, which raises a ParamValidationError (unknown key). The same tag list works for benchmark/LLMAJ eval but breaks for MTRL. Since tags is a single inherited field with no format documented on it, this is a latent footgun. Consider normalizing casing inside _start_mtrl_execution (accept either form and convert to Key/Value) so the public contract is uniform across evaluator subclasses.

Minor / non-blocking

2. _tags_as_list stringifies values — model_builder.py:26

[{"key": str(k), "value": str(v)} for k, v in tags.items()] forces str() on values. This is fine for the {key: value} mapping convenience form, but note TagsDict values may be PipelineVariable; str()-ing one would produce its repr rather than a resolved value. Not reachable on the deploy paths here (plain strings), so just calling it out.

3. Duplicate tags not de-duped — trainers (e.g. sft_trainer.py, dpo_trainer.py) and evaluators now tags.extend(self.tags or []) with no dedup against the JumpStart/discovery tags. If a user supplies a key that collides with a SageMaker-managed tag, both entries are sent. Low risk, but a dedup (last-wins on key) would be more robust.

Tests

Good coverage on the fixed paths (Nova deploy, SFT trainer merge + Tag-object acceptance, evaluation pipeline lowercase + capitalized). Gaps worth considering:

• No test exercises the MTRL evaluator boto3 create_pipeline tag path — which is exactly the path with the casing pitfall in finding Use custom user agent string at all times #1.
• _deploy_model_customization non-Nova Endpoint.create tag forwarding (model_builder.py:5722/5746) and base_trainer._train_serverful_smtjtags=self.tags (base_trainer.py:723) are changed but not directly covered.

Overall the change is correct for the majority of paths and the approach is clean; finding #1 is the one I'd resolve (or explicitly confirm the MTRL tag format expectation) before merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 6m 17s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Review changed source files and callers for correctness
• Post review feedback

---

The change is clean and consistently applied: every accept-but-drop path now forwards self.tags, and each is backed by a test. Importantly, this revision resolves the casing footgun flagged in the prior review — MultiTurnRLEvaluator now normalizes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (boto3 CreatePipeline) and _build_job_config_doc() (the rendered Tags block), so the inherited lowercase tags field now works uniformly across evaluator subclasses. Good fix, and it's directly covered by the new MTRL tests.

I verified the potentially risky spots and they hold up:

• Tag-list mutation is safe._get_jumpstart_tags() (utils.py:243) returns a fresh list literal on each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates shared/module-level state.
• Format is consistent with the surrounding convention._deploy_nova_model / _deploy_model_customization emit lowercase {"key","value"} dicts to Endpoint.create — the same form the pre-existing Nova code already used, and Endpoint.create is pydantic-validated List[Tag] that coerces it. The _tags_as_list dict-expansion for the {key: value} convenience form is correct.
• Trainer/evaluator field types (List[Tag] / List[TagsDict]) prevent the bare-mapping footgun — a user can't pass a single {k: v} dict that would get extend-ed key-by-key, because pydantic rejects it before train() runs.

Minor / non-blocking

1. No de-dup on merge. All the tags.extend(self.tags or []) sites (trainers, evaluators) and the Nova extend append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). If a user supplies a colliding key, both entries are sent. Low risk, but a last-wins dedup would be more robust. common_utils.tag_exists() already exists and could help here.

2. Casing helpers are slightly inconsistent on falsy values._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:268) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing execution.py:89-91 uses key ... or ... Key / truthiness (dropping empty values). Not wrong, just divergent behavior for empty-string values across the two evaluator paths. Worth aligning if you care about that edge.

Test coverage

Strong on the fixed paths — Nova deploy (merge + dict-form + no-tags), SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), and now the MTRL boto3 path (lowercase→capitalized, passthrough, no-tags, plus unit tests for the helper). Remaining gaps, both low priority:

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746) isn't directly exercised.
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723) isn't directly exercised.

Overall: correct, well-tested, and the one blocking concern from the prior pass is now fixed. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 2m 28s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Review changed source files and their callers
• Post review feedback

---

Clean, well-scoped fix. The accept-but-drop → accept-and-forward pattern is applied consistently across every path (trainers, evaluators, ModelBuilder deploy), and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

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

Verified correct

• Casing footgun is fixed.MultiTurnRLEvaluator now normalizes through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline, multi_turn_rl_evaluator.py:717-720) and _build_job_config_doc() (:564). The inherited lowercase tags field now works uniformly across evaluator subclasses, and both directions are tested (lowercase→capitalized, capitalized passthrough, no-tags).
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending, so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce cleanly. Trainers merge dict-form JumpStart tags ({"key","value"}) with self.tags typed List[Tag] (base_trainer.py:79). The TrainingJob.create / Endpoint.create / Pipeline.create calls are pydantic-validated List[Tag], which accepts both the dicts and Tag objects — the SFT test_train_accepts_tag_objects test confirms this.
• _tags_as_list dict-expansion for ModelBuilder's {key: value} convenience form is correct and matches the pre-existing Nova lowercase-dict convention.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators, Nova) appends user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries. Low risk; a last-wins dedup would be more robust.
2. _tags_as_list stringifies nothing but assumes JSON-serializable values — fine for the plain-string deploy paths here; just noting TagsDict values could in theory be PipelineVariable, not reachable on these paths.
3. Empty-value handling diverges._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas execution.py's pre-existing helper drops empty values by truthiness. Not wrong, just inconsistent for empty-string values across the two evaluator paths.

Test coverage

Strong on the fixed paths. Two changed lines remain unexercised (both low priority):

• Non-Nova _deploy_model_customizationEndpoint.create(tags=endpoint_tags) (model_builder.py:5722/5746).
• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:723).

Overall: correct, well-tested, and the one prior blocking concern is resolved. Nothing here blocks merge.
· fix-propagate-user-tags-to-created-resources

[github-actions]

Claude finished @rohangujarathi's task in 1m 16s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff
• Verify changed source paths against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each fixed path is backed by a test. I verified the spots that could bite and they hold up.

Note on scope: the current diff (12 files) touches trainers + evaluators only. The model_builder.py deploy paths described in the PR body and earlier review passes are not present in this changeset — if the ModelBuilder deploy(tags=...) fix is meant to be part of this PR, it appears to have been dropped. Worth confirming that's intentional.

Verified correct

• from_recipe forwarding.base_trainer._train_serverful_smtj now passes tags=self.tags, and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] (model_trainer.py:1231) — signature matches, no coercion issue.
• Casing normalization.MultiTurnRLEvaluator routes tags through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.
• No shared-state mutation._get_jumpstart_tags() (utils.py:243) returns a fresh list literal each call, and base_evaluator._start_execution reassigns tags = add_jumpstart_model_info_tags(...) before extending — so tags.extend(self.tags or []) never mutates module-level state.
• Mixed tag forms coerce. Dict-form JumpStart tags merged with self.tags (typed List[Tag]/List[TagsDict]) are passed to pydantic-validated TrainingJob.create / Pipeline.create, which accept both dicts and Tag objects — confirmed by test_train_accepts_tag_objects.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries — SageMaker's behavior on duplicate keys is undefined/last-wins. Low risk; a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys (multi_turn_rl_evaluator.py:84-91) keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths. Worth aligning if empty tag values matter.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Gaps, both low priority:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• The other trainers (DPO, RLVR, RLAIF, MultiTurnRL) get the same one-line tags.extend(...) change as SFT but rely on the SFT test as the representative case — reasonable given they're structurally identical, but MultiTurnRLTrainer additionally adds tags=tags to Job.create() (a distinct call site) with no direct test.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — just confirm the ModelBuilder changes weren't unintentionally dropped from the changeset.

[github-actions]

Claude finished @rsareddy0329's task in 2m 4s —— View job

---

PR Review: propagate user-supplied tags to created resources

• Read the PR diff (/tmp/pr.diff)
• Verify changed source paths + callers against the base repo
• Post review feedback

---

Clean, well-scoped fix. Every accept-but-drop path now forwards self.tags, and each is backed by a test. I re-verified the spots that could bite and they hold up.

Verified correct

• Job.create tag coercion (MTRL trainer).multi_turn_rl_trainer.py now merges dict-form JumpStart tags with self.tags and passes the list to Job.create. Job.create is typed tags: Optional[List[Tag]] and decorated with @Base.add_validate_call (resources.py:18192), so pydantic coerces the {"key","value"} dicts into Tag objects and serialize() maps them to the API's Key/Value form. The mixed dict + Tag list is handled correctly.
• from_recipe forwarding.base_trainer._train_serverful_smtj passes tags=self.tags and ModelTrainer.from_recipe accepts tags: Optional[List[Tag]] — signatures match, no coercion issue.
• No shared-state mutation._get_jumpstart_tags() (utils.py:244) returns a fresh list literal per call, and base_evaluator._start_execution initializes tags: List[TagsDict] = [] fresh each call (base_evaluator.py:988) before tags.extend(self.tags or []). Nothing mutates module-level state.
• Casing normalization (MTRL evaluator). Tags route through _tags_with_capitalized_keys() in both _start_mtrl_execution() (raw boto3 CreatePipeline) and _build_job_config_doc() (rendered Tags block), so the inherited lowercase tags field works uniformly across evaluator subclasses. Both directions are tested.

Minor / non-blocking

1. No de-dup on merge. Every tags.extend(self.tags or []) site (trainers, evaluators) and the MTRL pipeline_tags.extend(...) append user tags without checking for key collisions with the SageMaker-managed tags (sagemaker-sdk:jumpstart-*, SagemakerModelEvaluation). A user supplying a colliding key sends both entries; SageMaker's dedup behavior is undefined/last-wins. Low risk — a last-wins dedup would be more robust.

2. Empty-value handling diverges across evaluator paths._tags_with_capitalized_keys keeps a tag when key is not None and value is not None (so value="" survives), whereas the pre-existing helper in execution.py drops empty values by truthiness. Not wrong, just inconsistent for empty-string values between the MTRL and benchmark/LLMAJ paths.

Test coverage

Strong on the fixed paths — SFT trainer (merge + Tag-object acceptance), evaluation pipeline (lowercase + capitalized), MTRL boto3 path (lowercase→capitalized, passthrough, no-tags), plus unit tests for the helper.

Two low-priority gaps remain:

• base_trainer._train_serverful_smtjtags=self.tags → from_recipe (base_trainer.py:1212) isn't directly exercised.
• MultiTurnRLTrainer adds tags=tags to Job.create() — a distinct call site from SFT — with no direct test; the other trainers (DPO, RLVR, RLAIF) reuse the SFT test as representative, which is reasonable since they're structurally identical one-line tags.extend(...) changes.

Overall: correct and well-tested for the paths in this diff. Nothing here blocks merge — the prior blocking casing concern is resolved.
· fix-propagate-user-tags-to-created-resources