Skip to content

fix(docs): cache R dependencies immediately after installing them#516

Open
Copilot wants to merge 10 commits into
mainfrom
copilot/cache-dependencies-after-installing
Open

fix(docs): cache R dependencies immediately after installing them#516
Copilot wants to merge 10 commits into
mainfrom
copilot/cache-dependencies-after-installing

Conversation

Copilot AI commented May 13, 2026

Copy link
Copy Markdown
Contributor

The docs workflow saves its R package cache in a post-step, which is skipped when the job is cancelled (e.g., by cancel-in-progress: true triggering on a new push during the long Build docs step). This means a cold cache on every cancelled run.

Changes

  • Explicit cache restore (actions/cache/restore) added immediately after setup-r, before dependency installation
  • Disabled built-in caching in setup-r-dependencies (cache: false) to prevent duplicate/conflicting post-step saves
  • Explicit cache save (actions/cache/save) added immediately after Install R package and before Build docs — ensuring the cache is persisted regardless of whether the subsequent build succeeds, fails, or is cancelled
  • Added id: setup-r to the setup-r step to reference installed-r-version in the cache key
  • Added .claude to .gitignore alongside .positai to prevent accidental commits of editor/agent artifacts
  • Fixed roxygen2 parse error in R/sim_pop_data.R by removing a duplicate/malformed @inheritParams comment that caused roxygen2 8.0.0 to fail with a subscript out of bounds error
  • Regenerated man/ files with roxygen2 8.0.0 (matching the CI environment), preserving full documentation for curve_params, verbose, and all other parameters; merged upstream changes to plot_curve_params_one_ab.Rd that remove fun/n/args from ... docs (those args are now blocked with an informative error in the implementation)
  • Added rlang to inst/WORDLIST to account for new text introduced by roxygen2 8.0.0's expanded @inheritDotParams output
- uses: r-lib/actions/setup-r@v2
  id: setup-r

- name: Restore R dependency cache 📦
  uses: actions/cache/restore@v4
  with:
    path: ${{ env.R_LIBS_USER }}
    key: ${{ runner.os }}-r-${{ steps.setup-r.outputs.installed-r-version }}-${{ hashFiles('**/DESCRIPTION') }}

- uses: r-lib/actions/setup-r-dependencies@v2
  with:
    cache: false
    ...

- name: Install R package 🚧
  ...

- name: Save R dependency cache 💾
  uses: actions/cache/save@v4
  with:
    path: ${{ env.R_LIBS_USER }}
    key: ${{ runner.os }}-r-${{ steps.setup-r.outputs.installed-r-version }}-${{ hashFiles('**/DESCRIPTION') }}

- name: Build docs 🏗  # cache already saved before this point
  ...

Thank you for taking the time to submit a pull request!

To maximize the chances of acceptance:

  • The title of your PR should briefly describe the change.
  • Commit/merge messages to be included in NEWS.md should begin with -.
  • Code should follow the tidyverse style guide.
  • Documentation should use roxygen2, with Markdown syntax.
  • Contributions should include unit tests (using testthat).

For more information see Contributing.

Replace the post-step cache save from r-lib/actions/setup-r-dependencies
with explicit actions/cache/restore + actions/cache/save steps. This ensures
the cache is saved before the Build docs step, so even if the workflow is
cancelled or fails during rendering, the cache is preserved for the next run.

Agent-Logs-Url: https://github.com/UCD-SERG/serocalculator/sessions/de780b35-1ac0-406a-b25f-d41f2847b7e5

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
Copilot AI changed the title [WIP] Update docs workflow to cache dependencies immediately after installation fix(docs): cache R dependencies immediately after installing them May 13, 2026
Copilot AI requested a review from d-morrison May 13, 2026 02:20
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

github-actions Bot added a commit that referenced this pull request May 13, 2026
@d-morrison d-morrison marked this pull request as ready for review May 13, 2026 05:13
Copilot AI review requested due to automatic review settings May 13, 2026 05:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the documentation build workflow to persist the R package cache earlier (so cancellations during long doc builds don’t lose the cache), alongside a regenerated set of Rd documentation/link formatting changes and minor ignore-file updates.

Changes:

  • Add explicit cache restore/save steps to the Docs workflow and disable setup-r-dependencies built-in caching to avoid post-step-only saves.
  • Regenerate/update Rd files (primarily link targets/formatting and inherited parameter listings).
  • Update ignore files and adjust package metadata related to roxygen output.

Reviewed changes

