Add machine-readable scenario summary report - #919
Conversation
📝 WalkthroughWalkthroughAdds ChangesSummary reporter and registry integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
||
| return [ | ||
| self._artifact(path) | ||
| for path in sorted(self.results_root.iterdir()) |
There was a problem hiding this comment.
this won't work gracefully when we run sweeping. in the sweeping case, the folders structure will be
/scenario foler/
/case 1/
/sweep 1/
/some-report.html
/sweep 2/
/some-report.html
There was a problem hiding this comment.
sweeping is actually quite important in our work. I believe this json summary report should bring insights if those were run
|
Addressed the requested changes and updated the PR branch from latest main. What changed:
Validation on the updated PR head dfa28da:
|
Generate cloudai-summary.json as a default scenario reporter so automation can discover scenario status, report artifacts, test run artifacts, and configured metrics without scraping workload-specific files. Issue: NVIDIA#917 Tested: uv run ruff check src/cloudai/reporter.py src/cloudai/registration.py src/cloudai/_core/registry.py src/cloudai/core.py tests/test_reporter.py tests/test_init.py Tested: uv run ruff format --check src/cloudai/reporter.py src/cloudai/registration.py src/cloudai/_core/registry.py src/cloudai/core.py tests/test_reporter.py tests/test_init.py Tested: uv run pytest Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
Trim the default scenario summary to the fields external tools need and group DSE/sweep outputs under their parent test run so nested sweep artifacts remain discoverable. Constraint: Reviewer requested a smaller JSON surface, a full-object regression assertion, and sweep-aware artifact reporting. Rejected: Keeping iteration, step, description, schema_version, and system metadata in the first summary contract | they widen the surface before consumers have asked for them. Confidence: high Scope-risk: narrow Directive: Treat cloudai-summary.json as an automation entry point; add fields only when they have clear consumer value. Tested: uv run ruff check src/cloudai/reporter.py tests/test_reporter.py tests/test_init.py src/cloudai/registration.py src/cloudai/_core/registry.py src/cloudai/core.py Tested: uv run ruff format --check src/cloudai/reporter.py tests/test_reporter.py tests/test_init.py src/cloudai/registration.py src/cloudai/_core/registry.py src/cloudai/core.py Tested: uv run pytest tests/test_reporter.py tests/test_init.py tests/test_registry.py -q Tested: uv run pytest -q Not-tested: Live CloudAI sweep run on an HPC system.
008e5d4 to
614712a
Compare
|
Rebased onto latest main to resolve merge conflicts (this had drifted significantly and was showing as
Re-ran |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new default scenario-level reporter that emits a machine-readable JSON summary (cloudai-summary.json) alongside existing scenario reports, intended to support automation without changing existing HTML/tarball/DSE/workload reports.
Changes:
- Added
SummaryReporterto generatecloudai-summary.jsonwith scenario/test-run status, paths, artifacts, and metrics. - Registered
summaryas a default scenario reporter (ordered beforetarball) and exported it viacloudai.core. - Updated registry ordering and extended tests to validate ordering and JSON output structure.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/cloudai/reporter.py |
Adds SummaryReporter implementation and JSON serialization logic. |
src/cloudai/registration.py |
Registers the new summary scenario reporter by default. |
src/cloudai/_core/registry.py |
Updates scenario reporter ordering so summary runs before tarball. |
src/cloudai/core.py |
Exports SummaryReporter from the public cloudai.core API. |
tests/test_reporter.py |
Adds coverage for SummaryReporter output and updates report ordering expectations. |
tests/test_init.py |
Updates default scenario reporter lists/config expectations to include summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not test_runs: | ||
| return "unknown" | ||
| if all(tr["status"] == "completed" for tr in test_runs): | ||
| return "completed" | ||
| return "failed" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/cloudai/reporter.py`:
- Around line 257-269: Remove the unnecessary intermediate summary assignment in
_sweep_test_run_summary and return the constructed dictionary directly,
preserving all existing fields and values.
- Around line 235-269: Update _scenario_status and its callers so an empty DSE
sweep remains "unknown" at the parent scenario level instead of being classified
as "failed"; preserve "unknown" for empty top-level runs, and ensure
_scenario_status propagates any nested "unknown" status before applying the
completed/failed result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 988ec2d3-4f2e-4d71-8b75-d9658003652e
📒 Files selected for processing (6)
src/cloudai/_core/registry.pysrc/cloudai/core.pysrc/cloudai/registration.pysrc/cloudai/reporter.pytests/test_init.pytests/test_reporter.py
| def _scenario_status(self, test_runs: list[dict[str, Any]]) -> str: | ||
| if not test_runs: | ||
| return "unknown" | ||
| if all(tr["status"] == "completed" for tr in test_runs): | ||
| return "completed" | ||
| return "failed" | ||
|
|
||
| def _test_runs_summary(self) -> list[dict[str, Any]]: | ||
| loaded_by_name: dict[str, list[TestRun]] = {} | ||
| for tr in self.trs: | ||
| loaded_by_name.setdefault(tr.name, []).append(tr) | ||
|
|
||
| summary: list[dict[str, Any]] = [] | ||
| for test_run in self.test_scenario.test_runs: | ||
| loaded_runs = loaded_by_name.get(test_run.name, []) | ||
| if test_run.is_dse_job: | ||
| summary.append(self._sweep_test_run_summary(test_run, loaded_runs)) | ||
| else: | ||
| summary.extend(self._test_run_summary(tr) for tr in loaded_runs) | ||
|
|
||
| return summary | ||
|
|
||
| def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]: | ||
| sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps] | ||
| summary = { | ||
| "name": tr.name, | ||
| "status": self._scenario_status(sweep_summaries), | ||
| "output_path": self._relative_path(self.results_root / tr.name), | ||
| "artifacts": self._artifacts_excluding( | ||
| self.results_root / tr.name, [sweep.output_path for sweep in sweeps] | ||
| ), | ||
| "metrics": {}, | ||
| "sweeps": sweep_summaries, | ||
| } | ||
| return summary |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"unknown" sweep status silently becomes "failed" at the parent level.
_scenario_status returns "unknown" when test_runs is empty (Line 236-237), which is correct at the top level. But _sweep_test_run_summary (Line 261) feeds sweep_summaries (empty when a DSE case has no loaded iterations yet) into the same _scenario_status, producing "unknown" for that nested entry. That nested "unknown" then propagates into the parent's all(tr["status"] == "completed" ...) check (Line 238), which treats "unknown" the same as "failed" — so a DSE case with no results yet reports the whole scenario as "failed" instead of surfacing the ambiguous state, misleading CI/automation consumers.
🐛 Proposed fix to propagate "unknown" instead of masking it as "failed"
def _scenario_status(self, test_runs: list[dict[str, Any]]) -> str:
if not test_runs:
return "unknown"
- if all(tr["status"] == "completed" for tr in test_runs):
+ if any(tr["status"] == "unknown" for tr in test_runs):
+ return "unknown"
+ if all(tr["status"] == "completed" for tr in test_runs):
return "completed"
return "failed"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _scenario_status(self, test_runs: list[dict[str, Any]]) -> str: | |
| if not test_runs: | |
| return "unknown" | |
| if all(tr["status"] == "completed" for tr in test_runs): | |
| return "completed" | |
| return "failed" | |
| def _test_runs_summary(self) -> list[dict[str, Any]]: | |
| loaded_by_name: dict[str, list[TestRun]] = {} | |
| for tr in self.trs: | |
| loaded_by_name.setdefault(tr.name, []).append(tr) | |
| summary: list[dict[str, Any]] = [] | |
| for test_run in self.test_scenario.test_runs: | |
| loaded_runs = loaded_by_name.get(test_run.name, []) | |
| if test_run.is_dse_job: | |
| summary.append(self._sweep_test_run_summary(test_run, loaded_runs)) | |
| else: | |
| summary.extend(self._test_run_summary(tr) for tr in loaded_runs) | |
| return summary | |
| def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]: | |
| sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps] | |
| summary = { | |
| "name": tr.name, | |
| "status": self._scenario_status(sweep_summaries), | |
| "output_path": self._relative_path(self.results_root / tr.name), | |
| "artifacts": self._artifacts_excluding( | |
| self.results_root / tr.name, [sweep.output_path for sweep in sweeps] | |
| ), | |
| "metrics": {}, | |
| "sweeps": sweep_summaries, | |
| } | |
| return summary | |
| def _scenario_status(self, test_runs: list[dict[str, Any]]) -> str: | |
| if not test_runs: | |
| return "unknown" | |
| if any(tr["status"] == "unknown" for tr in test_runs): | |
| return "unknown" | |
| if all(tr["status"] == "completed" for tr in test_runs): | |
| return "completed" | |
| return "failed" |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 269-269: Unnecessary assignment to summary before return statement
Remove unnecessary assignment
(RET504)
🤖 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 `@src/cloudai/reporter.py` around lines 235 - 269, Update _scenario_status and
its callers so an empty DSE sweep remains "unknown" at the parent scenario level
instead of being classified as "failed"; preserve "unknown" for empty top-level
runs, and ensure _scenario_status propagates any nested "unknown" status before
applying the completed/failed result.
| def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]: | ||
| sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps] | ||
| summary = { | ||
| "name": tr.name, | ||
| "status": self._scenario_status(sweep_summaries), | ||
| "output_path": self._relative_path(self.results_root / tr.name), | ||
| "artifacts": self._artifacts_excluding( | ||
| self.results_root / tr.name, [sweep.output_path for sweep in sweeps] | ||
| ), | ||
| "metrics": {}, | ||
| "sweeps": sweep_summaries, | ||
| } | ||
| return summary |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Unnecessary assignment before return (Ruff RET504).
♻️ Proposed fix
def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]:
sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps]
- summary = {
+ return {
"name": tr.name,
"status": self._scenario_status(sweep_summaries),
"output_path": self._relative_path(self.results_root / tr.name),
"artifacts": self._artifacts_excluding(
self.results_root / tr.name, [sweep.output_path for sweep in sweeps]
),
"metrics": {},
"sweeps": sweep_summaries,
}
- return summary📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]: | |
| sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps] | |
| summary = { | |
| "name": tr.name, | |
| "status": self._scenario_status(sweep_summaries), | |
| "output_path": self._relative_path(self.results_root / tr.name), | |
| "artifacts": self._artifacts_excluding( | |
| self.results_root / tr.name, [sweep.output_path for sweep in sweeps] | |
| ), | |
| "metrics": {}, | |
| "sweeps": sweep_summaries, | |
| } | |
| return summary | |
| def _sweep_test_run_summary(self, tr: TestRun, sweeps: list[TestRun]) -> dict[str, Any]: | |
| sweep_summaries = [self._test_run_summary(sweep) for sweep in sweeps] | |
| return { | |
| "name": tr.name, | |
| "status": self._scenario_status(sweep_summaries), | |
| "output_path": self._relative_path(self.results_root / tr.name), | |
| "artifacts": self._artifacts_excluding( | |
| self.results_root / tr.name, [sweep.output_path for sweep in sweeps] | |
| ), | |
| "metrics": {}, | |
| "sweeps": sweep_summaries, | |
| } |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 269-269: Unnecessary assignment to summary before return statement
Remove unnecessary assignment
(RET504)
🤖 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 `@src/cloudai/reporter.py` around lines 257 - 269, Remove the unnecessary
intermediate summary assignment in _sweep_test_run_summary and return the
constructed dictionary directly, preserving all existing fields and values.
Source: Linters/SAST tools
| with report_path.open("w") as f: | ||
| json.dump(self.build_summary(), f, indent=2) | ||
| f.write("\n") |
There was a problem hiding this comment.
a bit of nitpicking but let's make this code a bit more modern
summary = self.build_summary()
report_path.write_text(json.dumps(summary, indent=2))| return { | ||
| "scenario": self.test_scenario.name, | ||
| "status": self._scenario_status(test_runs), | ||
| "result_dir": self._relative_path(self.results_root), |
There was a problem hiding this comment.
why did you decide to make it relative? looking at your unit test I see
assert summary == {
"scenario": "test_scenario",
"status": "completed",
"result_dir": ".",
....
}What's the value in "result_dir": "."? I would go for absolute value here
| summary = { | ||
| "name": case_name(tr), | ||
| "status": "completed" if status.is_successful else "failed", | ||
| "output_path": self._relative_path(tr.output_path), |
There was a problem hiding this comment.
speaking about all the parths in the report:
- I would say the overall scenario path (result_dir) should be absolute
- all the other paths (per test run) should be relative to that result_dir path
| SUMMARY_FILE_NAME = "cloudai-summary.json" | ||
|
|
||
| def generate(self) -> None: | ||
| self.load_test_runs() |
There was a problem hiding this comment.
please research how this function works. apparently your implementation completely ignores the iterations feature (run one case multiple times). some people do fine it useful (when a workload is not consistent thus one runs it multiple times and calculates avg/max/min for metrics).
there are a few existing issues however:
- iterations aren't supported with sweeping
load_test_runsiterates wrongly over dse vs iters (issue number 1. though makes this bug not reproducible in real runs)load_test_runsdoesn't group test runs per iteration- there's no mechanism to select one output metric per set of iterations per test-case/per sweep
all in all, this reporter you work on is something we truly need + we have an internal initiative to implement one. however it would take way more to make it well and the second we release it - we will have to maintain backwards compatibility for long
with a heavy heart, maybe it would be better to park this PR for now... yes, you could make the first step but I'll likely will stay defensive and unsure because of our internal plans for the near future and all the ground work needed to be done
Summary
summaryscenario reporter that writescloudai-summary.jsonnext to the existing scenario reports.tarballas the final scenario reporter.Fixes #917.
Test Plan
Environment:
uv runCommands and results:
Additional Notes