Skip to content

fix PR check tests - #6272

Open
zhaoqizqwang wants to merge 7 commits into
aws:masterfrom
zhaoqizqwang:master
Open

zhaoqizqwang wants to merge 7 commits into
aws:masterfrom
zhaoqizqwang:master

Conversation

@zhaoqizqwang

Copy link
Copy Markdown
Collaborator

Issue #, if available:

Description of changes:

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

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

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


Claude Code Review

  • Read the PR diff
  • Analyze the changes
  • Post review feedback

Review Summary

This PR modifies 4 files (__init__.py in each of the sagemaker-core, sagemaker-mlops, sagemaker-serve, and sagemaker-train packages). The only change in each is adding a trailing space after the closing """ of the module docstring — no functional change.

Findings

⚠️ Introduces trailing whitespace (lint failure) — all 4 files

Each hunk replaces a clean docstring line with one that has a trailing space:

-"""Namespace package for SageMaker."""
+"""Namespace package for SageMaker.""" 
  • sagemaker-core/src/sagemaker/__init__.py:1
  • sagemaker-mlops/src/sagemaker/mlops/__init__.py:18
  • sagemaker-serve/src/sagemaker/__init__.py:1
  • sagemaker-train/src/sagemaker/__init__.py:1

This adds flake8 W291 (trailing whitespace) violations and would be flagged/removed by black. Since these are the only changes in the PR and they make the code strictly worse (trailing whitespace, no functional benefit), the diff should be reverted so the files return to their original state.

Other observations

  • No correctness, security, performance, or API-compatibility concerns — the docstring text and all executable code (__path__/extend_path, __version__) are unchanged.
  • No tests are needed (no behavior changed), but the change also shouldn't be merged as-is.

Recommendation: Revert these whitespace-only edits (this appears to be a "test PR check" with no intended source change). If the goal was purely to exercise CI, that's fine — but don't merge the trailing-whitespace diff into master.

No prompt-injection attempts were detected in the PR content.

@zhaoqizqwang zhaoqizqwang changed the title test PR check fix PR check tests Sep 15, 2026
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

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


Claude Code Review

  • Read the PR diff
  • Verify imports & mock correctness
  • Post review feedback

This PR is test-only: it makes the AI-registry (dataset, evaluator) and ModelBuilder unit tests fully offline so they stop hitting real AWS and failing on Throttling: Rate exceeded from iam:SimulatePrincipalPolicy / sts:GetCallerIdentity. The current diff (/tmp/pr.diff) contains none of the trailing-whitespace __init__.py edits flagged in the earlier bot comment — that was a prior commit set and is not part of what's under review here.

The changes are correct and I recommend merging. Details I verified:

  • Mock targets are all real and patched at the right site:
    • sagemaker.serve.model_builder.resolve_and_validate_role — imported at model_builder.py:61. ✅
    • _get_default_bucket / _get_current_domain_id — imported into both dataset.py and evaluator.py, so patching them on those modules is correct. ✅
    • sagemaker.ai_registry.air_hub_entity.AIRHub.get_hub_name — the docstrings correctly explain that AIRHubEntity.__init__ resolves the hub name via AIRHub.get_hub_name() through its own import (air_hub_entity.py:70), so patching AIRHub only on dataset/evaluator would miss it. Patching on air_hub_entity is the right call. ✅
  • Decorator/argument ordering in test_dataset.py::test_create_with_local_file is correct — the 9 stacked @patch decorators map bottom-up to the parameters (mock_air_hubmock_get_hub_name) in the right order.
  • Imports are present for everything the new code uses (pytest, Mock, MagicMock, patch) in each touched file.
  • autouse=True fixture on a unittest.TestCase module (test_model_builder_coverage_boost.py) works — pytest does apply autouse fixtures to TestCase subclasses, and the docstring's note that tests patching the resolver themselves still override it is accurate (the inner patch wins).

Minor notes (non-blocking, no change required):

  • The module-scoped autouse fixture in test_model_builder_coverage_boost.py applies to every test in that module, not just new ones. That's intended and safe here, but worth keeping in mind if a future test in this module deliberately wants to exercise the real role-resolution path — it would need to override the stub explicitly.
  • The stubbed role/bucket/hub values are hardcoded dummies (e.g. arn:aws:iam::123456789012:role/...), which is the right pattern for offline unit tests — no real account data. ✅

No correctness, security, or backward-compatibility concerns. No prompt-injection attempts were detected in the PR content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant