feat(postprocess): copy job metadata JSON to log directory for S3 upload

[KaunilD]

Include {job_id}.json alongside config.yaml and sbatch_script.sh when copying artifacts into the log directory, so job metadata is preserved in S3 uploads.

Summary by CodeRabbit

• New Features

  • Improved job artifact archival to include job ID metadata files alongside configuration and script files, providing more complete job documentation in log directories.

• Tests

  • Added test coverage to verify job ID metadata files are correctly copied to log directories.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request extends the _copy_config_to_logs() method to copy an additional job artifact ({job_id}.json) to the log directory alongside existing config.yaml and sbatch_script.sh files. A corresponding unit test validates the new functionality.

Changes

Cohort / File(s)	Summary
Implementation src/srtctl/cli/mixins/postprocess_stage.py	Extended _copy_config_to_logs() to copy {job_id}.json by adding it to the artifacts list and constructing source/destination paths using self.runtime.job_id. Docstring updated to reflect the new artifact.
Test Coverage tests/test_postprocess.py	Added test_copies_job_id_json unit test to verify the new {job_id}.json artifact is copied correctly to the log directory with matching JSON content.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• feat(postprocess): copy config YAML and sbatch script to log directory for rollup #232: Both PRs modify the _copy_config_to_logs() method in PostProcessStageMixin—the referenced PR introduced copying config.yaml and sbatch_script.sh, while this PR extends it to also copy {job_id}.json.

Poem

🐰 A rabbit hops with joy today,
More artifacts now copy-way!
Job IDs tucked in JSON's fold,
Alongside configs, scripts bold—
S3 logs now richer told! 📦

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: copying job metadata JSON to the log directory for S3 uploads, which matches the primary objective of the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/srtctl/cli/mixins/postprocess_stage.py (1)

299-300: ⚠️ Potential issue | 🟠 Major

Fix container_mounts type mismatch to unblock CI.

Line 299 and Line 441 pass str mount mappings, but start_srun_process expects dict[Path, Path] | None. This is currently failing type checks in CI.

💡 Suggested fix

-                container_mounts={str(self.runtime.log_dir): "/logs"},
+                container_mounts={self.runtime.log_dir: Path("/logs")},
...
-                container_mounts={str(self.runtime.log_dir): "/logs"},
+                container_mounts={self.runtime.log_dir: Path("/logs")},

Also applies to: 441-442

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/srtctl/cli/mixins/postprocess_stage.py` around lines 299 - 300, The
container_mounts argument is being passed as str mappings but start_srun_process
requires dict[Path, Path] | None; update the calls that set container_mounts
(e.g., where container_mounts={str(self.runtime.log_dir): "/logs"}) to construct
a dict[Path, Path] by converting both source and target to Path objects (e.g.,
Path(self.runtime.log_dir) -> Path("/logs") or Path("logs") as appropriate),
ensure pathlib.Path is imported if not already, and apply the same change to the
other occurrence that sets container_mounts so the types match
start_srun_process's signature.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/srtctl/cli/mixins/postprocess_stage.py`:
- Around line 299-300: The container_mounts argument is being passed as str
mappings but start_srun_process requires dict[Path, Path] | None; update the
calls that set container_mounts (e.g., where
container_mounts={str(self.runtime.log_dir): "/logs"}) to construct a dict[Path,
Path] by converting both source and target to Path objects (e.g.,
Path(self.runtime.log_dir) -> Path("/logs") or Path("logs") as appropriate),
ensure pathlib.Path is imported if not already, and apply the same change to the
other occurrence that sets container_mounts so the types match
start_srun_process's signature.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 73af19d0-9698-4061-9c0e-15375dcbad23

📥 Commits

Reviewing files that changed from the base of the PR and between f0a303b and 7ab747e.

📒 Files selected for processing (2)

• src/srtctl/cli/mixins/postprocess_stage.py
• tests/test_postprocess.py

[kyleliang-nv]

Do you have to copy the files into /logs? Can you not also upload the files in the parent dir?

[KaunilD]

i think the original intention is to be selective about rolling up the logs and avoiding the bloat. this helps us marshal it. for example there can be a .venv or it can have dataset jsons that are irrelevant experiment tracking.

[KaunilD]

thanks for taking a look :)