Master nova follow ups by mujtaba1747 · Pull Request #6051 · aws/sagemaker-python-sdk

mujtaba1747 · 2026-07-17T23:45:02Z

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.

* Feat: Add show_metrics() and stream_logs() for monitoring training jobs * Feat: show_metrics() and stream_logs() --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

) * show_metrics() Enhancement: Display MLFlow metrics for OSS models * Update unit tests * Address code comments --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

* feat(serve): add opt-in model source tag-based resource reuse Add reuse_resources to ModelBuilder.build/deploy and BedrockModelBuilder.deploy. On a hit, discover an existing resource by the model-source tag and return it instead of creating a duplicate (warn, do not raise). Honored per call. - New sagemaker/serve/model_reuse.py: tag helpers + service-client discovery - Consolidate Nova manifest/checkpoint reading into sagemaker/core/training/utils.py - SageMaker: build() skips Model creation on reuse (sets built_model to the existing Model); deploy() reuses the endpoint after validating env vars/image/ instance type (PrimaryContainer with Containers[0] fallback for Nova) - Reuse gates are skipped for inference-component builds/deploys so IC create/update (via _deploy_for_ic) is never silently intercepted - Bedrock: reuse custom model + active deployment; response includes modelArn - Reuse discovery uses the cached session/bedrock clients - Support raw S3 URI model input via model_metadata BASE_MODEL_NAME - Unit tests + notebook examples * feat(serve): support Nova inference-component deployment and harden reuse Route Nova model-customization deploys through the shared single-inference -component path when a ResourceRequirements inference_config is supplied, so each Nova checkpoint (full-rank or LoRA-merged) is hosted as one inference component referencing the built Model. Nova without an inference_config keeps the direct model-on-variant path. - Broaden _is_nova_model to identify Nova from a package-less source (raw S3 checkpoint or trainer) via base_model_name, in addition to the model package recipe/hub-content name. - Set EnableNetworkIsolation on the IC endpoint config to match the built Model (always True for Nova), fixing CreateInferenceComponent rejection on mismatched network isolation. - Guard model-package-dependent logic (restricted-package path, PEFT/recipe metadata, lineage tracking) so package-less Nova checkpoints deploy cleanly. - Apply accumulated tags (including the model-source reuse tag) to endpoints created on the shared IC path so they remain discoverable. - build(reuse_resources=True) only short-circuits when the backing Model can be resolved; IC endpoints and stale/deleted configs fall through and build a real Model, preventing a None built_model on later IC deploys. - deploy() warns that reuse_resources has no effect for inference-component deployments, which manage their own reuse by component name. - Surface both the manifest.json and output.tar.gz errors when Nova checkpoint URI resolution fails, instead of masking the primary failure. Add unit tests covering the Nova IC path (routing, network isolation, IC spec) and the model-on-variant fallback. * fix(core): resolve Nova checkpoint manifest across all three output layouts Nova training jobs write their checkpoint manifest to different locations depending on the training platform: HyperPod: <output>/<job>/manifest.json Serverless: <output>/<job>/output/output/manifest.json Serverful: <output>/<job>/output/output.tar.gz (manifest inside) resolve_nova_checkpoint_uri previously only tried the serverless manifest path and the serverful tar.gz, so HyperPod jobs (manifest directly under the job directory) failed to resolve. Add build_nova_hyperpod_manifest_s3_uri and try all three layouts in turn, aggregating every failure into the raised error so the real cause is not masked by the last attempt's message. Add unit tests for the HyperPod builder and for resolution from the HyperPod and serverless layouts. * feat(serve): Model-tag reuse, IC-deploy guard, and instance_type fix Simplify reuse discovery by tagging SageMaker Models (not just endpoints) with the model-source identifier, so build(reuse_resources=True) can find and skip recreating an existing Model directly — no IC-state dependency. - Tag non-Nova Models at build time with the model-source tag (matching the Nova path's existing behavior). Both Nova and OSS Models are now discoverable by tag. - Add _find_reusable_model: build(reuse_resources=True) searches Models by source tag, skipping Model creation on a hit. Also discovers the endpoint for deploy() to reuse later. - Simplify _get_model_for_endpoint back to variant-only lookup (returns None for IC endpoints). No longer needs IC-spec resolution since the Model is found directly by tag. - _reused_endpoint_matches_config returns True for IC endpoints (can't read container config from variant; Model was already matched by tag). - deploy() with reuse_resources=True on an IC deploy logs a warning that the flag has no effect (ICs manage reuse by endpoint_name + IC name). - Fix deploy() to set self.instance_type from the caller's explicit value before calling _deploy_model_customization, preventing recipe-resolved defaults from overriding the user's intent. - Add model-source tag assertion to the existing OSS deploy integ test.

codecov · 2026-07-18T00:09:01Z

Codecov Report

❌ Patch coverage is 71.59468% with 342 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.80%. Comparing base (b8b4518) to head (7a839a3).
⚠️ Report is 25 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6051      +/-   ##
==========================================
+ Coverage   69.64%   69.80%   +0.15%     
==========================================
  Files         548      551       +3     
  Lines       65542    66607    +1065     
==========================================
+ Hits        45649    46496     +847     
- Misses      19893    20111     +218

🚀 New features to boost your workflow:

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

* feat(train): add dry_run=True to train() Add dry_run parameter to all trainers (SFT, DPO, RLVR, RLAIF). When dry_run=True: - All existing validation runs inline (IAM role, hyperparameters, recipe constraints, infrastructure availability) - Returns None without submitting a job or consuming compute - Raises with clear error message on validation failure Additionally, validate_data_path_exists() is called unconditionally (regardless of dry_run) before job submission to catch non-existent S3 paths or dataset ARNs early. Design follows nova-forge-sdk pattern: validation always runs as part of the normal code path, dry_run short-circuits before the actual TrainingJob.create API call. Changes: - data_utils.py: add validate_data_path_exists() utility (S3 + DataSet ARN) - base_trainer.py: add dry_run to abstract train(), _train_serverful_smtj(), and _train_hyperpod() - sft/dpo/rlvr/rlaif_trainer.py: add dry_run param, pass through to shared methods, short-circuit serverless path - Notebook examples added to SFT, DPO, RLVR, RLAIF notebooks - Unit tests added to existing test files - Integration test added * feat(evaluate): add dry_run=True to evaluate() Add dry_run parameter to BaseEvaluator.evaluate() and all subclasses (BenchMarkEvaluator, CustomScorerEvaluator, LLMAsJudgeEvaluator). When dry_run=True: - All existing validation runs (IAM role, model resolution, recipe, pipeline rendering) - Dataset S3 path / DataSet ARN validated via validate_data_path_exists() - Returns None without submitting a pipeline execution - Raises on validation failure Dataset validation runs unconditionally (not just during dry_run) for CustomScorerEvaluator and LLMAsJudgeEvaluator which accept user datasets. Changes: - base_evaluator.py: add dry_run to evaluate() signature - benchmark_evaluator.py: add dry_run, short-circuit before _start_execution() - custom_scorer_evaluator.py: add dry_run, validate dataset, short-circuit - llm_as_judge_evaluator.py: add dry_run, validate dataset, short-circuit - Notebook examples added to benchmark, custom_scorer, llm_as_judge notebooks * fix(dry_run): support DataSet objects, deduplicate ARN validation, expand coverage - validate_data_path_exists() now accepts Union[str, DataSet]; extracts .arn from DataSet objects for validation - Removed duplicate ARN validation logic; delegates to _validate_dataset_arn_exists() - _validate_dataset_arn_exists() warns on AccessDenied instead of raising (execution role may still have access) - Removed isinstance(..., str) guards in all trainers and evaluators so DataSet objects flow through validation - Added dry_run=True parameter to CPTTrainer.train() - Added dry_run=True parameter to ModelTrainer.train() - Integration test: valid_dataset fixture no longer re-creates on every run - Integration test: added nonexistent_dataset_arn and nonexistent_dataset_obj fixtures - Integration test: added TestDryRunServerful class (serverful compute path) - Unit test: added test_dataset_object_extracts_arn, test_dataset_object_not_found_raises * fix(dry_run): support all AWS partitions in DataSet ARN validation - Update ARN regex to aws(?:-[a-z]+)* to match aws-cn, aws-us-gov, aws-iso, aws-iso-b partitions - Use regex guard in validate_data_path_exists() for consistent matching - Add unit tests for each partition (standard, China, GovCloud, ISO, ISO-B) and invalid partition rejection

* [WIP] Job notifications setup * [WIP] Job notif update * Adding tests, dedupe logic, and example * Update ARN example values, clean up SM session definitions, rename func * Update trainers to include 'notifications' param and add arn regex check * Address PR comments, use botocore errors, return notification rule arn --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

…otification setup (#6065) * docs: Add show_metrics, stream_logs, and job notifications documentation * Update example job names * Move monitoring capabilities to model_customization --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

…ges (#6061)

#6075) * feat(evaluate): add dry_run and caller IAM permission validation to all evaluators - Add dry_run=True parameter to InspectAIEvaluator and MultiTurnRLEvaluator (Benchmark, CustomScorer, LLMAsJudge already had it) - Add verify_evaluation_caller_permissions() to validate the caller's identity has the pipeline-orchestration permissions before submitting - Define EVALUATION_CALLER_ACTIONS constant with the set of 22 IAM actions needed by whoever calls evaluator.evaluate() - Fix bug: LLMAsJudge InspectAI code path was missing the dry_run check - Wire dry_run through _get_aws_execution_context() for all evaluators - Add unit tests covering dry_run behavior across all evaluator types * change: move EVALUATION_CALLER_ACTIONS to iam_policies.py

* [Fix] Remove task-type from RLVR recipe, update RLVR image selection logic * Add check for RLVR/RFT before removing task_type * fix: prefer SMHP image over SMTJ fallback in _train_hyperpod In _train_hyperpod, try get_hyperpod_training_image first (native SMHP image) and only fall back to SMTJ image with SM-TJ->SM-HP tag replacement if the SMHP image is not available. Previously the order was inverted. --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

github-actions · 2026-07-22T21:09:43Z

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.}

Co-authored-by: Syed Jafri <syedjfr@amazon.com>

* fix: stream_logs_smhp extract training job from obj * fix: render mlflow metrics as png to handle large number of metrics * code cleanup: move io, base64 to top level imports --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

github-actions · 2026-07-27T20:27:34Z

Claude finished @mujtaba1747's task in 3m 22s —— View job

Code Review — Master nova follow ups

Overall this is a solid, well-tested PR — the new model_reuse, Nova manifest resolution, and IAM caller-validation helpers all have good unit coverage. A few findings, ranked by severity.

🔴 Correctness — `benchmark_evaluator.py` dry-run skips model resolution

In sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py, evaluate() has two if dry_run: blocks:

aws_context = self._get_aws_execution_context()

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

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached in dry_run
... # template rendering, VPC/KMS, pipeline definition

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

The first return fires before _resolve_model_artifacts(), _get_model_package_group_arn(), and template rendering. This means dry_run does not validate model resolution or template construction, which directly contradicts the method's own docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from the other evaluators, which correctly gate after resolution. Fix: remove the first if dry_run: block (line ~680) so the flow reaches the second one at line ~722.

🟡 Design — trainer constructor provisions AWS resources as a side effect

base_trainer.py:__init__ calls self._setup_notifications(notifications) when notifications is passed, which issues live AWS calls (put_rule, put_targets, get_topic_attributes) at construction time. This is not gated by dry_run — a caller who constructs a trainer with notifications and then calls train(dry_run=True) will still have created an EventBridge rule + targets. Consider deferring notification setup to train() (and skipping it under dry_run), so constructing a trainer stays side-effect-free and dry-run truly provisions nothing.

🟡 Performance — O(N) tag lookups in resource-reuse scans

model_reuse.py (find_existing_bedrock_model / find_existing_sagemaker_endpoint) and model_builder.py._find_reusable_model enumerate all resources and issue a list_tags / list_tags_for_resource call per resource. In accounts with many models/endpoints this is N+1 API calls and is throttling-prone. Not blocking, but worth a note / future optimization (e.g. resourcegroupstaggingapi:GetResources to filter by the model-source tag server-side).

🟢 Minor — exception handling by string matching

notifications.py:

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg.
delete_notification_rule checks "ResourceNotFoundException" in str(type(e).__name__).

_validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", ""). Prefer the boto ClientError error-code approach consistently — string matching on messages is locale/format-fragile.

🟢 Minor — `describe_log_streams` result ordering

cloudwatch_metrics.py._fetch_smtj_logs uses log_stream_name = streams[0]["logStreamName"] without passing orderBy/descending to describe_log_streams. If the intent is "the latest stream", the default ordering (LogStreamName) is not guaranteed to surface it. Pass orderBy="LastEventTime", descending=True if that's the intent.

When dry_run=True, _resolve_mlflow_resource_arn now lists existing apps but skips creation and waiting. All trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL) forward dry_run to MLflow resolution. 9 unit tests.

github-actions · 2026-07-28T18:14:52Z

Claude finished @mujtaba1747's task in 3m 1s —— View job

Review complete ✅

I reviewed the full diff focusing on correctness, backward-compatibility, Python best practices, security, performance, and test coverage. One correctness bug worth fixing before merge, plus a few design/robustness notes.

ℹ️ The inline-comment tool isn't available in this session, so all findings are consolidated below with file:line references (line numbers are from the PR diff).

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns too early, skipping validation

BenchMarkEvaluator.evaluate() has two if dry_run: blocks. The first fires right after _get_aws_execution_context() and before _resolve_model_artifacts() and job-name generation:

aws_context = self._get_aws_execution_context()
if dry_run:
    logger.info(...)
    return None          # <-- returns here
...
model_artifacts = self._resolve_model_artifacts(...)
...
if dry_run:              # <-- dead code, never reached
    return None

Consequences:

dry-run skips model-artifact resolution and template/config validation — the exact checks a dry-run is supposed to exercise. This contradicts the docstring and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), all of which gate dry_run after resolution/rendering.
the second block is unreachable dead code.

