Skip to content

CI: size check for added files and release-hosted fits (#438) - #517

Merged
GidonFrischkorn merged 20 commits into
developfrom
438-fit-size-check-and-loader
Oct 8, 2026
Merged

GidonFrischkorn merged 20 commits into
developfrom
438-fit-size-check-and-loader

Conversation

@GidonFrischkorn

@GidonFrischkorn GidonFrischkorn commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Closes the CI part of #438 (ADR 0003). No fit file is moved or removed here.

What the checks do

file-size (new; PRs into master, develop and dev-*) fails a PR when any commit in it adds a blob above 1 MiB, even if a later commit deletes the file. A merge commit keeps every commit of the PR, so a blob that a PR adds and removes still lands in the history for good (a squash merge would not, but the check cannot know which one will be used). The step names the path and commit of every blob over the limit. A blob held anywhere in the history of the base or of origin/develop passes, so renames, forward merges and restores of the fits tracked today (32 files, 150.6 MiB, in vignettes/articles/assets/; 87 .rds blobs, 471.8 MiB, reachable from develop) do not trip it. It also runs on edited, which is what retargeting a stacked PR raises.

The first version compared the PR tip with the base. A PR that added a 2 MB fit and deleted it in the next commit passed that scan. The new scan fails it.

pkgdown (changed):

  • A new step after the fits download checks every string-literal file = that an evaluated R chunk passes to bmm(), brm(), readRDS() or read_rds(), in the articles pkgdown builds (vignettes/**/*.[Rrq]md without _-prefixed files and vignettes/tutorials/). A fits/ file must be in the release, any other file must be tracked. It fails, naming the article and the file, when one is missing or readRDS() cannot read it. A fits/ path it cannot check fails too: a positional readRDS("fits/<name>"), a path built with paste0() or file.path(), a file = of any other function except a writer, and inline code that names one. With this step the no-release case (HTTP 404) stays a harmless no-op. No article loads from fits/ yet. On the current tree it checks 26 references in 16 articles, all in assets/, with 0 problems.
  • The job is skipped for the release event of the article-fits tag. Creating that release fires release: published (a pre-release does too), and without the guard the job builds the tagged commit and deploys it to gh-pages.

CONTRIBUTING states this scope.

Changes after review

  • The fit step moved from the workflow YAML to .github/scripts/check-article-fits.R.
  • The scanner follows knitr's fence rule: a chunk opens at any ```{r …} header line and #| options count only at the top of the chunk. Four shapes were missed before: a chunk shown inside a longer fenced block, a {r} header after an unclosed plain fence, a chunk that is never closed, and #| eval: false after the first line of code.
  • Files that pkgdown does not build (_-prefixed, vignettes/tutorials/) are not checked.
  • Every string-literal file = passed to bmm(), brm(), readRDS() or read_rds() is checked, assets/... as well as fits/.... A fits/ file must be in the release, any other file must be tracked. .rds is appended only where bmm() and brm() append it; readRDS() and read_rds() paths are checked as written.
  • A file = passed to a writer (saveRDS(), save(), write_rds(), write.csv(), write.table(), writeLines(), cat(), sink(), ggsave(), png(), jpeg(), pdf(), svg()) is ignored. Any other file =, such as read.csv(file = "assets/data.csv") or update(fit, file = "assets/x"), is ignored unless it names a fits/ path, which fails because it cannot be checked. update() is in neither list: update.bmmfit() only writes file, but update() of a brm() fit reads it.
  • The step stops with an error when there is no vignettes/ folder, so a changed working directory cannot turn it into a silent pass.
  • The widened check found three missing assets/ fits in evaluated chunks on the stale branch 308-hypothesis-testing-article (assets/bmmfit_postestimation_{sdm,sdm_null,mix2p}). The old step looked only at fits/ paths and did not see them.
  • file-size fails when an object cannot be fetched.
  • The annotation shows the short commit.

The limit

I set it at 1 MiB. That settles the open line in ADR 0003, whose text is not edited here.

Branches that fail today

Measured against origin/develop (c47e6f9) for the 57 remote branches other than develop and gh-pages:

Branch Large blob What it has to do
race-lnr (#501) bmmfit_lnr_hierarchical_vignette.rds, 3.3 MB Upload the fit to the release first, load it from fits/, and drop the commit that added it from the branch. A later delete does not help.
352-log-normal-race-model the same fit Superseded by race-lnr.
mv-article three fits, 8.1 MB Same as race-lnr.
reliability tests/testthat/assets/bmmfit_sdm_reliability.rds, 1.45 MB A test fixture, not an article fit. Shrink it below 1 MiB or decide on an exception.
feature/chkptstanr stale checkpoint files Nothing, unless revived.

Not covered

  • The workflows have not run on GitHub, and actionlint is not installed. The file-size block was run as text extracted from the YAML, and the fit check as Rscript .github/scripts/check-article-fits.R.
  • file-size is not a required check. Today develop requires only the three R-CMD-check jobs, so a red file-size blocks nothing until it is added.
  • A PR runs the workflow files of its own merge commit, so a PR that edits file-size.yaml can weaken its own check. Review of .github/ is the guard. This is inferred from the docs and was not run.
  • The fit step sees string-literal file = arguments to bmm(), brm(), readRDS() and read_rds() in evaluated R chunks. It does not see sourced code, child documents, or a directory held in a variable. It only reads the fit, so a readable fit from the wrong model passes.
  • Not confirmed from the GitHub docs: that uploading assets to an existing release fires no event. The docs list seven release activity types and none concerns assets. Also untested: whether a job skipped by if still takes the pkgdown concurrency slot.
What was run

After merging

Merge this PR first. A release event runs the workflow file at the tagged commit, so the guard has to be on develop before the release exists. Then create the release, then mark file-size required on develop and dev-* (repository settings, branch protection or rulesets, required status checks; the check is offered once it has run on a PR).

gh release create article-fits \
  --prerelease --latest=false \
  --target develop \
  --title "Article fits" \
  --notes "Cached fits for the articles (ADR 0003)"

A normal release would become "Latest", and install_github("popov-lab/bmm@*release") would resolve to it.

Optional follow-up for #489

The MPT article fits three models on every site build (fit-2htm, fit-ol, fit-ol-restricted; none uses file =). Uploading those fits to the release and adding file = "fits/..." would end that. It needs the release first, so it is not part of this PR.

The check diffed BASE...HEAD, so a fit added in one commit and deleted in a
later one passed, yet a merge commit carries the blob into develop for good.

The step now lists the objects reachable from the PR head and from neither
the base nor origin/develop, and fails on every blob above the limit, naming
its path and the commit that added it. Blobs already in develop are exempt,
so a dev-* PR whose head merged develop does not fail on grandfathered fits.

The blobs the blobless clone lacks come in one fetch. Looked up one by one,
each blob arrives in its own pack, about 0.4 s each, and the lookup aborts
after about 50 packs.
… release (#438)

When the article-fits release does not exist, the download step continues
with no fits, and a fit that an article loads from fits/ but the release
lacks is fitted during the site build. A new step lists every
file = "fits/<name>" in vignettes/articles/*.Rmd after the download and fails,
naming the article and the fit, when a listed fit is absent or readRDS()
cannot read it. A fits/ path that is not a string literal passed to file
fails too, because it cannot be listed.
…ase is created (#438)

The workflow runs on release: published, which also fires for a pre-release,
and under that event the job builds the commit of the tag and deploys it to
gh-pages. The article-fits release only holds fits, so the job is skipped for
that tag. Versioned releases deploy as before.
The section now says that the file-size check reads every commit of the PR,
not only the final tree; that files already in develop do not count; and that
the site build stops, naming the article and the fit, when a fit loaded from
fits/ is missing from the release. It also asks for the path as a string
literal, which the build needs to list the fits.
…ory; run on retarget (#438)

An independent review found three gaps in the scan.

- Excluding the base and develop with ^rev drops only the trees of their
  tips, so a PR that restores a fit that develop had deleted earlier failed
  although the blob is in the protected history. The step now removes every
  object reachable from the base or from develop.
- A stacked PR is retargeted to develop when its base merges, which is an
  edited event, so the check never ran for it. edited is now a trigger type.
- Paths with a comma, colon or percent sign broke the error annotation.
#438)

An independent review found fits the step could not see and references it
rejected for no reason.

- Read every vignettes/**/*.[Rrq]md, as pkgdown does, and look for the fit
  next to the article.
- Follow knitr's fences: chunks in blockquotes count, chunk syntax shown in
  a longer fence does not, chunks with eval = FALSE are skipped.
- Fail on inline code that names a fits/ path.
- A bare "fits" string counts only inside a file argument, so
  .key = "fits" no longer fails; `file` in backticks is recognised.
- Invalid UTF-8 in an article no longer makes the step see nothing.
A merge commit keeps every commit of a PR, a squash merge does not; files
held anywhere in the history of develop or of the base do not count; the
site build reads file = "fits/<name>" in R chunks only.
cat-file prints '<oid> missing' for an object the batched fetch did not
deliver, and the size loop skipped such a line, so an unsized candidate
passed unchecked. The step now reports the count and fails. The annotation
names the commit by its first ten hex digits (--no-abbrev also disabled %h),
and the comment on the edited trigger says it fires for title and body edits.
The R code of the "Check article fits" step moves unchanged to
.github/scripts/check-article-fits.R, so that it can be parsed, linted and
run on fixtures with Rscript.
…ath (#438)

The fit check's chunk scanner now applies knitr 1.52's rule: plain fences
are text, a chunk header opens a chunk wherever it stands, only a fence with
the header's own prefix and backtick count closes it, an unclosed chunk runs
to the end of the file, and #| options are read from the leading lines only.
A chunk shown inside a four-backtick block, a chunk after an unclosed plain
fence, an unclosed chunk and #| eval: false after code were evaluated by
knitr but passed the check.

The check skips the files pkgdown does not build (names starting with _,
vignettes/tutorials/), checks every literal relative file = path, so a fit
in assets/ that is not in the tree fails too, ignores saveRDS(), save() and
write_rds() calls, and escapes its annotations.
# Conflicts:
#	.github/CONTRIBUTING.md
…es/ is absent (#438)

The fit check treated every literal file = as a fit and appended .rds, so read.csv(), write.csv(), png(), cat() and readRDS(file = "x.RDS") failed the build. It now checks only bmm(), brm(), update(), readRDS() and read_rds(), appends .rds only where bmm() and brm() do, ignores writers, and still fails on any fits/ path it cannot check. Without a vignettes/ folder it stops instead of reporting 0 problems.
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.

1 participant