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.23209% with 502 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.59%. Comparing base (b8b4518) to head (a27fbb7).
⚠️ Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6051      +/-   ##
==========================================
- Coverage   69.64%   69.59%   -0.05%     
==========================================
  Files         548      552       +4     
  Lines       65542    67117    +1575     
==========================================
+ Hits        45649    46713    +1064     
- Misses      19893    20404     +511

🚀 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-31T23:00:49Z

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

Code Review — Master nova follow ups

Read changed files & verify prior findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

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

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:132-134

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.
The _setup_notifications docstring says it's "Called internally by trainer.train() after job submission," 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:139-149 and delete_notification_rule

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule matches on the exception type 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

_fetch_smtj_logs calls describe_log_streams (with only logStreamNamePrefix) and then takes streams[0]["logStreamName"]. Without an explicit 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, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* fix: usuability update from feedback * update documentation

github-actions · 2026-08-04T21:29:56Z

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

Code Review — Master nova follow ups

Read changed files & verify prior findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

ℹ️ Findings are consolidated below with file:line references. I re-verified each against the current branch — the 🔴 bug and all notes are still present, unchanged from prior reviews.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:132-134

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.

Consider deferring notification setup to train() (skipping under dry_run=True) and correcting any docstring that says it's called from train().

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

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:333 (delete_notification_rule) and _validate_sns_topic

delete_notification_rule branches on "ResourceNotFoundException" in str(type(e).__name__) / "does not exist" in str(e).lower(), and _validate_sns_topic matches on message substrings. Meanwhile _validate_notifications_permissions in the same file (line 154) already does the right thing via e.response.get("Error", {}).get("Code", "") — and log_streamer.py (lines 123, 285) in this same PR uses 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:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix (line 108) and then takes streams[0]["logStreamName"] (line 122). Without an explicit orderBy/descending, the default ordering is LogStreamName, not 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, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py (_find_bedrock_model_arn_by_tag, _find_sagemaker_endpoint_arn_by_tag) and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* fix: usuability update from feedback * update documentation * docs: organize model-customization examples into serverless/serverful/deployment/evaluation subfolders * add job notification integ test

github-actions · 2026-08-05T16:41:30Z

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

Code Review — Master nova follow ups

Verify prior findings against current code
Check correctness, performance, robustness, tests
Post consolidated review

I re-verified every prior finding against the current branch. The 🔴 correctness bug and all robustness/performance notes are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:132-134

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.

Consider deferring notification setup to train() (skipping under dry_run=True).

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

sagemaker-train/src/sagemaker/train/common_utils/notifications.py (_validate_sns_topic, delete_notification_rule)

_validate_notifications_permissions (line 154) correctly uses e.response.get("Error", {}).get("Code", ""), but _validate_sns_topic and delete_notification_rule still branch on message/type-name substrings ("NotFound", "AccessDenied", "ResourceNotFoundException"). String matching is fragile to botocore wording changes. Switch on the error code consistently.

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

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix and then takes streams[0]["logStreamName"]. Without an explicit orderBy/descending, the default ordering (LogStreamName) isn't guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True. Fine for single-stream jobs, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 to filter by the model-source tag server-side, or log a bounded-scan cap.

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), 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* feat: bedrock model reuse for OSS models * code cleanup: fixed log statements, renamed variables, added type checks --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

github-actions · 2026-08-05T21:18:44Z

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

Code Review — Master nova follow ups

Read PR diff
Verify correctness / breaking changes
Check best practices, security, performance, tests
Post consolidated review

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

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:132-134

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.

Consider deferring notification setup to train() (skipping under dry_run=True).

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

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:179,184,333

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

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

sagemaker-train/src/sagemaker/train/common_utils/cloudwatch_metrics.py:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix (line 108) and then takes streams[0]["logStreamName"] (line 122). Without an explicit orderBy/descending, the default ordering (LogStreamName) isn't guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True. Fine for single-stream jobs, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 to filter by the model-source tag server-side, or log a bounded-scan cap.

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), 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

