Master nova follow ups

[mujtaba1747]

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.

[codecov]

Codecov Report

❌ Patch coverage is 92.58410% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (3024a57) to head (58caf17).
⚠️ Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
...rain/tests/integ/train/test_dry_run_integration.py	0.00%	94 Missing ⚠️
sagemaker-core/tests/unit/test_training_utils.py	99.15%	1 Missing ⚠️
...unit/train/common_utils/test_cloudwatch_metrics.py	99.35%	1 Missing ⚠️
...ests/unit/train/common_utils/test_notifications.py	99.28%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6051      +/-   ##
==========================================
+ Coverage   90.26%   90.29%   +0.03%     
==========================================
  Files         479      484       +5     
  Lines       72286    73590    +1304     
==========================================
+ Hits        65246    66451    +1205     
- Misses       7040     7139      +99     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Claude finished @mujtaba1747's task in 5m 15s —— View job

---

PR Review — Master nova follow ups

I reviewed the new modules (model_reuse.py, cloudwatch_metrics.py, notifications.py, training/utils.py, iam_role_resolver.py) and the dry_run / notifications / resource-reuse wiring across trainers, evaluators, and ModelBuilder. Overall this is solid, well-documented, defensively-coded work with good test coverage. A few issues worth addressing:

---

🔴 Correctness

1. benchmark_evaluator.py — dead dry_run block + under-validation
sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

aws_context = self._get_aws_execution_context()

if dry_run:                       # <-- returns here
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

artifacts = self._resolve_model_artifacts(aws_context['region'])
...                               # model pkg group resolution, template render
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

if dry_run:                       # <-- line 722: unreachable dead code
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

The early return at line 680 means the second check at line 722 is unreachable. More importantly, it exits before _resolve_model_artifacts(), _get_model_package_group_arn(), and _render_pipeline_definition() — so dry_run skips exactly the model-resolution and template validation its own docstring promises ("runs all validation (IAM, model resolution, data paths)"). Every other evaluator (llm_as_judge, inspect_ai, custom_scorer) places the dry_run gate after those steps. The first check should be removed so the flow reaches line 722. Fix this →

Worth confirming the unit tests actually assert this — test_dry_run_returns_none mocks _resolve_model_artifacts, so it would pass regardless of which return fires and wouldn't catch the regression.

---

🟡 Design / behavior

2. Notifications are created as a constructor side-effect, bypassing dry_run
sagemaker-train/src/sagemaker/train/base_trainer.py:128-130

if notifications:
    self.notification_rule_arn = self._setup_notifications(notifications)

_setup_notifications makes live AWS calls (put_rule, put_targets, get_topic_attributes) at __init__ time. Two consequences:

• Merely constructing SFTTrainer(..., notifications=...) mutates the account (creates an EventBridge rule) before any train() call — surprising for a constructor.
• train(dry_run=True) still leaves that rule behind, which contradicts the "no side effects" expectation of a dry run.

Consider deferring rule creation to train() (and skipping it when dry_run=True), or at minimum documenting that the constructor provisions AWS resources.

3. find_existing_bedrock_model / _find_reusable_model — O(N) tag lookups
sagemaker-serve/src/sagemaker/serve/model_reuse.py:154 and model_builder.py:2001

Both enumerate all models/endpoints and issue a list_tags/list_tags_for_resource call per resource until a match is found. In accounts with many models this is a lot of sequential API calls and a throttling risk. If the backing services support resourcegroupstaggingapi:GetResourcesByTag (or SageMaker Search with tag filters), that would collapse this to one call. Not blocking given the feature is opt-in, but worth a note.

---

🟢 Minor

4. notifications.py — string-matching on exceptions
sagemaker-train/src/sagemaker/train/common_utils/notifications.py:139-149, 294

_validate_sns_topic and delete_notification_rule branch on "NotFound" in str(e) / "AccessDenied" in error_msg rather than the boto ClientError error code (e.response["Error"]["Code"]). Elsewhere in this PR (e.g. iam_role_resolver.py, data_utils.py) you correctly use error codes — string matching is more fragile to wording changes. Consider catching ClientError and switching on the code, consistent with the rest of the PR.

5. cloudwatch_metrics.py — streams[0] assumes stream ordering
sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:86

_fetch_smtj_logs picks streams[0] from describe_log_streams without an explicit orderBy/descending, so it relies on the default ordering returning the intended stream. For a single-stream job this is fine, but an explicit orderBy="LogStreamName" (or filtering to the exact prefix match) would make it deterministic.

---

Everything else — the read-only IAM validation design (resolve_and_validate_role warning rather than blocking when it can't self-simulate; the *-resource smoke-test gating to avoid false denials), partition handling, the deterministic notification rule hashing, and the Nova checkpoint multi-layout fallback — looks careful and correct.

_{Note: I couldn't compute the exact origin/master...HEAD diff (shallow checkout, no merge base), so this review is based on reading the changed files directly.}