Skip to content

fix(serve): repack source_code for image_uri/ModelTrainer builds - #6112

Open
lucasjia-aws wants to merge 1 commit into
aws:masterfrom
lucasjia-aws:fix/6105
Open

fix(serve): repack source_code for image_uri/ModelTrainer builds#6112
lucasjia-aws wants to merge 1 commit into
aws:masterfrom
lucasjia-aws:fix/6105

Conversation

@lucasjia-aws

Copy link
Copy Markdown
Collaborator

ModelBuilder classified any image_uri build with a model artifact and source_code (no model/inference_spec) as passthrough and dropped the inference code, so build() produced a model.tar.gz without code/ and register() yielded a package that could not serve. This regressed the classic v2 Model(model_data, entry_point, source_dir) repack behavior.

In _build_for_passthrough(), distinguish a pure image-only deployment from an "image + model artifact + source_code" build. For the latter, keep the source code and bridge model_path to s3_model_data_url so the existing _upload_code(repack=True) path repacks the code into the model artifact, producing a self-contained tarball. Pure-image (Nova/BYOC) passthrough behavior is unchanged. ModelTrainer builds are covered too since they are normalized to model_path before validation.

Also pass script_dependencies (a list derived from SourceCode) to repack_model instead of the deprecated self.dependencies auto-detect dict, which repack_model would otherwise iterate as filesystem paths.

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.

ModelBuilder classified any image_uri build with a model artifact and
source_code (no model/inference_spec) as passthrough and dropped the
inference code, so build() produced a model.tar.gz without code/ and
register() yielded a package that could not serve. This regressed the
classic v2 Model(model_data, entry_point, source_dir) repack behavior.

In _build_for_passthrough(), distinguish a pure image-only deployment
from an "image + model artifact + source_code" build. For the latter,
keep the source code and bridge model_path to s3_model_data_url so the
existing _upload_code(repack=True) path repacks the code into the model
artifact, producing a self-contained tarball. Pure-image (Nova/BYOC)
passthrough behavior is unchanged. ModelTrainer builds are covered too
since they are normalized to model_path before validation.

Also pass script_dependencies (a list derived from SourceCode) to
repack_model instead of the deprecated self.dependencies auto-detect
dict, which repack_model would otherwise iterate as filesystem paths.
@lucasjia-aws

Copy link
Copy Markdown
Collaborator Author

related to issue #6105

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

Claude finished @lucasjia-aws's task in 6m 29s —— View job


Code Review — PR #6112: repack source_code for image_uri/ModelTrainer builds

  • Read PR diff (/tmp/pr.diff)
  • Trace passthrough → _create_model_prepare_container_def(_base)_upload_code(repack=True) paths
  • Analyze correctness, best practices, tests
  • Post review

Overall this is a well-reasoned, well-commented fix. The logic is sound: bridging model_paths3_model_data_url, clearing s3_upload_path, and letting is_repack()/_upload_code(repack=True) produce a self-contained model.tar.gz correctly restores the classic v2 repack behavior, and the model-data resolution (repacked_model_data or s3_upload_path or s3_model_data_url at model_builder.py:2437 / :2588) picks up repacked_model_data as intended. The script_dependencies fix is also correct — _upload_code's non-repack branch already relies on self.script_dependencies (model_builder.py:2280), and self.dependencies is the deprecated auto-detect dict ({"auto": ...}) that repack_model would wrongly iterate as filesystem paths.

A few points worth addressing:

1. No tests added for the new behavior (most important)

All 46 additions are in model_builder.py; there are no test changes. This PR fixes a regression and adds a new decision branch in _build_for_passthrough() plus changes repack dependency handling, yet nothing guards against re-regression. There's already a good home for these in sagemaker-serve/tests/unit/test_model_builder_methods.py (test_build_for_passthrough_*). Please add coverage for:

  • image_uri + s3 model_path + source_code → asserts _upload_code(repack=True) runs and repacked_model_data drives ModelDataUrl (code ends up under code/).
  • model_paths3_model_data_url bridging and s3_upload_path reset to None.
  • pure image-only passthrough (Nova/BYOC) → unchanged (regression guard for the existing behavior at model_builder.py:1920-1929).
  • repack_dependenciesrepack_model is called with the script_dependencies list, not self.dependencies.

2. has_source_code requires both entry_point and source_dir (model_builder.py:1894-1896)

_initialize_script_mode_variables can set entry_point while leaving source_dir=None (model_builder.py:1659-1662) when SourceCode.source_dir is unset but entry_script is a standalone file. In that case has_source_code is False, so this new branch is skipped and the fall-through path at :1926-1927 silently nulls entry_point/source_dir — dropping the inference code, i.e. the exact regression this PR fixes, just for the single-file layout. This does mirror is_repack() (:2246), so it may be an acceptable known limitation, but if single-file entry points are a supported input it's worth handling or at least logging a warning rather than silently dropping code.

3. Minor: inconsistent str() guarding

model_builder.py:1887 uses str(self.model_path).startswith("s3://") while the pre-existing fall-through at :1920 uses self.model_path.startswith(...). Both are guarded by a truthiness check so neither errors, but the defensive str() in the new code suggests model_path might sometimes be non-str — if that's a real concern it applies to :1920 too; if not, the str() is unnecessary. Not blocking.

Nothing here is a correctness blocker for the happy path — the primary ask is test coverage (#1). Points #2 and #3 are for your judgment.

Note: the inline-comment tool wasn't available in this session, so I've consolidated the review here with file:line references instead of inline anchors.

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.

2 participants