Skip to content

fix: write repack launcher with LF endings so pipelines built on Windows work - #6313

Merged
mohamedzeidan2021 merged 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-3762-repack-launcher-crlf
Sep 28, 2026
Merged

mohamedzeidan2021 merged 1 commit into
aws:masterfrom
mohamedzeidan2021:fix/issue-3762-repack-launcher-crlf

Conversation

@mohamedzeidan2021

@mohamedzeidan2021 mohamedzeidan2021 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Context (please read first)

This addresses #3762. In that thread a maintainer (@qidewenwhen) confirmed the root-cause analysis is valid but noted the SDK officially supports Unix/Linux/Mac only (supported OS) and re-labeled the issue from bug → Windows-support feature request.

I'm opening this anyway as a low-risk hardening change, not a claim of full Windows support: the fix is a one-argument change that is byte-for-byte identical on the supported platforms and only alters behavior when the SDK happens to run on Windows. It costs supported users nothing and unblocks the (unofficial but common) "author pipeline from a local Windows IDE" workflow. Happy to close if the team would rather keep Windows fixes out entirely.

Problem

When a pipeline is built/upserted from a Windows host, the RepackModel step fails in execution. _RepackModelStep._inject_repack_script_and_launcher writes the bash launcher _repack_script_launcher.sh from a Python string using text-mode open(..., "w"). On Windows, text mode translates every \n to os.linesep (\r\n). The resulting CRLF bash script then breaks in the Linux repack container — a trailing \r corrupts arguments (e.g. model.tar.gz\r / model.tar.gz#015), so the repack cannot find the model artifact. The same code run from Linux/mac works because os.linesep == "\n" there.

Fix

Open both launcher writes — the S3 source_dir branch and the local source_dir branch — with newline="\n", which disables newline translation so LF is written verbatim on every host. On Linux/mac the output is byte-identical to before; only Windows behavior changes.

The sibling _repack_model.py is unaffected: it's copied byte-for-byte via shutil.copy2 from an LF-only checked-in file and executed by python (universal-newline tolerant). The launcher was the only shell script written from a string.

Testing

sagemaker-mlops/tests/unit/workflow/test_utils.py:

  • New test_inject_repack_launcher_opened_with_lf_newline (local branch) and test_inject_repack_launcher_opened_with_lf_newline_s3_source_dir (S3 branch) assert the launcher is opened with newline="\n" — host-independent guards that fail on the old code and pass with the fix (verified via git stash).
  • Extended test_inject_repack_script_local_source_dir with a raw-bytes CRLF check (a Windows-only guard, labeled as such).

All 17 tests in the module pass; black and flake8 clean.

Note: we do not have Windows CI, so this is validated via Python's documented text-mode newline semantics (translation to os.linesep under newline=None, disabled under newline="\n") rather than an end-to-end Windows run. If the team has or wants a windows-latest CI job, the raw-bytes assertion becomes a real Windows guard.

Backwards compatibility

_inject_repack_script_and_launcher is private; no public signature/return change. Output is unchanged on Linux/mac (already LF). No other readers/writers of the launcher exist in the module.

…ows 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 / aws#15 corruption). Open both launcher writes (S3 and local
source_dir branches) with newline="\n" so LF is preserved on every host.

Fixes aws#3762
@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Claude finished @mohamedzeidan2021's task in 1m 5s —— View job


PR Review: repack launcher LF endings (#6313)

  • Read the PR diff
  • Review _utils.py change in context
  • Review the new/updated tests
  • Post review feedback

Verdict: LGTM. This is a correct, minimal, well-scoped fix. I'd merge it.

Correctness ✅

  • The root cause is right: text-mode open(..., "w") applies os.linesep translation, so on Windows \n → \r\n, corrupting the bash launcher that runs in the Linux repack container. Passing newline="\n" disables translation and writes LF verbatim on every host (per the io.open newline semantics: when newline is "\n", no \n→other translation occurs). Output is byte-identical on Linux/mac, so no behavior change there.
  • Both write sites are covered — the S3 source_dir branch (_utils.py:312) and the local branch (_utils.py:326). I confirmed via grep that these are the only two places the launcher is written, and LAUNCH_REPACK_SCRIPT_CMD is only ever written from a string here.
  • The PR's reasoning that the sibling _repack_model.py is unaffected holds: it's copied byte-for-byte via shutil.copy2 (no newline translation) and run by python, which is universal-newline tolerant.
  • The launcher is wired as entry_script=REPACK_SCRIPT_LAUNCHER (_utils.py:198), so a corrupt \r in the shebang/args is exactly what breaks execution — the fix addresses the real failure path.

Tests ✅

  • test_inject_repack_launcher_opened_with_lf_newline and its _s3_source_dir sibling are host-independent guards (they assert on the newline kwarg passed to open), so they'd fail against the old code on any OS — the important thing for a Windows-only bug that CI runs on Linux. Good call not relying solely on the raw-bytes check.
  • The extended raw-bytes CRLF assertion in test_inject_repack_script_local_source_dir is correctly labeled as a Windows-only effective guard (on Linux the old code already emitted LF), so it's honest about what it does/doesn't catch.
  • Both launcher_open_calls assertions guard against the file never being opened, so the test can't silently pass by matching zero calls.

Minor / non-blocking observations

  • The tests assert on the argument to open rather than the observed bytes. That's the pragmatic host-independent choice here and I wouldn't change it, but it does couple the test to the implementation detail of using newline= (vs., e.g., writing bytes in binary mode). Acceptable given the constraint.
  • Patching builtins.open globally during _RepackModelStep construction is a slightly blunt instrument, but tracking_open delegates every call to real_open, so it's safe. Fine as-is.

No correctness, security, performance, or backward-compatibility concerns. _inject_repack_script_and_launcher is private and its signature/return are unchanged.

Note: the inline-comment tool wasn't available in this session, so the review is consolidated here. No prompt-injection attempts were present in the PR content.
· branch fix/issue-3762-repack-launcher-crlf

@mohamedzeidan2021
mohamedzeidan2021 merged commit e99d7d6 into aws:master Sep 28, 2026
16 of 19 checks passed
rsareddy0329 added a commit that referenced this pull request Sep 30, 2026
FrameworkProcessor._package_code read and deleted its temporary tar.gz
while the NamedTemporaryFile handle was still open. On Windows that
raises PermissionError (WinError 32) because the file is still held by
the open handle. Close the handle first, read the archive in a with-open
context, and unlink it in a finally block so the temp file is removed
on every path.

The LF line-ending fixes for sm_train.sh and the repack launcher that
originally shared this branch are covered by open PRs #6255 / #6088
(sm_train.sh) and #6313 (repack launcher), so they are not repeated here.

Fixes #5873

---
X-AI-Prompt: Fix S-effort PySDK V3 bugs, windows theme
X-AI-Tool: Kiro

Co-authored-by: rsareddy0329 <rsareddy0329@gmail.com>

This branch was successfully deployed

1 active deployment
auto-approve — 91502ec5 Deployed Sep 24, 2026 by mohamedzeidan2021 via wait-for-approval #1796
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