Fix: remove the first if dry_run: block so the flow reaches the later one after resolution.

🟡 Design — `base_trainer.py`: notifications set up as a constructor side-effect, not gated by `dry_run`

In __init__:

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

_setup_notifications → enable_notifications makes live AWS calls (put_rule, put_targets, get_topic_attributes) at construction time. This means:

constructing a trainer has network/permission side effects before any train()/dry_run call, which is surprising and hard to test.
dry_run can't suppress it because the rule is already created in __init__.

Consider deferring notification setup to train() and gating it behind the dry_run check (validate only, don't create, on dry-run).

🟡 Performance — O(N) tag lookups in `model_reuse.py` / `model_builder.py`

_find_bedrock_model_arn_by_tag / _find_sagemaker_endpoint_arn_by_tag (and _find_reusable_model in model_builder.py) enumerate all resources and issue a list_tags_for_resource / list_tags call per resource to match the source tag. On accounts with many models/endpoints this is a large number of sequential API calls per build/deploy. Prefer the Resource Groups Tagging API (resourcegroupstaggingapi:GetResources) to filter by tag server-side, or otherwise bound/short-circuit the scan.

🟢 Robustness — `notifications.py`: substring matching on exceptions instead of error codes

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions in the same file correctly uses e.response["Error"]["Code"]. String matching is fragile (message wording changes, localization). Use ClientError error codes consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

_fetch_smtj_logs takes streams[0]["logStreamName"] from describe_log_streams without passing orderBy/descending, so the "first" stream isn't guaranteed to be the latest/most-relevant. Pass orderBy="LastEventTime", descending=True (or otherwise select deterministically).

Everything else — the Nova checkpoint/manifest resolution in sagemaker-core/.../training/utils.py, the verify_evaluation_caller_permissions IAM check (correctly uses iam:SimulatePrincipalPolicy and error codes), and the dry_run threading in the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers — looks correct and is reasonably covered by tests. Nice addition of the dry-run integration tests.

#6111) * Update error message on ModelBuilder when deploying from S3 checkpoint * Update ModelBuilder to automatically find image_uri * Update import and methods

github-actions · 2026-07-28T20:44:58Z

Claude finished @zhaoqizqwang's task in 3m 25s —— View job

Code Review — PR #6051 (Master nova follow ups)

Review checklist

Reviewed the full diff. Overall this is a solid, well-tested feature set (dry_run across trainers/evaluators, Nova checkpoint resolution, resource reuse, notifications, CloudWatch metrics). One correctness bug stands out, plus a few smaller items.

🔴 `benchmark_evaluator.py` — dry_run returns too early, skips the validation it advertises

In evaluate() there are two if dry_run: blocks:

aws_context = self._get_aws_execution_context()

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

artifacts = self._resolve_model_artifacts(aws_context['region'])
...
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

The first block returns immediately after _get_aws_execution_context(), so _resolve_model_artifacts() and template/name resolution never run. That contradicts this method's own docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom scorer, LLM-as-judge, InspectAI, MultiTurnRL), which all gate dry_run after _resolve_model_artifacts. A user running evaluate(dry_run=True) on a benchmark with an unresolvable model would incorrectly get a "validation passed" result.

Fix: remove the first if dry_run: block so control reaches the second one after resolution.

Note: the unit test TestBenchMarkDryRun.test_dry_run_returns_none does not catch this — it patches _resolve_model_artifacts but never asserts it was called, and passes regardless of which block returns. Consider adding mock_resolve_artifacts.assert_called_once() (or equivalent) so the test actually pins the "validation runs before returning" contract, mirroring what the other evaluator tests exercise.

🟡 `base_trainer.py` — notifications set up in `init` make live AWS calls, not gated by dry_run

__init__ calls self._setup_notifications(notifications) at construction time, which reaches enable_notifications (put_rule / put_targets / get_topic_attributes). This means:

Constructing a trainer with notifications=... performs mutating AWS calls before train() is ever called, and independent of dry_run. That's surprising for a constructor and undercuts the dry_run promise of "no side effects."
The _setup_notifications docstring says it is "Called internally by trainer.train() after job submission," which is inconsistent with the actual call site (__init__).

Consider deferring notification setup to train() (and skipping it under dry_run=True), and fixing the docstring to match wherever it ends up.

🟢 `notifications.py` — exception handling matches on stringified messages instead of error codes

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions (same file) correctly uses e.response.get("Error", {}).get("Code", ""). String matching is brittle (message wording changes across botocore versions/regions). Prefer the error-code approach consistently.

🟢 `cloudwatch_metrics.py` — `streams[0]` is not deterministically the intended log stream

_fetch_smtj_logs does log_stream_name = streams[0]["logStreamName"] from describe_log_streams without orderBy=LastEventTime/descending=True. The first returned stream isn't guaranteed to be the relevant one. Pass an explicit ordering (or filter by expected stream prefix) so the metric extraction reads the right stream.

🟢 Performance — O(N) tag lookups in resource-reuse scans

model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and the corresponding _find_reusable_* helpers in model_builder.py enumerate all resources and issue a list_tags_for_resource / list_tags call per resource. In accounts with many models/endpoints this is an N+1 pattern with real throttling risk. Not blocking, but worth a follow-up (e.g. Resource Groups Tagging API get_resources with a tag filter, or at least bounded pagination + a logged cap).

Everything else — Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correct error-code handling, returns None when undeterminable), the dry_run threading through the other trainers, and the removed stray print() in dpo_trainer.py — looks correct and is well covered by the new tests. Nice work on the test breadth.