Copilot reviewed 3 out of 27 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
.github/workflows/docs.yaml Adds early cache restore + pre-build cache save for R dependencies in docs workflow
DESCRIPTION Adjusts roxygen-related metadata fields
.Rbuildignore Ignores additional local/agent artifact files for package builds
.gitignore Ignores .positai artifact file
man/simresp.tinf.Rd Updates internal links in ... argument documentation
man/simcs.tinf.Rd Updates link formatting for forwarded ... arguments
man/sim_pop_data.Rd Updates forwarded-links and return-type link; removes some argument docs
man/sim_pop_data_multi.Rd Updates link formatting; removes some inherited dot-params docs
man/serocalculator-package.Rd Adds author entry
man/reexports.Rd Adjusts ggplot2 reexport link target formatting
man/plot_curve_params_one_ab.Rd Expands inherited ... argument documentation from ggplot2::geom_function()
man/load_sr_params.Rd Updates tibble class link targets
man/load_pop_data.Rd Updates tibble class link targets and internal link formatting
man/load_noise_params.Rd Updates tibble class link targets
man/graph.curve.params.Rd Minor Rd formatting/link output changes
man/graph_seroresponse_model_1.Rd Updates internal link formatting for forwarded ...
man/graph_loglik.Rd Updates internal link formatting for forwarded ...
man/est_seroincidence_by.Rd Updates internal link formatting for forwarded ...
man/dot-nll.Rd Updates internal link formatting for forwarded ...
man/count_strata.Rd Updates tibble class link targets
man/autoplot.summary.seroincidence.by.Rd Updates internal link formatting for forwarded ...
man/autoplot.seroincidence.by.Rd Updates internal link formatting for forwarded ...
man/as_sr_params.Rd Updates tibble class link targets
man/as_pop_data.Rd Updates tibble class link targets
man/as_noise_params.Rd Updates tibble class link targets
man/analyze_sims.Rd Updates tibble class link targets
man/ab.Rd Updates internal link formatting for forwarded ...
Files not reviewed (23)
  • man/ab.Rd: Language not supported
  • man/analyze_sims.Rd: Language not supported
  • man/as_noise_params.Rd: Language not supported
  • man/as_pop_data.Rd: Language not supported
  • man/as_sr_params.Rd: Language not supported
  • man/autoplot.seroincidence.by.Rd: Language not supported
  • man/autoplot.summary.seroincidence.by.Rd: Language not supported
  • man/count_strata.Rd: Language not supported
  • man/dot-nll.Rd: Language not supported
  • man/est_seroincidence_by.Rd: Language not supported
  • man/graph.curve.params.Rd: Language not supported
  • man/graph_loglik.Rd: Language not supported
  • man/graph_seroresponse_model_1.Rd: Language not supported
  • man/load_noise_params.Rd: Language not supported
  • man/load_pop_data.Rd: Language not supported
  • man/load_sr_params.Rd: Language not supported
  • man/plot_curve_params_one_ab.Rd: Language not supported
  • man/reexports.Rd: Language not supported
  • man/serocalculator-package.Rd: Language not supported
  • man/sim_pop_data.Rd: Language not supported
  • man/sim_pop_data_multi.Rd: Language not supported
  • man/simcs.tinf.Rd: Language not supported
  • man/simresp.tinf.Rd: Language not supported
Comments suppressed due to low confidence (1)

DESCRIPTION:84

  • DESCRIPTION drops the standard RoxygenNote field and replaces it with Config/roxygen2/version: 8.0.0. This is likely to cause the Documentation check workflow (.github/workflows/R-check-docs.yml) to fail if roxygen2::roxygenise() rewrites DESCRIPTION back to RoxygenNote (and/or if other tooling expects RoxygenNote).

Suggestion: keep RoxygenNote managed by roxygen2, and only add extra config fields if they are confirmed to be stable outputs of the roxygen2 version used in CI.

LazyData: true
NeedsCompilation: no
Roxygen: list(markdown = TRUE, roclets = c("collate", "rd", "namespace"))
Remotes: 
    d-morrison/snapr
Config/roxygen2/version: 8.0.0

Comment thread man/sim_pop_data.Rd
Comment thread man/sim_pop_data_multi.Rd
Comment thread .Rbuildignore
Comment thread man/plot_curve_params_one_ab.Rd Outdated
@d-morrison

Copy link
Copy Markdown
Member

@copilot apply changes based on the comments in this thread

@d-morrison

Copy link
Copy Markdown
Member

@copilot fix failing workflows

@d-morrison d-morrison removed their request for review May 13, 2026 05:24
github-actions Bot added a commit that referenced this pull request May 13, 2026
- Revert man/ files and DESCRIPTION back to pre-808ee97 state, which had
  accidentally regenerated docs using roxygen2 8.0.0 instead of 7.3.3,
  causing curve_params and verbose to disappear from sim_pop_data.Rd,
  curve_params to disappear from sim_pop_data_multi.Rd ..., and
  fun/n/args/geom to be incorrectly added to plot_curve_params_one_ab.Rd
