fix(serve): create local model_path dir before using it as download dir

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.

[lucasjia-aws]

ModelBuilder.build() assigned the default model_path (/tmp/sagemaker/model-builder/) to settings._local_download_dir without creating it on disk, so repack_model()'s _tmpdir() validation raised "Inputted directory ... does not exist" for source_code repack builds. Only use model_path as the local download dir when it is a local path, creating it first; skip s3:// URIs.

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]

Claude finished @lucasjia-aws's task in 50s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff
• Review the change for correctness, best practices, security, tests

---

The fix looks correct and well-targeted. ModelBuilder's default model_path is a local dir (/tmp/sagemaker/model-builder/<uuid>) created lazily, and repack_model()'s _tmpdir() validation requires it to exist on disk — so creating it before wiring it into settings._local_download_dir resolves the "Inputted directory ... does not exist" error.

A few things I verified:

• ✅ os is already imported (model_builder.py:22), so os.makedirs is safe.
• ✅ The isinstance(self.model_path, str) guard correctly avoids the case where model_path could be non-string, and not startswith("s3://") correctly skips remote artifacts. This is consistent with the existing self.model_path.startswith("s3://") checks elsewhere in the file (lines 1883, 1918).
• ✅ os.makedirs(..., exist_ok=True) is idempotent, so re-running build() won't raise.

Minor considerations (non-blocking)

1. Behavior change for s3:// paths: Previously _local_download_dir was set unconditionally to self.model_path (even when it was an s3:// URI, e.g. from a TrainingJob/ModelTrainer). Now it's left unset for s3:// paths. This is almost certainly the right behavior (a local download dir should never be an S3 URI), but it is a subtle behavior change — worth confirming no downstream code relied on _local_download_dir holding the S3 URI.

2. file:// prefix: The guard treats file://... paths as local and would call os.makedirs("file://..."), creating a literal file:/ directory. Looking at the code, model_path is stored as a plain path and file:// is only prepended when building s3_upload_path (line 1788), so this shouldn't occur in practice — just flagging it as a latent edge case if a user ever passes a file://model_path.

3. Tests: No test accompanies this change. A small unit test asserting that (a) a local model_path is created on disk and assigned to settings._local_download_dir, and (b) an s3://model_path leaves it unset, would guard against regressions of exactly the bug this fixes.

Overall this is a small, sensible bug fix. The main suggestion is adding a regression test for the source_code repack path.

[lucasjia-aws]

1. s3:// behavior change — Intentional and safe. settings.local_download_dir is only ever consumed as the parent dir for tempfile.mkdtemp() in repack_model() and fw_utils — nothing reads it as an S3 URI. The previous unconditional assignment was actually buggy for an S3 model_path (e.g. from a TrainingJob/ModelTrainer): _tmpdir() would call os.path.exists("s3://...") and raise the same ValueError. Leaving it unset for S3 paths falls back to the system temp dir, which is the correct behavior.

2. file:// prefix — Not reachable in practice. model_path is always stored as a plain filesystem path; file:// is only prepended when constructing s3_upload_path, so os.makedirs("file://...") can't occur here. Agreed it's a latent edge case, but out of scope for this fix.

3. Regression test — Added TestModelBuilderLocalDownloadDir in test_model_builder_core.py covering both branches: a local model_path is created on disk and wired into settings.local_download_dir, and an s3://model_path leaves it unset.

[github-actions]

Claude finished @lucasjia-aws's task in 46s —— View job

---

PR Review: create local model_path dir before using it as download dir

• Read the PR diff (/tmp/pr.diff)
• Review the fix for correctness against repack_model/fw_utils consumers
• Review the added regression tests

---

The updated PR looks clean and correct. The added tests address the one gap flagged in the earlier round.

Correctness — confirmed

• The guard isinstance(self.model_path, str) and not self.model_path.startswith("s3://") correctly scopes _local_download_dir to real local paths, and os.makedirs(..., exist_ok=True) satisfies the os.path.exists(...) and os.path.isdir(...) check in fw_utils._tmpdir/repack_model (sagemaker-core/src/sagemaker/core/fw_utils.py:450-463).
• The behavior change for s3:// paths is the right call and, as the author noted, actually fixes a latent bug: previously an S3 model_path (from TrainingJob/ModelTrainer, e.g. model_builder.py:3064,3072) would be assigned verbatim and then trip the same ValueError in _tmpdir. Both consumers (fw_utils.py:462, common_utils.py:595-601) only use it as the dir= parent for tempfile.mkdtemp, so leaving it unset falls back to the system temp dir — no code reads it as an S3 URI. ✅
• exist_ok=True keeps build() idempotent on re-run. ✅

Tests — good coverage

• test_local_model_path_is_created_and_used and test_s3_model_path_leaves_local_download_dir_unset cover both branches directly, using a real SessionSettings() so the local_download_dir property (not just the mock attribute) is exercised. This is exactly the regression guard the source_code repack path needed. ✅

Minor (non-blocking)

• The file:// edge case is still theoretically present but, as discussed, unreachable — model_path is only ever a plain filesystem path and file:// is prepended solely for s3_upload_path. Fine to leave out of scope.

No blocking issues. Ship it.

[lucasjia-aws]

Both test failures are flaky and unrelated to this PR (which only touches sagemaker-serve/model_builder.py):

• mlopstest_feature_processor_transform_online_only_store_ingestion — a ConcurrentModificationException in the shaded AWS SDK AttributeMap when the Feature Store Spark connector builds its client concurrently across executor threads.
• us-east-1test_inspect_ai_upload_benchmarks — the pipeline execution failed server-side with Pipeline ... has been modified since your last read (a concurrent-modification conflict under parallel test runs).

Both are concurrency races in infra/service code, not regressions from this change. Retrying should turn them green.