Skip to content

feat: split the AD benchmark report out of the ad-backends template (#299)#305

Open
seabbs-bot wants to merge 4 commits into
mainfrom
fix/299-benchmark-report-split
Open

feat: split the AD benchmark report out of the ad-backends template (#299)#305
seabbs-bot wants to merge 4 commits into
mainfrom
fix/299-benchmark-report-split

Conversation

@seabbs-bot

Copy link
Copy Markdown
Collaborator

Summary

Fixes #299. Follow-up from EpiAwareADTools#28: the AD backend-comparison benchmark (the "Packages used" DIT/CairoMakie setup and the whole "Benchmark" section -- summary table, box plots, per-scenario plot) lived on the ad-backends.jl tutorial page, reading as a how-to guide even though it is a cost report. That page is a kit-managed template shared verbatim across every AD-enabled adopter, so a per-repo split would be clobbered on the next sync -- the split has to happen here.

What moved

  • New templates/docs/src/getting-started/tutorials/ad-comparison.jl: the benchmark computation, summary table, and both plots, moved out of ad-backends.jl largely verbatim. Cross-links back to ad-backends for the how-to-choose narrative and configuration rather than duplicating it.
  • ad-backends.jl keeps the backend-support table, Enzyme configuration, "Choosing a backend", and "Debugging" sections, now linking to ad-comparison for the actual numbers instead of carrying them.
  • Both pages are heavy tutorials, registered via _ad_heavy_tutorials/_ad_tutorial_stubs; the shared ad-only docs-env deps (_ad_docs_deps etc.) are unchanged since ad-comparison.jl is the one that actually needs the DIT/plotting stack.

Nav

New _benchmarks_nav(benchmarks, ad) replaces the old flat bench_nav computation:

  • benchmarks && ad: nests "Performance history" (benchmarks.md) and "AD comparison" under one "Benchmarks" section -- the shape EpiAwareADTools#28 asked for, rather than five one-off per-repo nav edits.
  • benchmarks only: the existing flat "Benchmarks" => "benchmarks.md", unchanged.
  • ad only (the common default, no performance-history suite): a flat "Benchmarks" => "...ad-comparison.md" entry, so a tooling-only-benchmarks package still gets a Benchmarks nav entry rather than none.
  • Neither: no entry, unchanged.

ad-comparison.md is registered under Benchmarks, not Tutorials -- that's the entire point of the split.

Rollout note

docs/pages.jl is package-owned and write-once (substituted only on first scaffold, never re-applied by update), so an already-scaffolded ad = true adopter needs a one-line manual edit to pick up the new Benchmarks nav entry pointing at ad-comparison.md -- this mirrors the existing pattern for a package that later flips benchmarks = true and needs the same manual pages.jl edit today. The ad-comparison.jl file itself is a fully managed template, so it lands automatically on the next update(); only its own nav link needs the one-line follow-up.

Verification

  • Extended the kit's own test suite: new/updated assertions on both split pages' content (the moved benchmark code is gone from ad-backends.jl, present in ad-comparison.jl; cross-refs present in both; valid-Julia parse check on both), the three _benchmarks_nav shapes, and fixed two pre-existing benchmarks_gating.jl tests whose fixtures used the scaffold default ad = true and so now correctly get a Benchmarks nav entry even with benchmarks = false (previously "no Benchmarks entry at all" was the only case tested).
  • Went beyond the kit's own suite: a real Documenter.makedocs build resolves the @ref ad-backends/@ref ad-comparison cross-links between the split pages (anchors only, not the full AD/DIT/CairoMakie compute), and Literate.jl (execute = false) processes both substituted templates cleanly.
  • Full suite: 2529 pass, 8 pre-existing broken (xfail) markers, 0 failures.
  • JuliaFormatter clean.

This was opened by a bot. Please ping @seabbs for any questions.

…299)

Follow-up from EpiAwareADTools#28: the AD backend-comparison benchmark
(the "Packages used"/DIT/CairoMakie setup and the whole "Benchmark"
section) lived on the ad-backends.jl tutorial page, reading as a
how-to guide even though it is a cost report. Splitting it per-repo
would be clobbered on the next sync since it is a kit-managed
template, so the split happens here.

- New template docs/src/getting-started/tutorials/ad-comparison.jl:
  the benchmark computation, summary table, and plots, moved verbatim
  out of ad-backends.jl. Cross-links back to ad-backends for the
  how-to-choose narrative and configuration.
- ad-backends.jl keeps the backend-support table, Enzyme configuration,
  choosing-a-backend, and debugging sections, now linking to
  ad-comparison for the numbers instead of duplicating them.
- Both pages are heavy tutorials (`_ad_heavy_tutorials`/
  `_ad_tutorial_stubs`), sharing the existing ad-only docs-env deps
  (`_ad_docs_deps` etc., unchanged -- ad-comparison.jl is the one that
  actually needs the DIT/plotting stack).
- New `_benchmarks_nav(benchmarks, ad)`: nests "Performance history"
  (benchmarks.md) and "AD comparison" under one "Benchmarks" section
  when both are enabled -- the shape EpiAwareADTools#28 asked for --
  falls back to whichever single page applies when only one is, and
  omits the section entirely when neither is. ad-comparison.md is
  registered under Benchmarks, not Tutorials: that is the entire point
  of the split.

Verified beyond the kit's own (extended) test suite: a real
`Documenter.makedocs` build resolves the `@ref ad-backends`/
`@ref ad-comparison` cross-links between the split pages, and
Literate.jl processes both substituted templates cleanly
(`execute = false`, no heavy AD/DIT/CairoMakie compute needed for
that check).

Existing ad=true adopters need a one-line docs/pages.jl edit to pick
up the AD-comparison nav entry -- pages.jl is package-owned/write-once,
so this mirrors how enabling `benchmarks = true` later already
requires the same manual edit.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
@seabbs-bot

Copy link
Copy Markdown
Collaborator Author

Review: fix/299-benchmark-report-split

Independent review (read-only checkout, PR head 119615a). Verdict: needs-changes — the split, nav logic, and rollout mechanics are all correct and well-verified; the one real gap is that the rollout note isn't documented anywhere a downstream adopter would actually see it.

Probe 1: template split fidelity

Diffed the old ad-backends.jl (444 lines) against the union of the new ad-backends.jl (199) + ad-comparison.jl (295, new). No content lost, and — the specific regression #299 exists to prevent — the benchmark computation, summary table, and both plots appear exactly once, only in ad-comparison.jl. Each page correctly gained its own self-sufficient imports now that they're separate Literate units (ad-comparison.jl picked up its own using ADFixtures, since it can no longer rely on the earlier using ADFixtures that used to live upstream on the same page before the split). Cross-references were reworded appropriately in both directions (e.g. "as named entries in the support table above" → "in the [AD backends](@ref ad-backends) support table", since the support table moved to the other page). The "Reproducing this page" boilerplate legitimately appears on both pages (build instructions, not benchmark content), which is fine.

Probe 2: nav matrix

Ran _benchmarks_nav directly for all four (benchmarks, ad) combinations and confirmed each produces a well-formed, evaluable pages fragment:

  • neither → no entry (unchanged)
  • benchmarks only → flat "Benchmarks" => "benchmarks.md" (unchanged)
  • ad only (the common default, the rarer case to check) → flat "Benchmarks" => ".../ad-comparison.md" — correct, gives a tooling-only-benchmarks package a Benchmarks entry rather than none
  • both → nested "Benchmarks" => ["Performance history" => ..., "AD comparison" => ...], the ADTools#28 shape

All four parse and evaluate correctly when spliced into a real pages = [...] array literal.

Probe 3: reproduced the end-to-end verification myself

The PR's own diff only adds Meta.parseall syntax checks, not a real Literate/Documenter run — so I reconstructed their claimed verification independently (matching the kit's actual Literate.markdown invocation: flavor = Literate.DocumenterFlavor(), mdstrings = true, credit = false, since that's what _process_tutorials actually uses). Result: Literate converts both substituted templates cleanly, and a real Documenter.makedocs build resolves the @ref ad-backends/@ref ad-comparison cross-links between the two pages with zero cross-reference errors. The only failures in my run were @example code-execution errors from missing real Wombat/ADFixtures/DIT/CairoMakie dependencies in my throwaway environment — exactly the "not the full AD/DIT/CairoMakie compute" scope the PR itself claims to have checked, not a cross-reference problem.

Probe 4: the two fixed gating tests

  • First fix: the assertion (!occursin("benchmarks.md", pages)) was already correct and unchanged — only the comment was clarified to note a Benchmarks entry may still exist (pointing at AD-comparison) since ad = true is the scaffold default. Not a real fix, just better documentation.
  • Second fix: genuinely stale. With ad = true (default) and benchmarks = true, the nav is now the nested shape, so the literal substring "Benchmarks" => "benchmarks.md" no longer appears (it's "Performance history" => "benchmarks.md" now). Confirmed this isn't an unintended breaking change to scaffold defaults: I directly simulated the rollout scenario — scaffolded a package, overwrote pages.jl to look exactly like a genuine pre-Split the benchmark report out of the shared ad-backends tutorial template #299 adopter's file (no Benchmarks entry, since the old bench_nav ignored ad entirely), then ran update() with the new kit code. Result: pages.jl came out byte-identical, confirming no nav entry appears unbidden. The new ad-comparison.jl template file does land automatically, and the resynced ad-backends.jl page gains a working @ref ad-comparison cross-link — pointing at a page that exists in docs/src but isn't listed in the adopter's nav (Documenter resolves @ref/@id independent of the pages listing, confirmed in probe 3, so this doesn't break their build — it just means the new page is reachable and cross-linked-to but not in the sidebar until the manual edit lands).

Probe 5: the rollout note

Confirmed structurally: Template("docs/pages.jl", ..., false, true)managed = false, i.e. genuinely write-once/package-owned — while both AD template files are managed = true. This matches the PR's claim exactly, and my simulation above confirms it in practice, not just in the type signature.

However: this exact disclosure isn't in NEWS.md, despite an established convention for it — the current top entry in NEWS.md (#294, "Every template-sync/self-drift caller... an adopter... needs a one-line fix... before their first sync on this kit version") is the direct in-repo precedent for exactly this kind of "existing adopter must make a manual edit" note. This PR's rollout note lives only in the PR description, which isn't visible to a downstream adopter after merge — NEWS.md is the actual persisted, adopter-facing channel. Ask: add a NEWS.md entry mirroring #294's format, naming the exact one-line edit (e.g. adding "Benchmarks" => "getting-started/tutorials/ad-comparison.md" — or the nested form if benchmarks = true too — to the end of the pages array in docs/pages.jl).

Probe 6: suite + formatter + hygiene

  • julia --project=test test/runtests.jl: 2529 pass / 8 broken(xfail) / 0 fail — matches the PR's claim exactly.
  • JuliaFormatter: clean, no diff.
  • Hygiene: author/committer seabbs-bot, Co-authored-by: Sam Abbott present, no AI-tool footers. Trivial, non-blocking: two new @testset name lines exceed 80 chars (105/91), and two template lines by 1 char (81) — consistent with the existing long-descriptive-name pattern elsewhere in this repo.

This was opened by a bot. Please ping @seabbs for any questions.

@github-actions

Copy link
Copy Markdown
Contributor

Try this Pull Request!

Option 1: Julia Package Manager

Open Julia and type:

import Pkg
Pkg.activate(temp=true)
Pkg.add(url="https://github.com/EpiAware/EpiAwarePackageTools.jl", rev="fix/299-benchmark-report-split")
using EpiAwarePackageTools

Option 2: Local Checkout

If you have the repo locally:

git checkout fix/299-benchmark-report-split
julia --project=. -e "using Pkg; Pkg.instantiate()"

@github-actions

Copy link
Copy Markdown
Contributor

📖 Documentation preview is ready!

View the docs for this PR at: https://EpiAware.github.io/EpiAwarePackageTools.jl/previews/PR305/

This preview will be updated automatically when you push new commits.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unit 94.83% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/scaffold.jl 99.45% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

seabbs-bot and others added 3 commits July 21, 2026 09:26
Review on this PR asked for the rollout note (existing ad=true adopters
need a one-line docs/pages.jl edit to surface the new nav entry) to be
disclosed in NEWS.md rather than only in the PR description, mirroring
the #294 entry's format and precedent for this kind of adopter-facing
manual-edit note.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
AlgebraOfGraphics caps DimensionalData (transitively, via Makie) below
the range FlexiChains requires, so any package that hard-deps both
(e.g. DistributionsInference.jl) cannot resolve a workspace that
includes the scaffolded docs environment (kit#283). The #299 split
moved both grammar-of-graphics plots onto the new ad-comparison.jl
page, so this is the one place left carrying the AlgebraOfGraphics/
DimensionalData pull-in -- swap them to plain CairoMakie/Makie
(per-facet Axis grids, boxplot!/scatter! recipes, a manual Legend)
with identical output, and drop AlgebraOfGraphics from the docs-env
deps and compat it no longer needs.

The scenario-scatter facets build their Axis grid via a comprehension
rather than a mutating for-loop: `@example` blocks execute each
top-level statement in global scope, where reassigning a pre-declared
variable inside a for loop is Julia's ambiguous soft-scope case
(warns locally, hard `UndefVarError`s under Literate/Documenter).
Verified with a standalone repro against synthetic data: both figures
render correctly (box plot; scatter faceted by metric, coloured by
backend family, marker by forward/reverse mode, with a working
legend), and the kit's own suite passes unchanged bar the one dropped
assertion for the removed dependency.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
@seabbs-bot

Copy link
Copy Markdown
Collaborator Author

Pushed two follow-ups after review:

  1. NEWS.md entry for the rollout note (existing ad = true adopters
    need a one-line docs/pages.jl edit to surface the new AD-comparison
    nav entry), mirroring Move to Public not exported update vs scaffold_update #294's format and naming the exact edit.
  2. kit#283 fix, folded into this PR: while touching this page, found
    that the new ad-comparison.jl template (where both benchmark plots
    landed after the Split the benchmark report out of the shared ad-backends tutorial template #299 split) still imported AlgebraOfGraphics,
    carrying the DimensionalData conflict with FlexiChains that AlgebraOfGraphics (docs ad-backends tutorial) conflicts with FlexiChains as a hard dep #283
    reported. Swapped both plots to plain CairoMakie/Makie (per-facet
    Axis grid, boxplot!/scatter!, a manual Legend) and dropped
    AlgebraOfGraphics from _ad_docs_deps/_ad_docs_compat. Verified
    against synthetic data standalone (both figures render correctly,
    scatter legend included) and the kit's own suite passes unchanged
    bar the one dropped dependency assertion.

Also rebased (via merge, not rebase, to avoid rewriting the original
commit) onto current main, which now includes #303 and #304 -- no
file overlap, clean merge, full suite green after (2549 pass / 8
broken(xfail) / 0 fail).

This was opened by a bot. Please ping @seabbs for any questions.

@seabbs

seabbs commented Jul 21, 2026

Copy link
Copy Markdown
Member

Sorry what we should have both of these in a benchmarks tab with one being AD comparison and the other Over time or similar we had an issue steer for this on i.e convolveddistributions which was very clear. This PR description currently reads as a reversion to past practice but also current state seems not in line with where we want to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split the benchmark report out of the shared ad-backends tutorial template

2 participants