* fix: stream_logs_smhp extract training job from obj * fix: render mlflow metrics as png to handle large number of metrics * code cleanup: move io, base64 to top level imports * fix: add helper method to create sns topic * fix: renamed IDs for readability in SNS access policy --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

github-actions · 2026-07-28T21:03:00Z

Claude finished @mujtaba1747's task in 1m 48s —— View job

Code Review — Master nova follow ups

Read full PR diff
Verify findings against current code
Post consolidated review

ℹ️ The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references. I verified each item against the current branch — all are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns too early, skipping validation

sagemaker-train/src/sagemaker/train/evaluate/benchmark_evaluator.py:680

evaluate() still has two if dry_run: blocks. The first fires right after _get_aws_execution_context() and returns before _resolve_model_artifacts() (line 685), _get_model_package_group_arn() (688), and _render_pipeline_definition() (717):

aws_context = self._get_aws_execution_context()

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

artifacts = self._resolve_model_artifacts(aws_context['region'])   # never reached under dry_run
...
if dry_run:                       # line 722 — dead code, unreachable
    _logger.info("Dry-run validation passed. No evaluation submitted.")
    return None

This means dry_run does not exercise model resolution or template construction — the exact checks a dry-run is supposed to cover. It contradicts the method's own docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user running evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

Fix: remove the first if dry_run: block (680–682) so the flow reaches the one at 722.

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts with a mock but never asserts it was called, so it passes regardless of which block returns and won't catch a regression. Add mock_resolve.assert_called_once() (as the other evaluator tests effectively do by threading through resolution) to pin the "validation runs before returning" contract.

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

sagemaker-train/src/sagemaker/train/base_trainer.py:129-130

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

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

Merely constructing SFTTrainer(..., notifications=...) creates an EventBridge rule + targets before any train() call — surprising for a constructor and hard to test.
train(dry_run=True) can't suppress it — the rule already exists by construction, which undercuts the "no side effects" promise of dry-run.
The _setup_notifications docstring (line 524) says it's "Called internally by trainer.train() after job submission," which is inconsistent with the actual __init__ call site.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting the docstring.

🟢 Robustness — `notifications.py`: exception handling by string matching

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:140,145 and :373

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule on "ResourceNotFoundException" in str(type(e).__name__). Meanwhile _validate_notifications_permissions in the same file already does the right thing via e.response.get("Error", {}).get("Code", "") — and iam_role_resolver.py / data_utils.py elsewhere in this PR use error codes too. String matching on messages/type-names is fragile to botocore wording changes. Catch ClientError and switch on the error code consistently.

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:86

