Feat/array summaries - #93
Open
ovalerio wants to merge 8 commits into
Open
Conversation
Append a summary after each job array (behind --array-summary), giving at-a-glance insight into array progress and efficiency. Default output is unchanged: with the feature disabled the rendered rows are byte-identical to before, and summaries are suppressed in --parsable mode. Each summary reports: - completion progress (completed/total, percent) and per-state task counters - min / mean / max for the displayed efficiency columns (completed tasks only) - total accumulated task-time and per-state mean runtime - shared values shown once vs. differing values listed (e.g. multiple partitions) - compute nodes collapsed into Slurm-style hostlist ranges (e.g. somacpu[001-088,101-124],somagpu[001-093]) - a low-mean-efficiency marker, reusing the existing click color thresholds An optional runtime histogram (minutes) is drawn for arrays larger than a threshold, using Unicode block bars with an ASCII fallback for non-UTF output; a compact one-line sparkline is available as an alternative. New CLI options: - --array-summary: enable the summary block - --array-summary-hist: add a runtime histogram for large arrays - --array-summary-hist-min-tasks (default 50): histogram size threshold - --array-summary-sparkline: render a one-line sparkline instead of a histogram Implementation: - new src/reportseff/array_summary.py: grouping, completed-only aggregation, hostlist expand/compact, and histogram/sparkline builders (no color, no circular import with the renderer) - output_renderer.py: RenderOptions fields and a colored summary block that reuses render_eff; block rendering only when enabled - console.py: new flags; get_implementation now takes a RenderOptions - parameters.py: four new fields - README.md: documents the new options Tests: - tests/test_array_summary.py: grouping, stats, hostlist expand/compact, histogram binning/bars, sparkline, duration formatting - tests/test_output_renderer.py: summary on/off (unchanged when off), ordering after each array, parsable suppression, histogram threshold, sparkline - tests/test_reportseff.py: end-to-end CLI wiring
…tover Foundation work for the `summarize` subcommand, with no behavior change to the default `reportseff <jobs>` invocation (verified byte-identical output against baseline for a representative query). - Migrate the CLI entry point from a single click.command() to a click.group() (DefaultGroup), with a default-command shim so bare `reportseff <jobs>` / `reportseff --format=... jobs` keep working without a subcommand keyword. `--version` moves to the group level; `--help` at the top level now lists subcommands, while `reportseff report --help` reproduces the original detailed options text. Entry point in pyproject.toml updated to `console:cli`. - Extract the "query sacct -> build/filter job collection" pipeline out of get_jobs() into fetch_job_collection(), reusable by both `report` and the future `summarize` command. - Hard-remove the legacy --array-summary / --array-summary-hist / --array-summary-hist-min-tasks / --array-summary-sparkline flags and their ReportseffParameters/RenderOptions fields and rendering code path, per the agreed hard-cutover decision -- as if they never existed. array_summary.py (the underlying aggregation/rendering engine) is untouched and will be re-plumbed onto `summarize` in Phase 1. - Update/add tests accordingly: remove now-obsolete array-summary rendering tests, add a hard-cutover check that the old flags are rejected, and add DefaultGroup dispatch coverage. - Trim the now-stale --array-summary* section from README.md; the full `summarize` documentation lands in Phase 4. Drafted with AI assistance (Claude) for the click.Group dispatch boilerplate and the mechanical option/field removal; reviewed by the requesting engineer before merge.
…d core Adds the `summarize` subcommand: array/name grouping, per-task rows plus a summary block per group, and runtime graphing (sparkline/histogram), built on Phase 0's shared query pipeline and reusing array_summary.py's aggregation/rendering engine unchanged. - parameters.py: SummarizeParameters, plus a new BaseQueryParameters shared by both ReportseffParameters and SummarizeParameters so the "querying slurm" field list isn't duplicated. Validates --graph-format eagerly in __post_init__. - console.py: shared_query_options decorator factors the ~15 options common to `report` and `summarize` out of two separate @click.option chains. `summarize` adds --group-by (array/name, default array), --graph-style (sparkline/histogram/none), --graph-format (comma- separated, case-insensitive), --min-tasks (default 50), and --ascii-fallback. - array_summary.py: group_jobs_by_array refactored onto a shared _group_jobs_by_key helper; adds group_jobs_by_name (falls back to a task's own base id when JobName wasn't captured -- e.g. a PENDING job, whose fields are mostly uncached by Job.update() -- rather than merging unrelated tasks under an empty key). Adds parse_graph_format and GRAPH_FORMAT_VOCABULARY = runtime/cpueff/memeff/energy/gpueff/ gpumem (energy, not power; gpu metrics included, per the agreed plan). - output_renderer.py: add_required_column() lets `summarize --group-by name` pull JobName into the sacct query without displaying it. format_grouped_summary() is the new rendering entry point, re-plumbing the array-summary block logic Phase 0 removed onto the new command's options instead of the deleted legacy RenderOptions fields. Only `runtime` currently draws a graph -- extending --graph-format to the other vocabulary entries needs raw per-task series retained for them, which is Phase 3, not this one; requesting them is still validated, they just don't graph yet (marked with a TODO at the call site). Also fixes a gap found during verification: the summary block wasn't being suppressed in --parsable mode (per-task rows now correctly remain parsable while the decorative block is dropped, matching the original prototype's own safeguard). - Tests: group_jobs_by_name (including the fallback/mixed-availability cases) and parse_graph_format in test_array_summary.py; format_grouped_summary and add_required_column end-to-end in test_output_renderer.py; full `summarize` CLI coverage (both grouping modes, graph-style/format/min-tasks/ascii-fallback, parsable suppression, bad --graph-format rejection, singleton jobs) in test_reportseff.py. Drafted with AI assistance (Claude) for the CLI/renderer scaffolding and the bulk of the test suite; reviewed by the requesting engineer before merge.
…block
Replaces the per-metric bullet/middot line ("CPUEff: min 90.0 . mean
95.0 . max 99.0") with an aligned Metric/Min/Mean/Max table, per
troycomi's original review comment, reusing ColumnFormatter's width/
alignment/coloring machinery instead of hand-rolled string joins --
the same approach the main report table already uses, so summarize's
output now shares its visual language.
- _format_metric_line replaced by _format_metrics_table, which builds
four ColumnFormatter instances (Metric left-aligned, Min/Mean/Max
right-aligned), sizes each from its own header/cell contents, and
reuses format_title()/format_entry() for rendering -- no new parsing
or alignment logic.
- Drops the separate "(low)"/warning-glyph marker that flagged
below-threshold values in the old line format. The color coding
(render_eff, same thresholds, same as the main report table) already
flags this per-cell -- now on each of min/mean/max independently
rather than a single mean-based check -- which is what troycomi's
review comment on the original prototype asked for. If this reads
as a step too far, it's a one-line-per-cell change to reinstate.
- No unicode is introduced by the table (no box-drawing characters,
same as the main report table's own style), so --ascii-fallback
needs no table-specific handling; the existing bullet/histogram
ascii_only behavior elsewhere in the block is unaffected.
- Total task-time already used format_duration() as of Phase 1 (found
during that phase's verification pass), so nothing to change there.
- Tests: table header/alignment (ANSI-stripped width comparison, since
colored cells add invisible escape bytes), confirms the old inline
"TITLE: min X . mean Y . max Z" text is gone, confirms no extra "low"
text/glyph, empty-metrics edge case. Two Phase 1 tests that asserted
on the old "CPUEff:" text are updated to match the new table cells.
Drafted with AI assistance (Claude) for the table-rendering rewrite and
its tests; reviewed by the requesting engineer before merge.
test_format_metrics_table_low_efficiency_uses_color_not_extra_text called _summary_job(..., total_cpu=...), but the helper hard-coded TotalCPU to "00:09:00" and never exposed it as a parameter -- caught by the real pytest run (TypeError: unexpected keyword argument 'total_cpu'), not by my own verification, which had drifted into using a hand-retyped local reimplementation of the helper instead of the actual committed one. Adds the missing parameter, matching the pattern test_array_summary.py's _make_job already uses. Re-verified this time by extracting the actual function source via ast from the committed file and executing it directly, rather than retyping it -- to close this specific class of gap, not just this one instance of it. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.
…tric Generalizes graphing beyond the runtime special case: --graph-format cpueff/memeff/energy/gpueff/gpumem now actually draw a sparkline or histogram, not just get summarized in the metrics table. Validation and --ascii-fallback wiring were already in place as of Phase 1; this closes the remaining gap the plan called out explicitly. - array_summary.py: MetricStat gains a `values: list[float]` field, populated in build_array_summary alongside min/mean/max, so any summarized metric's raw per-completed-task series is retained (not just runtime's, via the pre-existing elapsed_minutes). Backward compatible -- defaults to an empty list for direct construction. render_histogram gains a `label` parameter (default "Runtime", so every existing call site/test is unaffected) so the same renderer can be reused for any metric's histogram, not just runtime's. - output_renderer.py: the runtime-only graph dispatch in _format_summary_block is replaced by a loop over every metric named in --graph-format, extracted into a new _format_metric_graph helper shared between the sparkline and histogram code paths. Percent-style metrics (cpueff/memeff/gpueff/gpumem) get a "%" unit; Energy gets "J" (Joules, per Slurm's TRES energy accounting convention); everything still respects --min-tasks, --graph-style=none, and --ascii-fallback uniformly, not just for runtime. - Tests: MetricStat.values retention, render_histogram's label/unit handling (including the empty-unit-omits-parens case), multi-metric graphing (min-tasks gating, graph-style=none, ascii-fallback, and sparkline mode all applied across several simultaneously-graphed metrics), and Energy's Joules unit specifically. Two tests whose premise Phase 3 made obsolete (asserting a non-runtime metric *doesn't* graph) are rewritten to assert the new, correct behavior. Drafted with AI assistance (Claude) for the generalization and its tests; reviewed by the requesting engineer before merge.
Completes the array-summary feature per the implementation plan: user docs for `summarize`, the last testing-strategy item not yet covered by an earlier phase, and a commit-hygiene note for contributors. - README.md: new "The `summarize` Subcommand" section under Usage, covering --group-by/--graph-style/--graph-format/--min-tasks/ --ascii-fallback, with three real, freshly-captured examples (array grouping, name grouping, histogram style) rather than hand-typed ones, matching how the rest of the README documents behavior. Also adds the AI-assistance-disclosure + small-commits note (agreed earlier in this project's design discussion) to the Contributions section, since there's no separate CONTRIBUTING.md to put it in. - Tests: the plan's testing strategy explicitly called for "graceful no-graph behavior for gpueff/gpumem when jobstat-caching data isn't present" -- this was implicitly correct already (an all-"---" column never accumulates a MetricStat to graph) but wasn't exercised by any existing test. Added at both the renderer level (test_output_renderer.py) and the CLI level (test_reportseff.py): requesting --graph-format gpueff/gpumem on non-GPU jobs succeeds, draws nothing, and doesn't otherwise disturb the summary. Everything else the plan asked Phase 4 to cover -- name-grouping and raw-series-retention tests, click.group dispatch and hard-cutover tests -- already landed in Phases 0, 1, and 3 respectively, as unit tests were written alongside each phase rather than deferred here. Drafted with AI assistance (Claude) for the README section and the GPU-absence tests; reviewed by the requesting engineer before merge.
Owner
|
Let me know if you want me to review this or wait until the checks are all passing. |
Contributor
Author
|
Hello @troycomi👋 I'm on vacation and I've already started with the cleanup (coverage & lint issues). Of course you can start looking at it if you have time. I'm not planning to do any big changes just fixing the CI stuff that is currently preventing the merge. Then I will sent you a review request when I get back from vacation. In around a week time. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An array summaries new feature for
reportsefffollowing the feedback from @troycomi and @biermanr here: #92The concept is mine and I've refined, implemented and regression test the new feature using and Agentic coding website (claude.ai/Sonnet 5).