Feat/array summaries by ovalerio · Pull Request #93 · troycomi/reportseff

ovalerio · 2026-08-03T11:39:54Z

An array summaries new feature for reportseff following the feedback from @troycomi and @biermanr here: #92

The concept is mine and I've refined, implemented and regression test the new feature using and Agentic coding website (claude.ai/Sonnet 5).

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.

Under strict runtime type checking (pytest --typeguard-packages= reportseff, the project's "typed-tests" task), every `summarize` invocation failed with: TypeCheckError('is not an instance of reportseff.parameters.ReportseffParameters') fetch_job_collection was typed to take a ReportseffParameters, but get_summary() passes it a SummarizeParameters -- a sibling class under BaseQueryParameters, not a subtype of ReportseffParameters. The function only ever touches fields both classes inherit unchanged from BaseQueryParameters (slurm_format, state, since/until, partition, cluster, extra_args, user, jobs, debug, array_min_size), so retyping it against that shared base is correct, not just a workaround -- it matches what the function actually requires from its caller. This also would have failed under `mypy --strict` for the same reason. Verified without typeguard installed (no network access in this sandbox) by reproducing its core check directly: comparing typing.get_type_hints() against isinstance() for both parameter classes before and after the fix. Also fixes a docstring line left over from an earlier refactor, claiming this function sorts jobs -- it doesn't; sorting happens in the caller. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

Addresses everything `poe lint` (ruff check) flagged across the array-summary work: - PLR0913 (too many arguments): render_histogram and format_grouped_summary were missing the `# noqa: PLR0913` suppression already used elsewhere in this codebase for functions whose argument count is a deliberate, reviewed choice (matching _format_summary_block and _format_metric_graph). - D403 (docstring should start capitalized): console.py's cli() docstring, and three test docstrings starting with "summarize"/ "report's". Fixed by capitalizing, same as `ruff --fix` would do. - D205 (missing blank line between docstring summary and description): one multi-line test docstring reflowed into a proper summary + blank line + description. - PLC0415 (import not at top-level): a `from reportseff import __version__` inside test_default_group_version's body is moved to the file's top-level imports. No functional changes -- confirmed by rerunning every test whose file was touched. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

pyproject.toml enforces `fail_under = 100` with branch coverage on (`[tool.coverage.run] branch = true`), so both of these -- while functionally harmless -- would fail the coverage-tests task: - DefaultGroup.parse_args: `if not args or (...)` has two ways to be True, but every existing test hit the second ("first token isn't a subcommand name") without ever calling `reportseff` with zero arguments, leaving the `not args` branch unexercised. - OutputRenderer._format_metric_graph: its `if not values: return []` guard protects min()/max() in the sparkline branch from a future caller that doesn't check first, but both of the two current call sites in _format_summary_block already skip calling it with an empty series, so the guard itself was unreachable through format_grouped_summary. Tested directly instead, same as _format_metrics_table's analogous empty-input guard already is. Both found by reviewing the new code for likely gaps rather than by running coverage directly (no network access in this sandbox to install pytest-cov) -- please run `poe coverage-tests` to confirm these were the only two, since this is inferred from code review, not measured. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

…rize options.

troycomi · 2026-08-12T13:02:36Z

Let me know if you want me to review this or wait until the checks are all passing.

ovalerio · 2026-08-12T23:38:11Z

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!

The previous coverage commit was based on code review (no network in that sandbox to run pytest-cov); this one is based on the actual `poe coverage-tests` report, which found more gaps than review alone caught -- 98.59% total, missing: array_summary.py 196, 207, 217-218, 320, 323, 342->344, 365, 558 console.py 313 output_renderer.py 361->363, 369->377, 377->386, 387-390, 400 array_summary.py -- all pre-existing helpers, untouched by any prior phase, direct-tested rather than routed through the full pipeline: - _coerce_float: bool (excluded despite bool being an int subclass) and other non-numeric, non-string types. - _elapsed_seconds: an unparseable Elapsed value. Job.update() itself validates Elapsed on construction, so the malformed value is set via other_entries directly post-construction to reach this function's own defensive handling of it. - _summarize_nodes: a mix of valid/invalid NodeList entries, and the case where every entry is invalid (returns None). - _split_top_level / expand_hostlist: an empty string, a trailing comma, and an empty token between commas. - render_sparkline: its max_count<=0 guard can't actually be produced by compute_histogram for non-empty input (every value lands in some bin, so bin counts always sum to len(values)) -- same situation as _format_metric_graph's analogous guard from the last commit, tested the same way, via a patched compute_histogram. console.py -- summarize's pager branch (>20 entries) had no test; report's did. Mirrors test_long_output exactly, against format_grouped_summary instead of format_jobs. output_renderer.py -- three _format_summary_block/format_grouped_summary paths with no rendering-level coverage: - An empty task list (only reachable directly -- is_array_group always gates on >=1 real task in normal dispatch) skips the state-counters segment instead of appending an empty one. - Every task having an unparseable Elapsed skips both the total task-time and mean runtime lines, rather than showing a misleading zero. - A constant categorical column (e.g. Partition) renders a "Shared:" line, and NodeList renders a collapsed "Nodes:" line -- both were covered at the build_array_summary computation level already (in test_array_summary.py) but never at the rendering level. Verified by re-executing the actual committed test bodies via ast extraction, same as every prior phase; still can't run pytest-cov itself in this sandbox (no network access), so please confirm with `poe coverage-tests` that this reaches 100%. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

…agged by pip-audit (PYSEC-2026-2132).

Two independent issues from a `poe checks` run against the full branch: - ruff FBT003 (boolean positional value in a call): the literal True/False passed to _coerce_float in test_coerce_float_rejects_bool IS the test input, not an ambiguous flag argument to some other API -- _coerce_float takes a single positional value, so there's no keyword-argument alternative that would satisfy the rule's actual intent. Suppressed with a per-line noqa, same as this project's existing PLR0913 suppressions for similar deliberate, reviewed exceptions. - mypy arg-type: test_compute_histogram_basic assigns an int literal list to a variable, then passes it to compute_histogram(values: list[float]). Since list is invariant, this only works when mypy can apply bidirectional inference to a literal passed directly as an argument -- once it's behind a variable, list[int] no longer satisfies list[float]. This test predates this branch entirely (it was already in the uploaded prototype, untouched by any phase or prior CI-fix commit) -- first surfaced now because this is the first time mypy --strict has actually been run against it. Fixed to build float values directly, matching the [float(i) for i in range(...)] style already used by every other test in this section. Both re-verified by re-executing the actual test bodies; still can't run mypy/ruff themselves in this sandbox (no network access), so please confirm with `poe checks` -- and since it's a sequence that stops at the first failure, note that `audit`/`mypy`/`pytype` may each still turn up their own findings once `lint` finally gets past them cleanly. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

old-click-tests (installs click==6.7 in an isolated env specifically to test this project's declared minimum, click>=6.7) failed at import time: TypeError: Choice.__init__() got an unexpected keyword argument 'case_sensitive' click.Choice's `case_sensitive` constructor argument was added in a later click release than 6.7. This isn't related to the recent click version bump for the pip-audit fix -- that only affects the normally pinned/locked version; old-click-tests has always overridden it to exactly 6.7 regardless, via its own `executor = {with = ["click==6.7"]}`. This has been a latent incompatibility since Phase 1 introduced --group-by and --graph-style; it was never reached before now because every earlier `poe tests` run had something else (coverage, then lint, then mypy) aborting the sequence first. Fixed with a small _CaseInsensitiveChoice(click.Choice) subclass that lower-cases the input in convert() before deferring to the normal Choice validation, rather than relying on a constructor argument that not all supported click versions have. Works identically across every click version from 6.7 up, and --help/validation-error text is unaffected since choices are still declared and displayed normally. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

…compat fix Two small issues surfaced by running mypy and coverage against the previous commit's _CaseInsensitiveChoice fix: - mypy --strict (disallow_any_generics): click.Choice is generic, parameterized by the choice value type. `class _CaseInsensitiveChoice(click.Choice):` needs to be `click.Choice[str]` -- purely a type-checking annotation, no runtime behavior change (confirmed by re-running the same case-insensitivity checks). - coverage: convert()'s `if isinstance(value, str)` guard had its False branch (a non-string value skipping the lower-case step) unexercised -- real CLI usage always hands convert() a string, so this is defensive against a value arriving pre-converted through some other click code path, same situation as a couple of guards from the last coverage-gap commit. Tested directly by calling convert() with a non-string value and confirming it still falls through to the normal BadParameter rejection. Drafted with AI assistance (Claude); reviewed by the requesting engineer before merge.

… with click 6.7 while preserving conversion behavior. Using a version of click >= 8.3.3 is strongly advised and enforced also in the pyproject.toml (security audit).

troycomi

Good work. Left some (probably opinionated) comments. Happy to discuss further.

troycomi · 2026-09-09T17:07:52Z

+#: The title (case-insensitive) whose values are collapsed into a hostlist.
+_NODELIST_TITLE = "nodelist"
+#: State considered "completed" for completed-only efficiency statistics.
+COMPLETED_STATE = "COMPLETED"


Not sure this warrants a variable, pretty sure it's hard coded throughout

Removed _NODELIST_TITLE / COMPLETED_STATE module constants; inline the literals "nodelist" / "COMPLETED" at each call site.

troycomi · 2026-09-09T17:11:36Z

+    seen: set[str] = set()
+    result: list[str] = []
+    for token in tokens:
+        if token not in seen:
+            seen.add(token)
+            result.append(token)


Since you already use the sorted({}) comprehension above, I'm wondering why it uses this format here to remove duplicates?

Yes, that was done to remove duplicates. Rewriting this to use the sorted({..}) comprehension idiom.

Also updated the test_parse_graph_format_basic and test_parse_graph_format_empty_tokens_ignored to expect the new (sorted) order.

troycomi · 2026-09-09T17:16:06Z

+    specifically -- by a single task's jobid already showing array syntax
+    (e.g. a lone ``123_1``).
+    """
+    return len(tasks) >= _MIN_ARRAY_TASKS or any("_" in job.jobid for job in tasks)


Wondering about the choice to apply the min tasks filter to only non-array jobs. Seems less surprising to also use that to filter array jobs?

troycomi · 2026-09-09T17:29:39Z

+    return compact_hostlist(names)
+
+
+def _split_top_level(value: str) -> list[str]:


Take another go at this, I'm sure a regex can do the job here. Also don't think you handle ill-formed hostlists?

Agreed, using a regex split: re.split(r",(?![^\[]*\])", value)
Added tests for ill-formed hostlists (unbalanced brackets, non-numeric ranges).

troycomi · 2026-09-10T16:19:29Z

+        metric_col.width = max(len(metric_col.title), *(len(r[0]) for r in rows)) + 2
+        min_col.width = max(len(min_col.title), *(len(r[1][0]) for r in rows)) + 2
+        mean_col.width = max(len(mean_col.title), *(len(r[2][0]) for r in rows)) + 2
+        max_col.width = max(len(max_col.title), *(len(r[3][0]) for r in rows)) + 2


I thought column formatter does this automatically, but am foggy on the details. If this is a better implementation you can port it to the formatter code, otherwise I'd prefer the existing mechanism to eliminate code duplication.

There is one called compute_width. On a first glance is a yes, But the type of arguments, wouldn't agree unless doing some cast or that kind of trick. In any case to avoid the duplication, it's now out as a little helper function and documenting it's intended use.

troycomi · 2026-09-10T16:21:51Z

+from reportseff.job import Job
+
+
+def _make_job(  # noqa: PLR0913


I'd replace this list with kwargs for at least some of the parameters, then .get with a default if they aren't set. Also curious about the position of your * for keyword only arguments

good idea, Changed that to use kwargs plus some job default values.

troycomi · 2026-09-10T16:23:10Z

+
+
+def test_group_jobs_by_name_preserves_order() -> None:
+    """Grouping bundles tasks by JobName in first-appearance order."""


I think you'd need to also test that swapping lines 77 and 78 change the order for this to be true.

Added a second case with the two input jobs' order swapped.

troycomi · 2026-09-10T16:24:41Z

+    )
+
+
+def test_parse_graph_format_empty_tokens_ignored() -> None:


Is this also handled in the format token of reportseff already? If not should be added

troycomi · 2026-09-10T18:36:42Z

+```txt
+reportseff summarize 34473805
+
+34473805_1    COMPLETED       01:28:00    86.6%    34.4%
+34473805_2    COMPLETED       01:22:00    88.4%    30.7%
+34473805_3    COMPLETED       01:29:00    86.4%    34.4%
+...
+Array 34473805  •  60/60 completed (100%)  •  COMPLETED 60
+  Metric      Min   Mean    Max
+  CPUEff    85.4%  91.4%  98.6%
+  MemEff    29.8%  32.8%  35.8%
+  Total task-time (wall-clock): 85h40m across 60 tasks
+  Mean runtime: COMPLETED 1h25m
+  Runtime dist: ▅▃▄▃▄▄▃█▅▄ (n=60, 78-92 min)
+  CPUEff dist: █▅▇▆▂▆▅▅▃▆ (n=60, 85-99%)
+  MemEff dist: ▆▄█▄▆▄▅▆▆▆ (n=60, 30-36%)
+```


Looking only at the summaries saves me already a lot of scrolling in the morning.
In particular with the larger job arrays. =)

Array 39482941 • 3937/3937 completed (100%) • COMPLETED 3937 Metric Min Mean Max TimeEff 3.6% 7.0% 10.3% CPUEff 12.2% 14.1% 19.5% MemEff 36.0% 41.7% 46.3% ReqCPUS 12.0 12.0 12.0 Total task-time (wall-clock): 276-02:22:03 across 3937 tasks Mean runtime: COMPLETED 01:40:59 Shared: ReqMem=175000M, Partition=GPU, JobName=mutex_watershed_mag Nodes: somagpu[001-093] Runtime dist: ▂▃▃▃▇▇█▆▂▁ (n=3937, 52-148 min) CPUEff dist: ▃█▆▃▂▂▂▁▁▁ (n=3937, 12-20%) MemEff dist: ▁▂▂▄▆█▇▆▃▁ (n=3937, 36-46%) Array 39483442 • 1103/1231 completed (90%) • COMPLETED 1103 • RUNNING 128 Metric Min Mean Max TimeEff 5.4% 7.7% 9.6% CPUEff 12.3% 13.5% 16.2% MemEff 38.4% 41.7% 45.2% ReqCPUS 12.0 12.0 12.0 Total task-time (wall-clock): 92-12:39:33 across 1231 tasks Mean runtime: COMPLETED 01:50:15 • RUNNING 01:30:55 Shared: ReqMem=175000M, Partition=GPU, JobName=mutex_watershed_mag Nodes: somagpu[001-093] Runtime dist: ▂▃▃▅█▆▇▇▄▂ (n=1231, 72-139 min) CPUEff dist: ▂█▇▆▅▃▂▂▁▁ (n=1103, 12-16%) MemEff dist: ▂▂▇▇█▆▆▆▃▁ (n=1103, 38-45%)

…, and array-group detection - Remove _NODELIST_TITLE/COMPLETED_STATE constants in favor of inline literals, matching how "COMPLETED" is already hardcoded elsewhere. - Simplify parse_graph_format's dedup to the sorted-set idiom already used for categorical aggregation (its result is only ever used as a set downstream). - Replace _split_top_level's manual bracket-depth counting with a regex split; ill-formed hostlists (unclosed brackets, non-numeric ranges) now degrade to a verbatim token instead of relying on untested manual state tracking. - Document and lock in compact_hostlist's mixed zero-pad-width behavior with a regression test. - is_array_group now requires >=2 tasks uniformly for both --group-by=array and --group-by=name, dropping the special case that summarized a lone task with an array-like jobid (e.g. "100_5"). - format_duration now renders sacct-style HH:MM:SS/D-HH:MM:SS instead of a compact "1h12m" notation, matching how Elapsed displays elsewhere in reportseff. Reviewed and modified with the assistance of an AI coding agent, under the supervision of the PR author.

…asks flag, Runtime alias - Replace format_grouped_summary's five separate keyword arguments (and the two rendering helpers below it) with a SummaryOptions dataclass, removing all three `# noqa: PLR0913` markers. - Add --tasks/--no-tasks to `summarize`, defaulting to --no-tasks: only the summary blocks are printed unless per-task rows are explicitly requested, since printing both by default duplicated `report`. - Extract the repeated per-column width computation in _format_metrics_table into one shared helper. - Remove docstring/comment noise flagged in review (DefaultGroup, fetch_job_collection, _format_metrics_table). - Add "Runtime" as a --format alias for sacct's "Elapsed" column (ColumnFormatter._TITLE_ALIASES), for both `report` and `summarize`; Elapsed remains the canonical, sacct-matching name and display value. Reviewed and modified with the assistance of an AI coding agent, under the supervision of the PR author.

… Runtime alias Reflect --tasks/--no-tasks (summary-only by default), the switch to sacct-style HH:MM:SS duration formatting in summary blocks, and document Runtime as an accepted alias for Elapsed in --format. Reviewed and modified with the assistance of an AI coding agent, under the supervision of the PR author.

ovalerio · 2026-09-25T10:47:31Z

Hi @troycomi,

Please have a look to the updated PR. I tried to address all your concerns/ideas.

Thanks & regards!

troycomi · 2026-10-07T15:19:12Z

Github seems to be having issues as I can't resolve or comment individual posts.

I think this is looking great. Only thing to change is to update the minimum click version and remove the custom case insensitive choice from console.py.

You can also bump the version of reportseff, probably to 3.0.0 since we are dropping old dependencies but I'd also be fine with 2.11.

I'll merge after those two changes

Address PR review feedback: raise the minimum click version, remove the custom case-insensitive choice, and bump the major version. - console: remove _CaseInsensitiveChoice and use click.Choice(..., case_sensitive=False) for --group-by and --graph-style. The workaround existed only for click 6.7, which lacked the case_sensitive argument. This also drops the now-unneeded "type: ignore[type-arg]". - pyproject: bump version to 3.0.0 (old click and python support is dropped). Remove the stale old-click-tests poe task and its entry in the composite tests task. click>=8.3.3 was already required, following a pip-audit report against older click releases. - uv.lock: update the reportseff package version to 3.0.0. - README: correct the stated requirements to python >= 3.10 and click >= 8.3.3 (previously python >= 3.6 and click >= 6.7). - tests: remove the unit tests that targeted _CaseInsensitiveChoice and the unused click import. Add end-to-end tests that --group-by accepts any case for both "name" and "array", and that invalid values are rejected for both --group-by and --graph-style. The existing --graph-style mixed-case test is kept. Reviewed and modified with the assistance of an AI coding agent (Claude Sonnet 5.5), with the supervision of the PR main author.

ovalerio · 2026-10-21T10:49:53Z

Hi @troycomi ,

Please have another look. No major changes, mostly dropping the old click version support as you asked, bumping the version and updating the docs (README.md)

ovalerio and others added 12 commits July 5, 2026 04:54

Ignoring local work space folder and .vscode settings.

cea8e20

Edited help message(s) for the '--graph-style' and '--group-by' summa…

8f73baa

…rize options.

Claude (agent) and others added 7 commits August 20, 2026 09:57

chore: bump click in uv.lock file to address a known vulnerability fl…

c6ea372

…agged by pip-audit (PYSEC-2026-2132).

chore: bump click in uv.lock file to address a known vulnerability fl…

cbf19c1

…agged by pip-audit (PYSEC-2026-2132).

Removing type from choice in console.py to keep runtime compatibility…

7f57989

… with click 6.7 while preserving conversion behavior. Using a version of click >= 8.3.3 is strongly advised and enforced also in the pyproject.toml (security audit).

troycomi requested changes Sep 10, 2026

View reviewed changes

ovalerio added 3 commits September 17, 2026 14:50

ovalerio requested a review from troycomi October 21, 2026 10:45

		return compact_hostlist(names)


		def _split_top_level(value: str) -> list[str]:

		from reportseff.job import Job


		def _make_job( # noqa: PLR0913



		def test_group_jobs_by_name_preserves_order() -> None:
		"""Grouping bundles tasks by JobName in first-appearance order."""

		)


		def test_parse_graph_format_empty_tokens_ignored() -> None:

Conversation

ovalerio commented Aug 3, 2026

Uh oh!

troycomi commented Aug 12, 2026

Uh oh!

ovalerio commented Aug 12, 2026

Uh oh!

troycomi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovalerio Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovalerio commented Sep 25, 2026

Uh oh!

troycomi commented Oct 7, 2026

Uh oh!

ovalerio commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ovalerio Sep 17, 2026 •

edited

Loading