…es=True) (#6158) - Add show_metrics() assertions to existing Nova SFT tests (serverless + serverful) - Add show_metrics() via MLflow to OSS Llama SFT test - Add show_metrics() to MTRL trainer integration test - Add reuse round-trip tests to test_model_customization_deployment.py (OSS, us-west-2) - Add reuse round-trip tests to test_nova_model_customization_deployment.py (Nova, us-east-1) - Add build(reuse_resources=True) tests to both deployment test files

github-actions · 2026-08-05T22:11:11Z

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

Code Review — Master nova follow ups

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

I re-verified each prior finding against the current branch. The 🔴 correctness bug and all robustness/performance notes are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:132-134

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.

Consider deferring notification setup to train() (skipping under dry_run=True).

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

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:179,184 (_validate_sns_topic) and delete_notification_rule

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule matches on the exception type name. Meanwhile _validate_notifications_permissions in the same file already uses e.response.get("Error", {}).get("Code", "") — as do iam_role_resolver.py / data_utils.py elsewhere in this PR. 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:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix (108) and then takes streams[0]["logStreamName"] (122). Without an explicit orderBy/descending, the default ordering (LogStreamName) isn't guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True. Fine for single-stream jobs, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 to filter by the model-source tag server-side, or log a bounded-scan cap.

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), 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

* fix: usuability update from feedback * update documentation * docs: organize model-customization examples into serverless/serverful/deployment/evaluation subfolders * add job notification integ test * feat(telemetry): Add tracking for dry_run, notifications, reuse_resources, show_metrics, and stream_logs * update telemetry * update telemetry

github-actions · 2026-08-05T23:15:54Z

Claude finished @zhaoqizqwang's task in 59s —— View job

Code Review — Master nova follow ups

Read changed files & verify prior findings against current code
Check correctness, best practices, security, performance, tests
Post consolidated review

I re-verified each prior finding against the current branch. The 🔴 correctness bug and all robustness/performance notes are still present.

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

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

evaluate() still has two if dry_run: blocks. The first (680–682) fires immediately after _get_aws_execution_context() and returns before _resolve_model_artifacts() (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
model_package_group_arn = self._get_model_package_group_arn()
...
pipeline_definition = self._render_pipeline_definition(template_str, template_context)
name = self.base_eval_name or f"benchmark-eval-{self.benchmark.value}"

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

So dry_run skips model-artifact resolution and template construction — the exact checks a dry-run should exercise. This contradicts the method's docstring ("runs all validation (IAM, model resolution, data paths)") and diverges from every other evaluator in this PR (custom_scorer, inspect_ai, llm_as_judge, multi_turn_rl), which gate dry_run after resolution. A user calling evaluate(dry_run=True) against an unresolvable model would incorrectly get "validation passed."

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

Test gap: test_dry_run_returns_none patches _resolve_model_artifacts but never asserts it was called, so it passes regardless of which block returns. Add mock_resolve.assert_called_once() 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:134-136

# Set up notifications if configured
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, undercutting the "no side effects" promise of dry-run.

Consider deferring notification setup to train() (skipping under dry_run=True).

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

sagemaker-train/src/sagemaker/train/common_utils/notifications.py:179,184 (_validate_sns_topic) and delete_notification_rule

_validate_sns_topic branches on "NotFound" in error_msg / "AccessDenied" in error_msg, and delete_notification_rule matches on the exception type name. Meanwhile _validate_notifications_permissions in the same file already uses e.response.get("Error", {}).get("Code", "") — as do iam_role_resolver.py / data_utils.py elsewhere in this PR. 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:108-122

_fetch_smtj_logs calls describe_log_streams with only logStreamNamePrefix and then takes streams[0]["logStreamName"]. Without an explicit orderBy/descending, the default ordering (LogStreamName) isn't guaranteed to surface the latest/intended stream. If "latest" is the intent, pass orderBy="LastEventTime", descending=True. Fine for single-stream jobs, non-deterministic in general.

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

sagemaker-serve/src/sagemaker/serve/model_reuse.py and model_builder.py._find_reusable_model

These enumerate all resources and issue a list_tags_for_resource / list_tags call per resource until a match — 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 to filter by the model-source tag server-side, or log a bounded-scan cap.

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), 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.

