From 91502ec596adf54767a9138128e21d3a17b136d6 Mon Sep 17 00:00:00 2001 From: Mohamed Zeidan Date: Thu, 24 Sep 2026 13:19:14 -0700 Subject: [PATCH] fix: write repack launcher with LF endings so pipelines built on Windows work _RepackModelStep._inject_repack_script_and_launcher wrote _repack_script_launcher.sh with text-mode open(), which on Windows translates LF to CRLF. The CRLF bash launcher then fails in the Linux repack container (model.tar.gz\\r / #015 corruption). Open both launcher writes (S3 and local source_dir branches) with newline="\n" so LF is preserved on every host. Fixes #3762 --- .../src/sagemaker/mlops/workflow/_utils.py | 10 +- .../tests/unit/workflow/test_utils.py | 109 ++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/sagemaker-mlops/src/sagemaker/mlops/workflow/_utils.py b/sagemaker-mlops/src/sagemaker/mlops/workflow/_utils.py index 332ef22f39..f421eb851e 100644 --- a/sagemaker-mlops/src/sagemaker/mlops/workflow/_utils.py +++ b/sagemaker-mlops/src/sagemaker/mlops/workflow/_utils.py @@ -307,8 +307,10 @@ def _inject_repack_script_and_launcher(self): custom_extractall_tarfile(t, targz_contents_dir) shutil.copy2(fname, os.path.join(targz_contents_dir, REPACK_SCRIPT)) + # newline="\n" forces LF endings so the bash launcher stays runnable in the + # Linux repack container even when written from a Windows host (see issue #3762). with open( - os.path.join(targz_contents_dir, REPACK_SCRIPT_LAUNCHER), "w" + os.path.join(targz_contents_dir, REPACK_SCRIPT_LAUNCHER), "w", newline="\n" ) as launcher_file: launcher_file.write(LAUNCH_REPACK_SCRIPT_CMD) @@ -319,7 +321,11 @@ def _inject_repack_script_and_launcher(self): _save_model(self._source_dir, new_targz_path, self.sagemaker_session, kms_key=None) else: shutil.copy2(fname, os.path.join(self._source_dir, REPACK_SCRIPT)) - with open(os.path.join(self._source_dir, REPACK_SCRIPT_LAUNCHER), "w") as launcher_file: + # newline="\n" forces LF endings so the bash launcher stays runnable in the + # Linux repack container even when written from a Windows host (see issue #3762). + with open( + os.path.join(self._source_dir, REPACK_SCRIPT_LAUNCHER), "w", newline="\n" + ) as launcher_file: launcher_file.write(LAUNCH_REPACK_SCRIPT_CMD) @property diff --git a/sagemaker-mlops/tests/unit/workflow/test_utils.py b/sagemaker-mlops/tests/unit/workflow/test_utils.py index 3c2d6e8f0f..efd4fa51a2 100644 --- a/sagemaker-mlops/tests/unit/workflow/test_utils.py +++ b/sagemaker-mlops/tests/unit/workflow/test_utils.py @@ -16,6 +16,7 @@ import pytest import os +import tarfile import tempfile from unittest.mock import Mock, patch @@ -318,6 +319,114 @@ def test_inject_repack_script_local_source_dir(self, mock_session, temp_entry_po assert "#!/bin/bash" in content assert "python _repack_model.py" in content + # Regression for issue #3762: the launcher is a bash script that runs + # in a Linux container, so it must use LF endings. NOTE: this raw-bytes + # check only actually catches the bug on a Windows host (on Linux/mac the + # old code already emitted LF); the host-independent guard is + # test_inject_repack_launcher_opened_with_lf_newline below. + with open(launcher_path, "rb") as f: + raw = f.read() + assert b"\r\n" not in raw + + def test_inject_repack_launcher_opened_with_lf_newline(self, mock_session, temp_entry_point): + """Regression for #3762: launcher must be opened with newline="\\n". + + On Windows, text-mode ``open(..., "w")`` translates ``\\n`` to ``\\r\\n``, which + corrupts the bash launcher when it later runs in the Linux repack container. This + asserts the fix (``newline="\\n"``) is in place independently of the test host OS. + """ + real_open = open + launcher_open_calls = [] + + def tracking_open(file, mode="r", *args, **kwargs): + if str(file).endswith(REPACK_SCRIPT_LAUNCHER) and "w" in mode: + launcher_open_calls.append(kwargs.get("newline")) + return real_open(file, mode, *args, **kwargs) + + with tempfile.TemporaryDirectory() as temp_dir: + with patch("sagemaker.mlops.workflow._utils.image_uris.retrieve") as mock_retrieve: + with patch("sagemaker.train.ModelTrainer") as mock_trainer: + with patch( + "sagemaker.mlops.workflow._utils.TrainingStep.__init__" + ) as mock_super: + with patch("builtins.open", side_effect=tracking_open): + mock_retrieve.return_value = "sklearn-image:latest" + mock_trainer_instance = Mock() + mock_trainer_instance.train = Mock(return_value=Mock()) + mock_trainer.return_value = mock_trainer_instance + mock_super.return_value = None + + _RepackModelStep( + name="repack-step", + sagemaker_session=mock_session, + role="arn:aws:iam::123456789012:role/SageMakerRole", + model_data="s3://bucket/model.tar.gz", + entry_point=temp_entry_point, + source_dir=temp_dir, + ) + + assert launcher_open_calls, "launcher file was never opened for writing" + assert all(nl == "\n" for nl in launcher_open_calls), ( + "launcher must be opened with newline='\\n' to keep LF endings, " + f"got {launcher_open_calls}" + ) + + def test_inject_repack_launcher_opened_with_lf_newline_s3_source_dir( + self, mock_session, temp_entry_point + ): + """Regression for #3762 on the S3 source_dir branch. + + The S3 path downloads, extracts, and rewrites the source tarball; the launcher it + writes into the extracted dir must also use ``newline="\\n"``. Mirrors + ``test_inject_repack_launcher_opened_with_lf_newline`` for the other code branch. + """ + real_open = open + launcher_open_calls = [] + + def tracking_open(file, mode="r", *args, **kwargs): + if str(file).endswith(REPACK_SCRIPT_LAUNCHER) and "w" in mode: + launcher_open_calls.append(kwargs.get("newline")) + return real_open(file, mode, *args, **kwargs) + + def fake_download(source_dir, dest_path, session): + # Write a minimal valid .tar.gz so the real tarfile extraction succeeds. + with tempfile.TemporaryDirectory() as seed: + seed_file = os.path.join(seed, "dummy.txt") + with real_open(seed_file, "w") as fh: + fh.write("seed") + with tarfile.open(dest_path, "w:gz") as t: + t.add(seed_file, arcname="dummy.txt") + + with patch("sagemaker.mlops.workflow._utils.image_uris.retrieve") as mock_retrieve: + with patch("sagemaker.train.ModelTrainer") as mock_trainer: + with patch("sagemaker.mlops.workflow._utils.TrainingStep.__init__") as mock_super: + with patch( + "sagemaker.mlops.workflow._utils.download_file_from_url", + side_effect=fake_download, + ): + with patch("sagemaker.mlops.workflow._utils._save_model"): + with patch("builtins.open", side_effect=tracking_open): + mock_retrieve.return_value = "sklearn-image:latest" + mock_trainer_instance = Mock() + mock_trainer_instance.train = Mock(return_value=Mock()) + mock_trainer.return_value = mock_trainer_instance + mock_super.return_value = None + + _RepackModelStep( + name="repack-step", + sagemaker_session=mock_session, + role="arn:aws:iam::123456789012:role/SageMakerRole", + model_data="s3://bucket/model.tar.gz", + entry_point=temp_entry_point, + source_dir="s3://bucket/source.tar.gz", + ) + + assert launcher_open_calls, "launcher file was never opened for writing (S3 branch)" + assert all(nl == "\n" for nl in launcher_open_calls), ( + "launcher must be opened with newline='\\n' to keep LF endings, " + f"got {launcher_open_calls}" + ) + def test_properties_returns_parent_properties(self, mock_session, temp_entry_point): """Test properties returns parent class properties.""" with patch("sagemaker.mlops.workflow._utils.image_uris.retrieve") as mock_retrieve: