Skip to content

fix: align recipe git clone error handling with git_utils - #6544

Open
aviruthen wants to merge 1 commit into
aws:masterfrom
aviruthen:fix-recipe-clone-error-handling
Open

aviruthen wants to merge 1 commit into
aws:masterfrom
aviruthen:fix-recipe-clone-error-handling

Conversation

@aviruthen

Copy link
Copy Markdown
Collaborator

Summary

The Training Recipes clone helper _run_clone_command_silent exists in two places (sagemaker-train and sagemaker-core modules). Its HTTPS failure handling has drifted from sagemaker.core.git_utils._run_clone_command. This change brings both copies in line with git_utils, so a failed recipe clone produces the same URL formatting and exception shape everywhere in the SDK.

Changes

  • sagemaker-train/src/sagemaker/train/utils.py and sagemaker-core/src/sagemaker/core/modules/utils.py:
    • Reuse the existing URL helper from sagemaker.core.git_utils when reporting an HTTPS clone failure.
    • Raise a new CalledProcessError built from the formatted URL (from None). This matches git_utils._run_clone_command and keeps tracebacks to a single, clean exception.
    • Error log messages use the same formatted URL as the raised exception.
  • Successful clones and the SSH code path are unchanged.

Testing

  • Added sagemaker-train/tests/unit/train/test_utils_clone.py and sagemaker-core/tests/unit/modules/test_utils_clone.py, which cover:
    • failure with userinfo in the URL (formatted URL in the log and exception, return code preserved)
    • failure with a plain URL (URL unchanged)
    • success (original URL passed to git clone, GIT_TERMINAL_PROMPT=0 still set)
  • Existing suites pass: sagemaker-train recipe tests, sagemaker-core tests/unit/modules, and tests/unit/test_git_utils.py.
  • black and flake8 are clean on the changed files.

@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:49 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:49 — with GitHub Actions Active
@aviruthen
aviruthen deployed to auto-approve October 28, 2026 22:50 — with GitHub Actions Active

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI code review

This PR aligns the HTTPS failure path of _run_clone_command_silent in both sagemaker-train and sagemaker-core with sagemaker.core.git_utils._run_clone_command: it redacts credentials from the URL via the existing _redact_credentials_from_url helper, reconstructs a CalledProcessError with the sanitized command, logs the safe URL, and re-raises from None. The SSH path is correctly left unchanged, consistent with prior git_utils work (PR #5811/#5812). New unit tests cover sanitized-failure, plain-URL, and success cases in both modules. The change is correct, well-scoped, and adequately tested; I found no issues.

Reviewed commit b8cb194. Automated review; verify before acting.

This branch was successfully deployed

1 active deployment
auto-approve — b8cb194f Deployed Oct 28, 2026 by aviruthen via wait-for-approval #771
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