The 🔴 benchmark dry-run bug is the one worth fixing before merge.

…w-ups

…tics) (#6164) - Add LogStreamer.poll_tail() for fetching last N events: - Stream mode (SMTJ): backward pagination via get_log_events nextBackwardToken - Filter mode (SMHP): filter_log_events with startFromHead=False, paginate until N matches collected (CW bounds pages by scan volume, not result count) - Multi-stream jobs: merge events across streams by timestamp, return globally last N - stream_log_loop: when tail_lines is set, call poll_tail() and return immediately - Refactor _stream_logs_smhp to delegate to LogStreamer + stream_log_loop, eliminating ~80 lines of duplicated inline polling logic - Validate start_time >= 2024-01-01 in _tail_filter_mode (CW API restriction) - Remove dead code: lines_printed counter no longer needed in SMHP forward loop - Add unit tests for poll_tail (stream mode, filter mode, multi-stream merge, multi-page pagination, pre-2024 validation) - Add unit tests for stream_log_loop tail_lines integration

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

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

fix: usuability update from feedback (#6150)

a0b7d5e

* fix: usuability update from feedback * update documentation

zhaoqizqwang temporarily deployed to auto-approve August 4, 2026 21:02 — with GitHub Actions Inactive

Doc update: reorganize examples folder (#6155)

5ccefa4

* fix: usuability update from feedback * update documentation * docs: organize model-customization examples into serverless/serverful/deployment/evaluation subfolders * add job notification integ test

zhaoqizqwang temporarily deployed to auto-approve August 5, 2026 16:39 — with GitHub Actions Inactive

feat: bedrock model reuse for OSS models (#6157)

6a7a962

* feat: bedrock model reuse for OSS models * code cleanup: fixed log statements, renamed variables, added type checks --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>

mujtaba1747 temporarily deployed to auto-approve August 5, 2026 21:18 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve August 5, 2026 22:10 — with GitHub Actions Inactive

mujtaba1747 temporarily deployed to auto-approve August 5, 2026 22:11 — with GitHub Actions Inactive

zhaoqizqwang temporarily deployed to auto-approve August 5, 2026 23:15 — with GitHub Actions Inactive

zhaoqizqwang and others added 3 commits August 6, 2026 10:57

Merge remote-tracking branch 'upstream/master' into master-nova-follo…

0ebc884

…w-ups

Remove duplicated dry_run return

bd336c1

Navigation Menu

Uh oh!

Master nova follow ups - #6051

Master nova follow ups#6051
mujtaba1747 wants to merge 31 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 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 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 31, 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 before the validation it advertises

🟡 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!

github-actions Bot commented Aug 4, 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 before the validation it advertises

🟡 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!

github-actions Bot commented Aug 5, 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 before the validation it advertises

🟡 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!

github-actions Bot commented Aug 5, 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 before the validation it advertises

🟡 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!

github-actions Bot commented Aug 5, 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 before the validation it advertises

🟡 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!

github-actions Bot commented Aug 5, 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 before the validation it advertises

🟡 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!

6 participants

codecov Bot commented Jul 18, 2026 •

edited

Loading

github-actions Bot commented Jul 31, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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

github-actions Bot commented Aug 4, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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

github-actions Bot commented Aug 5, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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

github-actions Bot commented Aug 5, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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

github-actions Bot commented Aug 5, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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

github-actions Bot commented Aug 5, 2026 •

edited

Loading

🔴 Correctness — `benchmark_evaluator.py`: dry-run returns before the validation it advertises

🟡 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