feat: add output_data property to ModelTrainer (#1936)

[mohamedzeidan2021]

Problem

ModelTrainer (v3) exposed no way to get the S3 location of a completed training job's output data archive (output.tar.gz — the non-model output under .../output/, distinct from the model artifact model.tar.gz). Users had to reach into the private _latest_training_job resource and hand-compose the path.

Fix

Adds a read-only output_data property to ModelTrainer that returns the fully-qualified S3 URI {s3_output_path}/{training_job_name}/output/output.tar.gz. The S3 output path is resolved from the latest training job's output_data_config, falling back to the trainer's own output_data_config. Raises a clear ValueError when no training job exists yet or no output path can be resolved. The layout matches the repo's canonical build_nova_output_tar_gz_s3_uri. No banned v2 patterns; backward-compatible (net-new, first @property on the class).

Testing

6 new unit tests in tests/unit/train/test_model_trainer.py covering the derived URI, trailing-slash normalization (both job-config and fallback paths), the trainer-config fallback, and both ValueError guards. Full module: 79 passed. black/flake8 clean. Negative control confirmed the tests bind to the change.

Fixes #1936

[github-actions]

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

---

Review: output_data property on ModelTrainer (#6355)

• Read PR diff
• Analyze source context (model_trainer.py, output-path conventions)
• Verify correctness & tests
• Post review feedback

Verdict: looks good. The change is correct, backward-compatible (net-new read-only property), well-documented, and thoroughly tested. No banned v2 patterns. A couple of optional notes below — nothing blocking.

Correctness ✅

• The path layout {s3_output_path}/{training_job_name}/output/output.tar.gz matches SageMaker's convention (the job's s3_output_path is the base; SageMaker appends {job_name}/output/…) and the repo's canonical build_nova_output_tar_gz_s3_uri helper.
• The falsy checks (if job_output_config: / if not s3_output_path) correctly handle sagemaker-core's Unassigned sentinel — I confirmed Unassigned.__bool__ returns False (sagemaker-core/src/sagemaker/core/utils/utils.py:306), so both an Unassigned config and an Unassigned s3_output_path fall through to the trainer-config fallback as intended.
• rstrip("/") is only reached after the if not s3_output_path guard, so there's no risk of calling it on None/Unassigned.
• Both ValueError guards (no job / no resolvable path) are reasonable and clearly messaged.

Optional suggestions (non-blocking)

1. Reuse the existing helper. sagemaker-core/src/sagemaker/core/training/utils.py:122 already implements exactly this path composition:

   def build_nova_output_tar_gz_s3_uri(s3_output_path, training_job_name):
       output_path = s3_output_path.rstrip("/")
       return f"{output_path}/{training_job_name}/output/output.tar.gz"

   The final two lines of the property duplicate it. Reusing it would keep the two in sync if the layout ever changes. The counter-argument is the helper's nova-specific name, so this is a judgment call — if you don't reuse it, the duplication is at least worth a comment cross-referencing the helper so they don't drift.

2. Naming consistency. Consider whether a companion accessor for the model artifact (model.tar.gz) is expected alongside this; if so, aligning names now (e.g. output_data / model_data) avoids churn later. Not needed for this PR.

Tests ✅

The 6 new tests in tests/unit/train/test_model_trainer.py cover the derived URI, trailing-slash normalization on both the job-config and fallback paths, the trainer-config fallback, and both ValueError guards. Good coverage for the behavior added.

Security / performance

No concerns — pure string derivation from already-loaded config, no I/O, no credential handling.

---

Note: the inline-comment tool wasn't available in this environment, so the file/line references are inlined above.