_fetch_smtj_logs takes streams[0]["logStreamName"] from describe_log_streams without passing orderBy/descending. The default ordering is LogStreamName, which isn't guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True; otherwise filter to the exact prefix match. Fine for single-stream jobs, but non-deterministic in general.

🟢 Performance — O(N) tag lookups in resource-reuse scans

sagemaker-serve/src/sagemaker/serve/model_reuse.py:154,169,178 and model_builder.py._find_reusable_model

_find_bedrock_model_arn_by_tag / _find_sagemaker_endpoint_arn_by_tag enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match is found — an N+1 pattern with throttling risk on accounts with many models/endpoints. Not blocking (opt-in feature), but a good follow-up: use resourcegroupstaggingapi:GetResources (or SageMaker Search) to filter by the model-source tag server-side, or at least log when a bounded scan cap is hit.

Everything else looks correct and well-covered: the Nova checkpoint/manifest resolution (training/utils.py), verify_evaluation_caller_permissions (correctly uses iam:SimulatePrincipalPolicy + error codes and returns None when undeterminable), the read-only IAM validation design, partition handling, deterministic notification-rule hashing, and the dry-run threading through the SFT/DPO/RLVR/RLAIF/CPT/MultiTurnRL trainers. Nice breadth on the new dry-run integration tests.

