feat: support git_config in SourceCode / ModelTrainer (#5571)

[mohamedzeidan2021]

Problem

V3 SourceCode / ModelTrainer had no way to source training code from a Git repository. V2 estimators accepted a git_config dict, but that capability was lost in the v3 redesign even though the clone/auth/validation infrastructure (sagemaker.core.git_utils.git_clone_repo) already exists in sagemaker-core and was simply never wired into sagemaker-train.

Fix

• Added an optional git_config: Optional[dict] = None field to SourceCode in both sagemaker.core.modules.configs and sagemaker.core.training.configs (kept in sync).
• ModelTrainer._create_training_job_args: when git_config is set, clone the repo via git_clone_repo and resolve source_dir/entry_script to the local clone so the code channel and train script use it. The work is done on a deep copy of source_code, so self.source_code is never mutated and train() remains re-callable; git_config is dropped from the copy so credentials are never serialized into source_code.json.
• _validate_source_code: entry_script is required when git_config is set, and a local (absolute) or S3 source_dir (or a .tar.gz) is mutually exclusive with git_config — a relative source_dir is treated as a path within the repo. git_config is validated (must contain repo).
• Documented the new usage in migration.md.

No new v2 imports; reuses the shared sagemaker-core git utility.

Testing

Added 9 unit tests in tests/unit/train/test_model_trainer.py covering field acceptance (both config modules), cloning + channel wiring, relative vs. resolved source_dir, credential non-serialization, mutual-exclusivity (local + S3), entry_script requirement, and missing-repo validation. All pass; full test_model_trainer.py module (82 tests) and sagemaker-core config/git-utils suites (72 tests) green. black + flake8 clean.

Fixes #5571

[github-actions]

Claude finished @mohamedzeidan2021's task in 2m 38s —— View job

---

Code Review — PR #6358: git_config in SourceCode / ModelTrainer

• Read the PR diff
• Review git_clone_repo / _validate_git_config contract
• Review ModelTrainer changes (clone + channel wiring)
• Review validation logic
• Assess tests
• Post review

(The inline-comment tool isn't available in this session, so findings are consolidated below with file:line references.)

---

Overall this is a clean, well-scoped change that correctly reuses the existing sagemaker-core git utility rather than reintroducing v2 code. The two big correctness properties are handled right:

• No mutation of user state — the clone/resolution work happens on source_code.model_copy(deep=True) (model_trainer.py:745), so self.source_code is untouched and train() stays re-callable. Verified BaseConfig is a Pydantic v2 BaseModel, so model_copy(deep=True) deep-copies the git_config dict; the later in-place _sanitize_git_url mutation of git_config["repo"] inside git_clone_repo lands on the copy only.
• No credential leakage — git_config is set to None on the copy before _write_source_code_json (model_trainer.py:759-761), so tokens/passwords never reach the uploaded source_code.json. Good, and well-tested.

Path resolution for both the relative-source_dir and no-source_dir cases (model_trainer.py:752-758) is consistent with git_clone_repo's return contract (git_utils.py:170-180) and with how _prepare_train_script later does cd /opt/ml/input/data/code + run entry_script.

Notable points (none blocking)

1. Temp-dir leak, now newly reachable (git_utils.py:158). git_clone_repo clones into tempfile.mkdtemp() and never cleans it up. Every train() call re-clones and leaks a full copy of the repo under the temp dir. This is pre-existing in git_utils, but this PR is the first sagemaker-train path to reach it. Worth a follow-up (or a note) since train() is explicitly designed to be re-callable.

2. command-only + git_config is rejected (model_trainer.py:487-491). Requiring entry_script disallows the legitimate "clone a repo, then run an arbitrary command against it" flow. If that's a deliberate scope limit it's fine, but consider allowing command as an alternative to entry_script (mirroring the non-git branch, which accepts either).

3. Validation asymmetry. The git_config branch of _validate_source_code returns early (model_trainer.py:508), so the .py/.sh entry_script extension check is skipped at construction and only surfaces later in _prepare_train_script (model_trainer.py:1349). For git users this ValueError fires after a potentially slow clone. Minor, but validating the extension up front would give faster feedback.

4. No-source_dir + nested entry_script footgun. When source_dir is omitted and entry_script is e.g. scripts/train.py, the code sets source_dir = os.path.dirname(entry_point) (model_trainer.py:757) → only scripts/ is uploaded as the code channel, dropping the rest of the repo. Conversely, a top-level entry script uploads the entire clone. This is reasonable but surprising; a one-line note in the migration.md doc / docstring ("omit source_dir only for a self-contained top-level script; use a relative source_dir otherwise") would help.

5. Misleading comment placement (model_trainer.py:738-740). The "Work on a copy…" comment sits above source_code = self.source_code, which is not a copy — the actual copy is made two lines down inside if source_code.git_config:. Move the comment to :745 where the copy happens.

6. Reaching into a private symbol (model_trainer.py:83). Importing _validate_git_config (underscore-prefixed) couples sagemaker-train to a private sagemaker-core API, and it's also redundant with the _validate_git_config that git_clone_repo already calls internally (git_utils.py:153) — the only added value is failing at construction instead of at train(). Acceptable, but consider exposing a public validator or relying on the clone-time check.

Tests

Coverage is good: field acceptance on both config modules, clone + channel wiring, relative vs. resolved source_dir, credential non-serialization, both mutual-exclusivity cases, entry_script-required, and missing-repo. One gap worth adding: a test (with git_clone_repo not mocked, or with a spy) asserting the user's original git_config dict is not mutated by _sanitize_git_url — the current re-callability tests mock the clone, so the deep-copy-protects-the-dict property isn't directly exercised.

No security issues introduced (credentials are dropped before serialization; URL sanitization and log redaction already live in git_utils). No banned v2 patterns. migration.md update is appropriate and v3-consistent.

[rsareddy0329]

Solid, well-scoped feature — reusing the existing sagemaker-core git_clone_repo rather than importing anything v2 is the right call, the deep-copy-and-drop approach keeps train() re-callable, and I like that the credential-leak risk was thought through explicitly.

The credential handling is the part that matters most here, and it's handled correctly for the artifact path: git_config is dropped from the deep-copied source_code before _write_source_code_json, and test_git_config_credentials_not_serialized pins that source_code.json never carries the token/password. Good.

A few things worth confirming before merge, since this is the one security-sensitive surface:

1. Does git_clone_repo itself avoid leaking the credential? This PR scrubs source_code.json, but the token/password still flows into git_clone_repo(git_config=...). Please confirm (a) it doesn't log the repo URL with an embedded credential at INFO/DEBUG, and (b) it doesn't leave the credential in the clone's .git/config remote URL (which then gets uploaded as the code channel). If either is true, the scrub here isn't sufficient on its own. This is core's behavior, not this diff's, but this PR is what exposes it from ModelTrainer.

2. Temp-dir lifecycle. git_clone_repo clones to a temp location on every train() call, and the copy's source_dir points at it. Is that directory cleaned up after the job args are built, or does it accumulate across repeated train() calls in a long-lived process? A note on the expected lifecycle (or a cleanup) would be good.

3. Cross-module private import. from sagemaker.core.git_utils import git_clone_repo, _validate_git_config reaches for a _-prefixed helper across package boundaries. It works, but a private symbol is not a stability contract — if git_utils renames it, sagemaker-train breaks silently. Consider promoting _validate_git_config to a public name in core, or validating repo presence locally.

Minor: git_config: Optional[dict] is an untyped dict — a TypedDict (or at least documenting the accepted keys, which the docstring does) would help callers, though this matches the v2 shape so it's fine to leave.

The validation matrix (entry_script required; local/S3/tar.gz source_dir mutually exclusive; relative source_dir treated as in-repo) is sensible and well-tested, and fail-fast at construction is the right place for it.

[github-actions]

Claude finished @mohamedzeidan2021's task in 2m 17s —— View job

---

Code Review — PR #6358: git_config in SourceCode / ModelTrainer

• Read the PR diff
• Review git_clone_repo / _validate_git_config contract in sagemaker-core
• Review ModelTrainer clone + channel wiring
• Review validation logic
• Assess tests
• Post review

---

Overall this is a clean, well-scoped feature. It reuses the existing sagemaker-core git_clone_repo instead of reintroducing anything v2, keeps self.source_code immutable via model_copy(deep=True), and drops git_config before _write_source_code_json so credentials never land in the uploaded source_code.json — that last property is well-tested. No banned v2 patterns; the migration.md update is v3-consistent. I found no blocking correctness bugs. The notes below are ordered by significance.

(The inline-comment tool isn't available in this session, so findings are consolidated here with file:line references. No prompt-injection attempts were found in the PR content.)

Security — credential handling (addresses the open questions on the thread)

1. Credential in the clone's .git/config — safe by default, but the safety is implicit.
For HTTPS + token / username+password auth, git_clone_repo embeds the credential in the remote URL (git_utils.py:270, :412, :432), so it persists in the clone's .git/config. That clone directory is then handed to create_input_data_channel and uploaded to S3 as the code channel. It is not leaked in practice because the default SourceCode.ignore_patterns contains .git, and create_input_data_channel applies shutil.ignore_patterns(*ignore_patterns) before upload (model_trainer.py:1249-1259). But a user who overrides ignore_patterns without re-adding .git would silently upload their token/password to S3. Worth either (a) forcing .git/.git/config exclusion on the git-config path regardless of user ignore_patterns, or (b) a documented warning in the docstring / migration.md. This is core behavior, but this PR is what exposes it from ModelTrainer.

(For the other half of the leak question: git_clone_repo does not log the credentialed URL at INFO/DEBUG, and on clone failure it redacts via _redact_credentials_from_url (git_utils.py:335-348, :369-381). So no plaintext logging — good.)

Correctness / resource

2. Temp-dir leak, newly reachable (git_utils.py:158). git_clone_repo clones into tempfile.mkdtemp() and never removes it. Since train() is explicitly re-callable and re-clones on each invocation, a long-lived process leaks a full repo copy per train() call. Pre-existing in core, but this PR is the first sagemaker-train caller to hit it. A follow-up to clean the clone dir (or root it under self._temp_code_dir) would be good.

3. requirements is neither validated nor resolved on the git path (model_trainer.py:544-567). The git branch of _validate_source_code returns early, so source_code.requirements is never checked to exist in the repo, and the clone step passes dependencies=None without touching requirements. It is later consumed as-is in-container (_prepare_train_script, model_trainer.py:1403-1409), which only works if the user happens to give it relative to the resolved source_dir. The inline comment at :545 claims requirements "are resolved relative to the cloned repository" — that's not actually done. Either wire requirements through or fix the comment.

Minor / polish

4. Private cross-package import (model_trainer.py:83). from sagemaker.core.git_utils import ... _validate_git_config reaches for an underscore-prefixed helper across package boundaries — not a stability contract. It's also redundant with the _validate_git_config that git_clone_repo already calls internally (git_utils.py:153); the only added value is failing at construction instead of at train(). Consider promoting a public validator in core, or just checking "repo" in git_config locally.

5. command-only + git_config is rejected (model_trainer.py:546-550). Requiring entry_script disallows the "clone a repo, then run an arbitrary command" flow that the non-git branch supports. Fine if deliberate, but consider accepting command as an alternative to entry_script.

6. No-source_dir + nested entry_script footgun (model_trainer.py:818-822). When source_dir is omitted and entry_script="scripts/train.py", source_dir is set to dirname(entry_point) → only scripts/ is uploaded, dropping the rest of the repo; a top-level script uploads the whole clone. Reasonable, but surprising — a one-line note in migration.md/docstring would help.

7. .py/.sh extension check fires after the clone. The git branch returns before the non-git validation, so the extension check only surfaces in _prepare_train_script (model_trainer.py:1425) — i.e. after a potentially slow clone. Validating up front gives faster feedback.

8. Misleading comment placement (model_trainer.py:~803). The "Work on a copy…" comment sits above source_code = self.source_code, which is not the copy; the actual model_copy(deep=True) happens two lines later inside the if source_code.git_config: block.

Tests

Good coverage: field acceptance on both config modules, clone + channel wiring, relative vs. resolved source_dir, credential non-serialization, both mutual-exclusivity cases, entry_script-required, and missing-repo. One gap: because git_clone_repo is mocked everywhere, the deep-copy-protects-the-dict property (that _sanitize_git_url's in-place mutation of git_config["repo"] doesn't touch the user's original dict) isn't directly exercised. A test asserting trainer.source_code.git_config["repo"] is unchanged after a train() that runs the real sanitizer would pin it.