feat: append JobResult timing and status to job_metadata.json#230
feat: append JobResult timing and status to job_metadata.json#230Abdelsalam-Abbas wants to merge 3 commits intomainfrom
Conversation
Closes #212. The metadata file now carries started_at, finished_at, runtime_seconds, status, and exit_code in addition to the GuidanceConfig snapshot, by merging asdict(job_result) on top after run_guidance finishes. save_everything still writes the GuidanceConfig-only snapshot as before, so a crash inside _run_guidance does not lose context.
JobResult carries output_dir and log_path that, on Docker runs, are container-internal paths. Without remapping, merging the JobResult dict on top of GuidanceConfig.as_dict() in job_metadata.json overwrites the already-remapped host paths with container paths, regressing the host-paths invariant. Add JobResult.as_dict() (mirroring GuidanceConfig.as_dict) so both halves of the merge produce host-remapped paths, and switch the helper to use it instead of dataclasses.asdict. Adds a regression test.
AGENTS.md mandates NumPy-style docstrings. JobResult.as_dict and _write_job_metadata now have Parameters/Returns sections matching the convention used elsewhere in this file (e.g., save_everything).
📝 WalkthroughWalkthroughThis PR adds metadata merging capability to the job execution pipeline. A new ChangesMetadata Merging and Persistence
Sequence DiagramsequenceDiagram
participant RG as run_guidance()
participant JR as JobResult
participant GC as GuidanceConfig
participant WM as _write_job_metadata()
participant FS as File System
RG->>JR: Construct JobResult (success/failure)
JR->>JR: Capture timing & status
RG->>WM: Call _write_job_metadata(output_dir, args, job_result)
WM->>GC: Call args.as_dict()
GC-->>WM: Return config metadata dict
WM->>JR: Call job_result.as_dict()
JR-->>WM: Return result dict (with remapped paths)
WM->>WM: Merge both dicts
WM->>FS: Create output_dir if missing
WM->>FS: Write merged metadata to job_metadata.json
WM-->>RG: Complete
RG-->>RG: Return JobResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/test_guidance_script_utils.py (1)
9-10: ⚡ Quick winPrefer validating metadata through a public entry point, not
_write_job_metadatadirectly.These tests are behavior-focused, but they’re still coupled to a private helper. Consider keeping one helper-level check at most and shifting the main assertions to a public path (e.g.,
save_everything/run_guidanceoutput behavior) to reduce brittleness during refactors.As per coding guidelines: "
**/test_*.py: Test public interfaces with expected behaviors."Also applies to: 64-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/test_guidance_script_utils.py` around lines 9 - 10, Tests currently call the private helper _write_job_metadata directly; change them to validate behavior via the public API (e.g., call save_everything or the run_guidance equivalent) and assert on observable outputs instead of internal state. Keep at most one helper-level assertion if necessary (convert any remaining _write_job_metadata direct checks into a single, minimal unit test that documents helper behavior), but move all main assertions that inspect metadata files, return values, or side-effects to tests that invoke save_everything/run_guidance and verify the public outputs/side-effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/utils/test_guidance_script_utils.py`:
- Around line 9-10: Tests currently call the private helper _write_job_metadata
directly; change them to validate behavior via the public API (e.g., call
save_everything or the run_guidance equivalent) and assert on observable outputs
instead of internal state. Keep at most one helper-level assertion if necessary
(convert any remaining _write_job_metadata direct checks into a single, minimal
unit test that documents helper behavior), but move all main assertions that
inspect metadata files, return values, or side-effects to tests that invoke
save_everything/run_guidance and verify the public outputs/side-effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c7586d9-ec6e-42f6-9301-6900d1454f3c
📒 Files selected for processing (3)
src/sampleworks/utils/guidance_script_arguments.pysrc/sampleworks/utils/guidance_script_utils.pytests/utils/test_guidance_script_utils.py
Summary
job_metadata.jsonnow includesstarted_at,finished_at,runtime_seconds,status, andexit_codefromJobResultin addition to the existingGuidanceConfigsnapshot.JobResult.as_dict()mirroringGuidanceConfig.as_dict()sooutput_dirandlog_pathget container-to-host remapping. Without this, the merged file would regress to container paths in Docker runs._write_job_metadatais called both insidesave_everything(config-only snapshot, in case the run crashes later) and at the end ofrun_guidance(enriched withJobResult, on both success and failure paths).Test plan
pixi run -e boltz-dev cpu-tests tests/utils/test_guidance_script_utils.py— 5 tests pass, including the newtest_write_job_metadata_remaps_job_result_paths_to_hostregression test that setsSAMPLEWORKS_HOST_RESULTS_DIRand asserts JobResult paths come out host-remapped.job_metadata.jsonnow contains both the GuidanceConfig fields and the JobResult timing/status fields.Summary by CodeRabbit
New Features
Refactor
Tests