fix/issue 5897 crlf sm train

[wasim-builds]

Fixes #5897

Problem

ModelTrainer writes files with platform-default line endings, causing CRLF/LF inconsistencies on Windows.

Fix

Explicitly normalize line endings to LF when writing training artifacts.

Scope

Single-line change in sagemaker-train/src/sagemaker/train/model_trainer.py.

Verification

Training artifacts now have consistent LF line endings regardless of OS.

[wasim-builds]

Hi everyone,

Quick follow-up on this PR concerning fix/issue 5897 crlf sm train in aws/sagemaker-python-sdk. Working on Amazon SageMaker Python SDK machine learning and model deployment pipelines has been a great experience, and I am very keen to continue contributing to aws/sagemaker-python-sdk.

I am also open to collaborating on a contract/freelance basis, discussing full-time opportunities, or joining the organization/team.

You can view my background and other open-source contributions at https://github.com/wasim-builds.

[mohamedzeidan2021]

Apologies for opening and re-opening, will add unit tests and look to get this merged!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[mohamedzeidan2021]

Codebuild succeeded, tests pass: https://tiny.amazon.com/kmwe9wt0/usweconsawsamazcodecode7296

[github-actions]

Claude finished @mohamedzeidan2021's task in 44s —— View job

---

Code Review — PR #6088 (CRLF → LF for sm_train.sh)

• Read the PR diff (/tmp/pr.diff)
• Review the source change in model_trainer.py
• Review the added unit tests
• Check for similar unprotected script writes nearby

Verdict: ✅ Looks good

The one-line change is correct and precisely scoped.

sagemaker-train/src/sagemaker/train/model_trainer.py:1394

with open(os.path.join(tmp_dir.name, TRAIN_SCRIPT), "w", newline="\n") as f:
    f.write(train_script)

On Windows, text-mode open(..., "w") with the default newline=None translates every \n to os.linesep (\r\n). Since sm_train.sh is a bash script executed inside a Linux training container, CRLF endings break it (the classic #!/bin/bash\r: bad interpreter failure, plus \r corrupting any line). Pinning newline="\n" guarantees LF regardless of the host OS. This is the right fix for #5897.

Observations

• Correctness: newline="\n" disables newline translation on write without altering the written string — behavior is unchanged on POSIX and fixed on Windows. No public API or signature change, so no backward-compatibility concern for SDK consumers.
• Other writes are fine: the two nearby writes (_write_source_code_json:1302, _write_distributed_json:1313) use json.dumps(...) without indent, producing single-line output with no embedded newlines, so they are not affected by CRLF translation. No action needed there.
• Tests: two tests are added.
  • test_prepare_train_script_opened_with_lf_newline is the meaningful one — it intercepts the open call and asserts newline="\n" was passed, so it fails without the fix regardless of the test host OS. This is the correct way to guard an OS-dependent behavior that can't be reproduced on the Linux CI runner.
  • test_prepare_train_script_uses_lf_line_endings is a weaker byte-level check (it would pass on POSIX even without the fix, as the docstring of the sibling test honestly notes), but it's a reasonable complementary assertion.

Minor (non-blocking) nits

• Both tests manually defeat TemporaryDirectory cleanup (tmp_dir._cleanup = False; tmp_dir.cleanup = lambda: None) and rely on a finally: shutil.rmtree(...). Mutating the private _cleanup attribute is a bit fragile across Python versions; a tmp_dir.name pointed at a pytest tmp_path fixture would be cleaner, but this is a test-only concern and not worth blocking.
• test_prepare_train_script_uses_lf_line_endings is largely subsumed by the second test. Keeping it is harmless.

No correctness, security, or performance issues found. The PR is already merged; no changes requested.

Note: the inline-comment tool was not available in this run, so findings are consolidated here.