- Add .claude to .gitignore alongside .positai

Agent-Logs-Url: https://github.com/UCD-SERG/serocalculator/sessions/9e438bed-cf4c-4206-9fb0-9e39fc0978b8

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>

Copilot AI commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@copilot apply changes based on the comments in this thread

Done in commit 1febee5:

  1. sim_pop_data.Rd / sim_pop_data_multi.Rd – Reverted the accidental roxygen2 8.0.0 doc regeneration (from commit 808ee97) that had dropped curve_params and verbose from sim_pop_data.Rd and removed curve_params from the ... section of sim_pop_data_multi.Rd. These are now restored.
  2. plot_curve_params_one_ab.Rd – Also restored; the fun, n, args, and geom entries that were incorrectly inherited into ... are removed.
  3. .gitignore – Added .claude alongside the existing .positai entry.

- Remove duplicate @inheritParams log_likelihood comment in sim_pop_data.R
  that caused roxygen2 8.0.0 parse error (subscript out of bounds)
- Regenerate man/ files with roxygen2 8.0.0 (matching CI) to fix Documentation check
- Add NEWS.md entry for docs workflow cache improvement (fixes changelog check)
- Increment version to 1.4.0.9010 (fixes version-check workflow)
- Add 'rlang' to inst/WORDLIST (fixes spellcheck workflow)
- Remove duplicate 'Serological' entry from WORDLIST

Agent-Logs-Url: https://github.com/UCD-SERG/serocalculator/sessions/552e56c7-f282-415b-94b5-9ccd8ad88ab4

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>

Copilot AI commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@copilot fix failing workflows

Fixed in commit 4b1a076:

  • Documentation check: Removed duplicate/malformed @inheritParams log_likelihood # verbose tag in sim_pop_data.R that caused a parse error in roxygen2 8.0.0, then regenerated all man/ files with roxygen2 8.0.0 (matching the CI environment)
  • Check NEWS.md Changelog: Added a changelog entry for the docs workflow cache improvement
  • Version increment check: Bumped version to 1.4.0.9010
  • Spellcheck: Added rlang to inst/WORDLIST (introduced by roxygen2 8.0.0's expansion of @inheritDotParams ggplot2::geom_function)

github-actions Bot added a commit that referenced this pull request May 13, 2026
@d-morrison d-morrison requested a review from Copilot May 13, 2026 06:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 30 changed files in this pull request and generated 1 comment.

Files not reviewed (23)
  • man/ab.Rd: Language not supported
  • man/analyze_sims.Rd: Language not supported
  • man/as_noise_params.Rd: Language not supported
  • man/as_pop_data.Rd: Language not supported
  • man/as_sr_params.Rd: Language not supported
  • man/autoplot.seroincidence.by.Rd: Language not supported
  • man/autoplot.summary.seroincidence.by.Rd: Language not supported
  • man/count_strata.Rd: Language not supported
  • man/dot-nll.Rd: Language not supported
  • man/est_seroincidence_by.Rd: Language not supported
  • man/graph.curve.params.Rd: Language not supported
  • man/graph_loglik.Rd: Language not supported
  • man/graph_seroresponse_model_1.Rd: Language not supported
  • man/load_noise_params.Rd: Language not supported
  • man/load_pop_data.Rd: Language not supported
  • man/load_sr_params.Rd: Language not supported
  • man/plot_curve_params_one_ab.Rd: Language not supported
  • man/reexports.Rd: Language not supported
  • man/serocalculator-package.Rd: Language not supported
  • man/sim_pop_data.Rd: Language not supported
  • man/sim_pop_data_multi.Rd: Language not supported
  • man/simcs.tinf.Rd: Language not supported
  • man/simresp.tinf.Rd: Language not supported

Comment thread DESCRIPTION
@d-morrison

Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

@d-morrison d-morrison removed their request for review May 13, 2026 09:42
Copilot AI and others added 2 commits May 13, 2026 10:50
…conflict

Resolved conflict by keeping main's version that removes fun/n/args from ...
docs (those args are now explicitly blocked with an informative error).

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
…line)

Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>

Copilot AI commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Done in commit 0afa122. Merged main (af35f864) into the branch. The only conflict was in man/plot_curve_params_one_ab.Rd — resolved by keeping main's version, which removes fun, n, and args from the ... docs (those args are now blocked with an informative error in the implementation).

Copilot AI requested a review from d-morrison May 13, 2026 10:53
github-actions Bot added a commit that referenced this pull request May 13, 2026
@d-morrison

Copy link
Copy Markdown
Member

@claude review

@d-morrison

Copy link
Copy Markdown
Member