ehsu3 and others added 3 commits July 9, 2026 13:20

Feat: show_metrics() and stream_logs() helper functions (#6002)

c0aead5

* Feat: Add show_metrics() and stream_logs() for monitoring training jobs * Feat: show_metrics() and stream_logs() --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

show_metrics() Enhancement: Display MLFlow metrics for OSS models (#6013

b3152fb

) * show_metrics() Enhancement: Display MLFlow metrics for OSS models * Update unit tests * Address code comments --------- Co-authored-by: Ealynn Hsu <ealynnh@amazon.com>

mujtaba1747 temporarily deployed to auto-approve July 17, 2026 23:45 — with GitHub Actions Inactive

This was referenced Jul 18, 2026

[DO NOT MERGE] test: verify Codecov config takes effect #6054

Closed

[DO NOT MERGE] test: verify Codecov config takes effect (same-repo) #6055

Closed

fix: remove codecov after_n_builds so single-submodule PRs report coverage #6056

Merged

mujtaba1747 temporarily deployed to auto-approve July 20, 2026 17:06 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 20, 2026 17:31 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 21, 2026 15:54 — with GitHub Actions Inactive

documentation: add dry-run and resource reuse docs to existing RST pa…

58caf17

…ges (#6061)

mujtaba1747 temporarily deployed to auto-approve July 21, 2026 17:59 — with GitHub Actions Inactive

Merge branch 'master' into master-nova-follow-ups

d577e50

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 17:56 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 20:25 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 22, 2026 21:09 — with GitHub Actions Inactive

fix: stream_logs_smhp extract training job from obj (#6084)

cf9847c

Co-authored-by: Syed Jafri <syedjfr@amazon.com>

mujtaba1747 temporarily deployed to auto-approve July 23, 2026 02:13 — with GitHub Actions Inactive

Fix mlflow (oss models) metrics viz (#6102)

0df4387

* fix: stream_logs_smhp extract training job from obj * fix: render mlflow metrics as png to handle large number of metrics * code cleanup: move io, base64 to top level imports --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

mujtaba1747 temporarily deployed to auto-approve July 27, 2026 20:27 — with GitHub Actions Inactive

fix(dry_run): skip MLflow app creation during dry_run (#6100)

81ecf84

When dry_run=True, _resolve_mlflow_resource_arn now lists existing apps but skips creation and waiting. All trainers (SFT, DPO, RLVR, RLAIF, MultiTurnRL) forward dry_run to MLflow resolution. 9 unit tests.

mujtaba1747 temporarily deployed to auto-approve July 28, 2026 18:14 — with GitHub Actions Inactive

Update error message on ModelBuilder when deploying from S3 checkpoint (

f1aaedd

#6111) * Update error message on ModelBuilder when deploying from S3 checkpoint * Update ModelBuilder to automatically find image_uri * Update import and methods

zhaoqizqwang temporarily deployed to auto-approve July 28, 2026 20:44 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve July 28, 2026 21:00 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Master nova follow ups - #6051

Master nova follow ups#6051
mujtaba1747 wants to merge 15 commits into
masterfrom
master-nova-follow-ups

mujtaba1747 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 27, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

mujtaba1747 commented Jul 17, 2026

Uh oh!

codecov Bot commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — Master nova follow ups

🔴 Correctness

🟡 Design / behavior

🟢 Minor

Uh oh!

github-actions Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py dry-run skips model resolution

🟡 Design — trainer constructor provisions AWS resources as a side effect

🟡 Performance — O(N) tag lookups in resource-reuse scans

🟢 Minor — exception handling by string matching

🟢 Minor — describe_log_streams result ordering

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete ✅

🔴 Correctness — benchmark_evaluator.py: dry-run returns too early, skipping validation

🟡 Design — base_trainer.py: notifications set up as a constructor side-effect, not gated by dry_run

🟡 Performance — O(N) tag lookups in model_reuse.py / model_builder.py

🟢 Robustness — notifications.py: substring matching on exceptions instead of error codes

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — PR #6051 (Master nova follow ups)

🔴 benchmark_evaluator.py — dry_run returns too early, skips the validation it advertises

🟡 base_trainer.py — notifications set up in __init__ make live AWS calls, not gated by dry_run

🟢 notifications.py — exception handling matches on stringified messages instead of error codes

🟢 cloudwatch_metrics.py — streams[0] is not deterministically the intended log stream

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — Master nova follow ups

🔴 Correctness — benchmark_evaluator.py: dry-run returns too early, skipping validation

🟡 Design — base_trainer.py: notifications provisioned as a constructor side-effect, not gated by dry_run

🟢 Robustness — notifications.py: exception handling by string matching

🟢 Robustness — cloudwatch_metrics.py: streams[0] assumes ordering

🟢 Performance — O(N) tag lookups in resource-reuse scans

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jul 18, 2026 •

edited

Loading

github-actions Bot commented Jul 22, 2026 •

edited

Loading

github-actions Bot commented Jul 27, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py` dry-run skips model resolution

🟢 Minor — `describe_log_streams` result ordering

github-actions Bot commented Jul 28, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns too early, skipping validation

🟡 Design — `base_trainer.py`: notifications set up as a constructor side-effect, not gated by `dry_run`

🟡 Performance — O(N) tag lookups in `model_reuse.py` / `model_builder.py`

🟢 Robustness — `notifications.py`: substring matching on exceptions instead of error codes

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering

github-actions Bot commented Jul 28, 2026 •

edited

Loading

🔴 `benchmark_evaluator.py` — dry_run returns too early, skips the validation it advertises

🟡 `base_trainer.py` — notifications set up in `init` make live AWS calls, not gated by dry_run

🟢 `notifications.py` — exception handling matches on stringified messages instead of error codes

🟢 `cloudwatch_metrics.py` — `streams[0]` is not deterministically the intended log stream

github-actions Bot commented Jul 28, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns too early, skipping validation

🟡 Design — `base_trainer.py`: notifications provisioned as a constructor side-effect, not gated by `dry_run`

🟢 Robustness — `notifications.py`: exception handling by string matching

🟢 Robustness — `cloudwatch_metrics.py`: `streams[0]` assumes ordering