feat: surface partial-run status from dropped engines (PERFNFV-464) - #652
Conversation
Completes the crucible-side piece of dropped-engine detection: rickshaw (PR#863), toolbox (PR#129), and CommonDataModel (PR#204) already track and persist partial/dropped-engines. log_result_directory() now prints a "partial: yes (N engine(s) dropped)" line alongside the existing status line whenever a loaded rickshaw-run.json has partial: true. Updates how-benchmark-execution-works.md with a new section describing when engines get dropped (per-sample vs. post-benchmark phases, and their different outcomes) and how it surfaces in rickshaw-run.json, the CLI, and CDM. Updates how-cdm-works.md's run doctype field list. Verified against real run directories: the clean run shows no partial line, the dropped-engine run shows "partial: yes (1 engine(s) dropped)".
PR Review: crucible#652 — feat: surface partial-run status from dropped engines (PERFNFV-464)Summary: Adds a Changed files: 3 Verified the code against the merged rickshaw implementation ( Documentation
File Coverage
LimitationsDid not execute the actual CLI against a live run directory (relied on the PR author's stated test plan and static code tracing). Did not verify runtime behavior of the rickshaw/CDM/toolbox pieces since those are already-merged, out-of-scope repos. VerdictApprove with comments — the code change is correct and consistent with the already-merged rickshaw/CDM work; both findings are doc-only inaccuracies/gaps that don't block merging but should be fixed (the 🤖 Generated with Claude Code |
The Document types table already listed these fields, but the Versioning table wasn't updated to match when they were added starting at v9dev, unlike the precedent set when default-aggregation was added to v10dev (both tables were updated together).
|
Thanks for the review — took the two findings in different directions per feedback: #1 ( #2 (Versioning table gap): fixed directly, 🤖 Generated with Claude Code |
atheurer
left a comment
There was a problem hiding this comment.
The Python logic looks correct and well-structured. One issue worth fixing before merge:
Documentation inaccuracy in docs/how-benchmark-execution-works.md
The new section states that both crucible ls and crucible get result surface partial status as a partial: yes (N engine(s) dropped) line. However, crucible get result routes through get-result-summary.sh (a CDM query script), not result-processor.py, and this PR makes no changes to that path. The formatted partial: yes line only appears in crucible ls output.
Either drop the crucible get result reference from that sentence, or clarify that CDM queries return the raw partial/dropped-engines fields in their own format (via CDM PR#206).
Minor items (not blockers):
- If
partial: trueever appears without adropped-engineskey, the output readspartial: yes (0 engine(s) dropped)which is self-contradictory. Addingand data.get('dropped-engines')to the guard at line 308 would prevent this. - Test plan covers
crucible ls --result-dirbut notcrucible tags ls --result-dir, thoughrun_results_tag_mode()(line 698) was also updated.
|
This issue is actually being addressed in perftool-incubator/CommonDataModel#206 |
Summary
log_result_directory()inbin/result-processor.pynow printspartial: yes (N engine(s) dropped)alongside the existingstatus:line when a run'srickshaw-run.jsonhaspartial: true.docs/how-benchmark-execution-works.mdcovering when engines get dropped, the different outcomes for per-sample vs. post-benchmark drops, and how it surfaces inrickshaw-run.json, the CLI, and CDM.docs/how-cdm-works.md's run doctype field list to includepartial/dropped-engines.Test plan
crucible ls --result-dir <clean run>— nopartial:linecrucible ls --result-dir <run with a dropped engine>— showspartial: yes (1 engine(s) dropped)python3 -m py_compile bin/result-processor.py🤖 Generated with Claude Code