@claude review

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 2m 59s —— View job


PR Review

  • Read repo context and changed files
  • Review workflow changes
  • Review other changed files
  • Analyze existing review comments
  • Post final review

Summary

The core fix — ensuring the R package cache is saved before the long Build docs step — is well-designed and correctly implemented. The approach (explicit restore + cache: false on setup-r-dependencies + explicit save) is the right pattern for this use case. The current diff is clean: only 5 files changed against origin/main.


.github/workflows/docs.yaml

The cache strategy is sound:

  • id: setup-r correctly exposes installed-r-version for use in the cache key.
  • restore-keys fallback allows partial cache hits when DESCRIPTION changes — prevents cold starts on every dependency bump.
  • cache: false on setup-r-dependencies eliminates the conflicting post-step save. Without this, the built-in action could still try to save a different cache key in a post-step, creating confusion.
  • Immutable cache: GitHub's cache is immutable per key, so if an exact hit is restored, the subsequent cache/save is a safe no-op.
  • No if: always() on save step: This is intentionally correct — if Install R package fails, we don't want to persist a partially-installed library. The save only runs on success, which is the desired behaviour.

One observation: **/DESCRIPTION in hashFiles() will only scan the workspace, not R_LIBS_USER, so the hash is stable between restore and save. This is fine.


inst/WORDLIST ⚠️

Serological is removed from the wordlist, but the word appears in several source-controlled files:

  • DESCRIPTION:3Title: Estimating Infection Rates from Serological Data
  • man/serocalculator-package.Rd:7 — same title
  • vignettes/references.bib — bibliography entry titles

If Serological was originally added to WORDLIST because the hunspell dictionary used by r-spellcheck-action doesn't recognise it, removing it risks breaking the spellcheck CI. Please verify that the spellcheck workflow passes with this removal before merging. Fix this →


NEWS.md ⚠️

There is a missing blank line between the new changelog entry and the ## Compatibility section (line 13–14):

- Docs workflow now saves the R package cache immediately after installation (before the long `Build docs` step), so cancellations no longer result in a cold cache on the next run (#516)
## Compatibility

Should be:

- Docs workflow now saves the R package cache immediately after installation (before the long `Build docs` step), so cancellations no longer result in a cold cache on the next run (#516)

## Compatibility

Fix this →


.Rbuildignore / .gitignore

Both correctly updated to ignore .positai and .claude. These match, consistent with the reviewer's earlier suggestion.


Existing reviewer concerns (status)

The earlier review comments from copilot-pull-request-reviewer about sim_pop_data.Rd, sim_pop_data_multi.Rd, and DESCRIPTION/RoxygenNote are not reflected in the current diff — those Rd and DESCRIPTION changes appear to have been reverted and are not present in the 5-file diff against origin/main. The branch is clean of those issues as it currently stands.


Bottom line

The workflow fix is correct and addresses a real problem. Two minor items to resolve before merging:

  1. Verify the spellcheck CI passes after removing Serological from WORDLIST
  2. Add the missing blank line in NEWS.md

github-actions Bot added a commit that referenced this pull request May 15, 2026
@d-morrison

Copy link
Copy Markdown
Member

@claude fix issues (including workflow errors)

@d-morrison d-morrison removed their request for review May 15, 2026 03:57
@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @d-morrison's task in 1m 53s —— View job


Fixes Applied

  • Read CLAUDE.md for repo-specific guidelines (no CLAUDE.md found)
  • Check CI status — 1 failing workflow: Version increment check
    • Root cause: both PR branch and main were at 1.4.0.9010; CI requires PR version > main version
  • Investigate inst/WORDLISTSerological is still present; spellcheck CI passes ✅
  • Fix DESCRIPTION — bumped version 1.4.0.90101.4.0.9011
  • Fix NEWS.md — added missing blank line before ## Compatibility
  • Committed and pushed (commit a729184)

Summary: Two issues resolved in commit a729184:

  1. Version increment (DESCRIPTION): the PR branch was at the same version as main (1.4.0.9010), causing the CI version check to fail. Bumped to 1.4.0.9011.
  2. NEWS.md formatting: added the missing blank line between the #516 cache entry and the ## Compatibility section heading.

- Increment version past main branch (both were at 1.4.0.9010)
- Add missing blank line before ## Compatibility in NEWS.md

Co-authored-by: Douglas Ezra Morrison <d-morrison@users.noreply.github.com>
github-actions Bot added a commit that referenced this pull request May 15, 2026
github-actions Bot added a commit that referenced this pull request May 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📖 https://UCD-SERG.github.io/serocalculator/preview/pr516/
Preview documentation for this PR (at commit 98a0a16)

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.

in docs workflow, cache dependencies immediately after installing them

3 participants