refactor: remove legacy R/Quarto benchmark report, rename v2 → benchmark_report - #100
Closed
edmundmiller wants to merge 114 commits into
Closed
refactor: remove legacy R/Quarto benchmark report, rename v2 → benchmark_report#100edmundmiller wants to merge 114 commits into
edmundmiller wants to merge 114 commits into
Conversation
…f-aggregate pipeline. The test now includes improved assertions and metadata updates for better compatibility with Nextflow versioning.
Updated seqera_run_dumps container and error handling in script block
…till need fusion logs support.
Enable using external run logs with nf-aggregate
Bumped version to 0.7.0 to prepare for release
Bump benchmark image version
- Root AGENTS.md: single architecture, no v1/v2 split - modules/local/AGENTS.md: drop benchmark_report row - workflows/AGENTS.md: drop legacy execution path - docs/DESIGN.md: remove migration section, fix process names
v1 is gone, drop the _v2 suffix everywhere: module dir, process name, includes, docs, tests.
API returns tasks as [{task: {...}}, ...] not flat dicts.
build_database doesn't unwrap these yet, so cost/hash fields are None.
Tests marked xfail(strict=True) — will fail CI if bug silently fixes itself.
Seqera API returns tasks as [{task: {...}}, ...]. Unwrap before
inserting into DuckDB so cost/hash/duration fields are populated.
Real SD-1043 data from Seqera Platform API. Two runs: cpu (m5.xlarge) and g5 (g5.xlarge GPU). Used as integration test fixtures.
Old CUR parquet may only have resource_tags_user_unique_run_id without resource_tags_user_nf_unique_run_id. Current query hardcodes both and crashes. xfail(strict=True) marks the expected failure.
DESCRIBE the parquet to check which resource_tags_user_* columns exist before building the SQL. Avoids crash on old-format CUR files that lack the nf_ variant column.
New AWS CUR exports use resource_tags as MAP instead of flattened resource_tags_user_* columns. build_database doesn't handle this yet. xfail(strict=True) on all 3 MAP tests.
Detect MAP vs flattened column format via DESCRIBE. MAP keys accessed directly as resource_tags['key'] (scalar, not list — no [1] index).
Task hash 'ab/cdef12' (Nextflow workdir format with '/') never matches CUR hash 'abcdef12' because LEFT(t.hash, 8) keeps the slash. xfail(strict=True) on both cost tests.
LEFT(REPLACE(t.hash, '/', ''), 8) so Nextflow workdir hashes like 'ab/cdef12' match CUR MD5 prefixes like 'abcdef12'.
Validates build_database + query_run_costs against committed SD-1043 data (cpu + g5 runs). Catches regressions in nested-task unwrap, cost population, and group assignment.
edmundmiller
force-pushed
the
remove-legacy-report
branch
from
March 21, 2026 22:44
60b1b24 to
1bc3bdb
Compare
Hardcoded grid.left: 250 wasted space for short labels (cpu, g5, etc), squeezing chart area and causing x-axis label overlap and clipped axis names. containLabel lets eCharts auto-size margins dynamically.
- Grid: add containLabel + percentage margins as theme defaults - Font: Inter with muted #666 axis labels/ticks (was brand green) - Tooltip: borderRadius 4, lighter bg, better padding - Legend: circle icons, muted text, animation - Bar: borderRadius 2, emphasis focus series, labelLayout hideOverlap - Split lines: subtle #E8E8E8 (was #CFD0D1) - Animation: 500ms duration - Color palette: diverse hues (deep green, blue, dark, gold, teal, mauve) replacing monotone all-greens for better series distinction
- Palette: diverse hues matching theme (was all-greens) - hbarChart/hbarStacked: use theme grid defaults, add bar borderRadius and emphasis focus, slightly taller per-bar spacing (45px) - Workflow status: semantic green (#16a34a) / red (#dc2626) - Process/boxplot/instance/scatter: containLabel replaces hardcoded left margins (350/250/180/70px) - Chart row gap: 32px (was 20px) for breathing room
Drop fontSize: 9/10 from process, boxplot, instance usage axis labels and legend - let theme defaults (11px) apply consistently. Remove barMaxWidth: 16 from process cost bars - theme default (60) is better. Add borderRadius + emphasis to process cost series.
Switch all xAxis from nameLocation:'end' to 'center' with nameGap:25
to prevent label text from being clipped at the grid boundary.
- hbarChart: nameLocation 'end'→'center', nameGap 8→25
- hbarStacked: add opts parameter with xName support
- Workflow Status: pass { xName: 'Tasks' } so axis is labeled
- Process runtime/cost, instance usage, task scatter, boxplot:
add nameLocation:'center' and nameGap:25 to all standalone axes
- Task scatter yAxis: nameGap:40 for longer label
Apply Tufte's direct-labeling principle: when there are few series, label marks directly instead of forcing legend-bouncing. - hbarChart: add series extraction + conditional label for 2–3 series - hbarStacked: inside segment labels (white text), hide legend when ≤3 - Process scatter: markPoint on max-x data point per group - Process cost bars: right-side group labels, conditional legend - Task scatter: markPoint labels per group, conditional legend - Single-series bars left unchanged (y-axis already labels groups) - Stacked bar labels hidden when segment value ≤5 (prevent overflow)
Add auto-generated takeaway subtitles to all charts following Tufte's principle that titles should state a finding, not just label variables. - Add takeaway() helper: compares groups and generates one-line insight (e.g. 'g5 was 3.4× cheaper ($5.4 vs $18.3)') - hbarChart/hbarStacked: accept opts.subtitle, render via title.subtext - Grid top adjusts dynamically (60px when subtitle present, 40px otherwise) - Wall time: 'X was N× faster' - CPU time: 'X was N× lower' - Compute cost: 'X was N× cheaper' - Workflow status: 'All tasks succeeded' or 'N tasks failed across all runs' - CPU/Memory efficiency: 'X was N× more efficient' - Data read/written: 'X was N× less I/O' - Process cost chart: markLine annotation on most expensive process
lib/ classes compile before plugin classes are on the classpath, so importing nextflow.boost.BoostFunctions always fails at semantic analysis. Use java.net.URL directly instead.
Was commented out but its output was still referenced, causing MissingPropertyException at runtime.
Missing closing brace for publish_dir_mode property caused JSON parse error during nf-schema validation.
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
4 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.
Summary
Remove the legacy R/Quarto benchmark report module and promote
benchmark_report_v2tobenchmark_report.Changes
Refactor
benchmark_reportmodule (R scripts, Quarto templates, conda env)workflows/nf_aggregate.nfand configsbenchmark_report_v2→benchmark_reporteverywhereBug fixes
{task: {...}}wrappers; all task fields were NULLnf_unique_run_idcolumn[1]index on MAP format resource_tagsab/cdef12(with/) never matched CUR hashabcdef12; strip/before comparingFeatures
MAP(VARCHAR,VARCHAR)resource_tags columnTests
bin/test_benchmark_report.pycovering all fixes