Gate pkgType = 'source' on ref, own it in val_build() only (fixes #181) - #182
aclark02-arcus wants to merge 10 commits into
Conversation
val_pipeline() used to push options(pkgType = 'source') into the parent session before dispatching val_build(), unconditionally and outside the ref gate. That pre-set state changed how configure_bioc_repositories_if_requested() -- which val_build() calls near entry -- resolved its available.packages() cache, so the same val_build(pkg_names = 'logrx', ref = 'source', ...) returned >90% covr_coverage when called directly but ~62% when called through val_pipeline(). Also fixes val_build's broken on.exit(function() options(old)) -- which constructed a function value and threw it away, never restoring the option snapshot, and silently wiped an earlier on.exit(options(old_cfg), add = TRUE) because it lacked add = TRUE. Ownership of the pkgType slot now lives in exactly one place: val_build(), via the extracted apply_val_build_options() helper, gated on ref == 'source' and set BEFORE the bioc-config helper runs. Verified: * devtools::test(filter = 'apply_val_build_options'): 17/17 pass. * devtools::test() full: 1088 pass, 0 fail (1 pre-existing skip). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Aaron flagged that essentially every real val.pipeline run uses ref='source' (that's what riskmetric needs to produce R CMD check + covr_coverage), so the ref gate is effectively always-true. That means the previous version of this PR -- which moved the pkgType set to BEFORE configure_bioc -- would have regressed bare val_build to the same 62% coverage that was reported through val_pipeline. The historical bare-val_build ordering (configure_bioc first, then set pkgType) is the state that produced the accurate >90% number on logrx. Preserve that. The fix for the val_pipeline path stays: drop the outer-seam pkgType pre-set so val_pipeline enters val_build with pkgType at the caller's default, matching what bare val_build has always done. Test suite: 1088/1088 pass (0 fail, 1 pre-existing skip). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Push That means the earlier commit Corrected ordering: val_pipeline() fix stays intact: the outer seam's unconditional Verified: full |
Reads dev/low_covr.rds (gitignored, sensitive) and drives val_build() against a filtered high-yield / low-cost subset (default: covr > 40% AND runtime <= 15 min) with replace = TRUE. Post-run diffs against the prior covr_coverage values and reports which pkgs cleared the 65% threshold. Companion .gitignore entries keep the sensitive rds inputs + generated compare tables out of the tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The rerun utility cannot read post-run coverage correctly, has a broken default config path, and the documented option ordering contradicts the implementation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR centralizes pkgType handling in val_build() to address inconsistent coverage results and option leakage.
Changes:
- Adds conditional option setup and restoration.
- Removes unconditional
pkgType = "source"fromval_pipeline(). - Adds tests, release notes, and a low-coverage rerun utility.
File summaries
| File | Description |
|---|---|
R/utils.R |
Adds the option-management helper. |
R/val_build.R |
Applies and restores build options. |
R/val_pipeline.R |
Removes unconditional pkgType setup. |
tests/testthat/test-apply_val_build_options.R |
Tests helper behavior. |
dev/dev_rerun_low_covr.R |
Adds targeted reassessment tooling. |
.gitignore |
Excludes rerun artifacts. |
DESCRIPTION |
Bumps version to 0.1.60. |
NEWS.md |
Documents the fix. |
Review details
Suppressed comments (1)
dev/dev_rerun_low_covr.R:214
- The comparison reads
_meta.rds, butval_pkg()'s metadata bundle does not containcovr_coverage(R/val_pkg.R:934-1061); the raw scalar is stored in_assess_record.rds. Consequently this condition is false for every successful rerun, socovr_after, every delta, and the threshold count are alwaysNA/zero. Read the assessment record instead.
m <- tryCatch(readRDS(meta_path), error = function(e) NULL)
covr_after <- if (!is.null(m) && "covr_coverage" %in% names(m)) {
as.numeric(m$covr_coverage)
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # state between `val_pipeline() -> val_build()` and bare `val_build()` | ||
| # and silently regressing covr_coverage on some source-tier packages. | ||
|
|
||
| test_that("apply_val_build_options(ref = 'source') sets repos AND pkgType", { |
| # Establish `options(repos, pkgType)` AFTER | ||
| # `configure_bioc_repositories_if_requested()` on purpose: the bioc | ||
| # helper's `available.packages()` probe must run under whatever | ||
| # `pkgType` the caller's session carried at entry -- pre-poisoning | ||
| # it with `pkgType = "source"` produces a different bioc-repo cache |
Three adoptions from Copilot's review of #182: 1. R/val_build.R: rewrite the comment above apply_val_build_options() to accurately name which helper's available.packages() call is pkgType-sensitive. Copilot was right that `configure_bioc_repositories_if_requested()` doesn't call available.packages() -- it only installs a session-scoped `assignInNamespace('repositories', ...)` shim on BiocManager. The offender is the NEXT helper we run, in the same block: `configure_riskmetric_offline_if_requested()`, which does call available.packages() to build its offline cache (R/bioc.R). The fix (setting options AFTER both helpers) is unchanged; only the rationale needed correcting. 2. tests/testthat/test-apply_val_build_options.R: add two source-scan regression tests. The pkgType leak was a call-path bug, not a helper-signature bug -- the pre-existing unit tests can't catch a re-introduction of unconditional `options(pkgType = 'source')` at the val_pipeline() -> val_build() seam. New tests grep the installed R sources and assert: - val_pipeline.R's post-prep options() block does NOT set pkgType. - val_build.R sets options AFTER configure_riskmetric_offline_if_requested(). Test file: 17 -> 22 assertions, all pass. 3. dev/dev_rerun_low_covr.R: fix config_path fallback. Previously defaulted to `file.path(getwd(), 'config.yml')` which errors when the script is run outside a launcher project. Now defaults to NULL, which val_build() resolves to the packaged `inst/config.yml`. Also updated the config_path echo to print '<val_build default>' when NULL, so operators can eyeball what resolution they're getting without opening val_build(). Tests: `devtools::test(filter = 'val_')` -> 218/218 pass. Refs #181, #182. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Adopted all 3 Copilot review suggestions in 0548949, plus a PR body refresh: 1. 2.
Test file: 17 → 22 assertions, all pass. 3. PR body refresh. Copilot's third comment on the same file noted the PR description contradicted the current code (it read as if options were set BEFORE the bioc helpers — that was the pre-course-correction narrative). Rewrote the body to reflect: (a) the current AFTER-the-helpers sequencing, (b) the 6d3d7a5 course-correction rationale, (c) the correct offender helper name, (d) the regression tests added post-review. Verification:
|
Aaron's empirical result: entering val_build under
getOption('pkgType') == 'source' regresses covr_coverage on some
source-tier packages (logrx: >90% -> ~62%). Under 'both' or 'binary'
the number holds at >90%. Intuition (source-tier assessment =>
pkgType='source') is exactly wrong.
Likely mechanism: configure_riskmetric_offline_if_requested() runs
near the top of val_build's body and calls
utils::available.packages(), whose default 'type = getOption("pkgType")'
filters the offline cache. Under 'source', binary-only pkgs from a
PPM-style mirror drop out of the cache and downstream test-time
installs fail silently -- the test suite short-circuits and covr
reports the resulting partial coverage. 'both' keeps the cache
complete (binary-preferred, source fallback) while 'ref = "source"'
still drives val.pipeline's own source-tarball assessment path in
val_pkg().
Changes:
- R/utils.R: apply_val_build_options() sets pkgType='both' when
ref='source', not pkgType='source'. Updated roxygen w/ the full
rationale.
- R/val_build.R: reworded the comment above apply_val_build_options()
to explain the 'both, not source' direction and the offline-cache
mechanism.
- tests/testthat/test-apply_val_build_options.R: updated the
primary source-tier test to start from pkgType='binary' (so the
helper's mutation is observable) and assert pkgType=='both' post-
call. Added a dedicated regression test asserting the helper
NEVER sets pkgType='source' under ref='source' -- forces any
future attempt to flip this back to the intuitive-but-wrong
direction to fail loudly.
- NEWS.md: expanded the 0.1.60 bullet to describe both prongs of
the fix (removal of val_pipeline's unconditional set + the
correction from 'source' to 'both' in val_build).
Tests: devtools::test(filter = 'val_') -> 219/219 pass.
Refs #181.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Course correction in a57bdc9: the true fix is You reported empirically that entering val_build under
New model:
Applied in a57bdc9:
Behavior change for bare Tests: Ready for one more workbench A/B on the low-covr cohort before merge. |
Companion helper for the low-covr re-run driver
(dev/dev_rerun_low_covr.R) and any other val_build(replace = TRUE)
cohort: reports done / remaining / throughput / ETA by filtering
assessed/<pkg>_<ver>_meta.rds by mtime relative to a caller-supplied
run_started timestamp.
The mtime filter is necessary because replace = TRUE overwrites
metas in place -- a naive file-count is fooled by pre-existing metas
from an earlier run.
Inputs (via globals in caller's env):
- pkgs : the cohort vector passed to val_build().
- run_started : POSIXct or 'YYYY-MM-DD HH:MM:SS' string; the kickoff
moment (or Sys.time() - elapsed_hr if only the
duration is known).
- out, val_date: standard val_build layout knobs; the script derives
val_dir = <out>/R_<Rver>/<YYYYMMDD>/.
Outputs:
- Console summary (cohort size, done, remaining, throughput, ETA).
- `progress` data.frame in caller env, one row per cohort pkg,
status in {done, pre_existing_meta_not_touched, no_meta}.
- `remaining_pkgs` vector for follow-up.
- Peek: 20 most-recent completions + first 20 remaining.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Companion to dev/dev_probe_run_progress.R + dev/dev_rerun_low_covr.R. Motivating scenario: the #181 fix (pkgType='both') lifts covr on some packages but may regress others; we need a thorough per-pkg delta to inspect regressions individually and, ideally, cluster them into a mechanism. Inputs (globals in caller env): - old_qa / new_qa: data.frame OR path to qual_assessments RDS. new_qa may also be a val_dir/ path, in which case the script walks assessed/*_meta.rds and reconstructs a qual_assessments- shaped frame -- important for runs kicked off with finalize = FALSE (top-level qual_assessments.rds not regenerated at end of assessment loop). - old_qm / new_qm: (optional) qual_metadata for decision-category diff. - pkgs: (optional) restrict diff to a cohort. - out_diff: (optional) output path; defaults to dev/covr_diff_<YYYYMMDD_HHMMSS>.rds (gitignored). Outputs (console): - Cohort membership (old-only / new-only / shared). - Metric column presence diff (catches silent config drift). - Coverage-band shift table (NA/0 through 90-100). - Improved/regressed/unchanged counts w/ +/- 5 pp thresholds. - Top-20 regressions + top-20 improvements. - Decision-category shift table (if qm supplied). Outputs (globals): diff_df, regressed_df, improved_df. .gitignore: add dev/covr_diff_*.rds pattern (output RDS may echo sensitive pkg names / assessment data). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Redesigned dev/dev_diff_run_assessments.R to support two workflow
needs Aaron surfaced while inspecting a mid-run cohort:
1. Visual + shareable output. In addition to the console tables, the
script now writes a self-contained HTML report to
<out_dir>/covr_diff_<run_tag>.html -- inline CSS, no external
assets, safe to email or drop in Slack. Green/red-highlighted
delta cells; band-shift and decision-shift matrices rendered as
HTML tables; per-section notes explaining what each list is
for. run_tag defaults to a timestamp; can be overridden.
2. Rev-dep-weighted top-20 lists on top of the raw delta ranks:
- UNLOCKERS: pkgs that crossed the 65% threshold UPWARD, ranked
by rev-dep count. Highest rev-dep count = largest downstream
slice of the qualified set unlocked by this pkg entering the
qualified pool. These are the run's leverage wins.
- BLOCKERS: pkgs still BELOW 65% after the run (regressed
across the line OR were already below and stayed below),
ranked by rev-dep count. Biggest downstream blockers surface
first -- highest-value investigation targets for follow-up
since unblocking them propagates.
Rev-dep source priority: reverse_dependencies column on new_qa ->
same column on old_qa -> fallback derived from qm's
Depends/Imports/LinkingTo columns -> NA w/ friendly warning.
Also added:
- crossed_up / crossed_down / below_after boolean flags on diff_df
so callers can slice however they want.
- covr_status categorical ({improved, regressed, unchanged,
lost_metric, new_only, both_na}) for easier filtering.
- version_changed flag preserved from prior draft -- caller can
eyeball which regressions coincide with upstream bumps vs which
are pipeline-side.
.gitignore: added dev/covr_diff_*.html to the existing rds pattern.
Smoke test with synthetic frames: 50 shared pkgs, forced
unlocker/regression rows -> HTML renders cleanly (13.4 KB), all
five global data.frames populate, band-shift matrix and top-20
tables all print.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
qual_metadata frames written by val_build() key on `pkg`, not
`package` (that's qual_assessments's convention). The diff script
hard-coded 'package' when subsetting qm for the decision-category
diff, producing:
Error: Can't subset columns that don't exist.
Column `package` doesn't exist.
Fix: new `.qm_key_col()` helper checks c('package', 'pkg',
'pkg_name') in order and picks the first hit. Applied at the qm
merge site; the built decision-diff frame is normalized to
'package' internally so the downstream merge onto diff_df still
keys correctly.
Also drop a dead `names(counts) <- qm_src$package` line in the
rev-dep qm-derivation fallback that likewise assumed 'package' and
was never actually used (counts was unpopulated; dep_tbl is built
independently from all_deps).
Smoke tested with a qm frame keyed on 'pkg' + final_decision col:
decision-shift table populates cleanly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bakes the covr-vs-pkgType mechanism write-up into the HTML report
so it travels with the tables when shared with colleagues.
Contents (styled as a highlighted callout box below the summary
cards, above the delta tables):
- Explicit statement that the ONLY pipeline-side change between
the two runs is options(pkgType) flipping from 'source' to
'both' -- no metric defs, thresholds, decision rules, or
{riskmetric} version changes.
- Why covr doesn't care about the tarball: it instruments the
install and runs the test suite in a subprocess, so target
code is byte-identical either way.
- Where the delta comes from: dependency install success.
Comparison table of source vs both vs binary and their common
failure modes.
- Two amplifiers specific to val.pipeline + PPM:
1. available.packages() type filter narrows the offline cache.
2. Binary-only mirrors mean 'source' can render packages
invisible even though they exist.
- Why the number swings 20+ pp, not just a little.
- Diagnostic tips for inspecting a specific regressor:
read assess_record.rds's covr slot for 'no package called X'
errors, grep val_pipeline.log for compilation failures.
CSS: added a .context styleset (yellow callout, warm border,
inner table with muted grid) so the write-up reads as
'meta / interpretation' rather than as another data table.
Smoke-tested: HTML contains the section, all downstream tables
still render.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #181.
Root cause recap
Two coupled problems in
val_pipeline() -> val_build()regressed logrx covr_coverage from >90% (bareval_build()) to ~62% (throughval_pipeline()):val_pipeline()pushedoptions(pkgType = "source")into the parent session unconditionally, outside therefgate.val_build()itself also setpkgType = "source"whenref == "source". This is the intuitive setting for a "source-tier assessment", but it's backwards. Confirmed empirically: enteringval_build(ref = "source", ...)undergetOption('pkgType') == 'source'reproduces the ~62% number; entering under"both"or"binary"gives >90%.Likely mechanism (
pkgType = "source"breaks source-tier covr)val_build()callsconfigure_riskmetric_offline_if_requested()near entry, which usesutils::available.packages()to build an offline cache.available.packages()defaultstype = getOption("pkgType"), so under"source"the cache is filtered to source-only. Any test-time dependency that exists in the caller's PPM-style mirror only as a binary drops out of the offline cache. Downstream, when covr instruments the target package and its test subprocess needs those deps, the install probe fails silently and the test suite short-circuits — covr reports the resulting partial coverage as the package's "real" number.pkgType = "both"keeps the cache complete (binary-preferred, source fallback) whileref = "source"still drives val.pipeline's own source-tarball assessment path inval_pkg(). That's what this PR now enforces.What this PR does
apply_val_build_options(ref, opt_repos)intoR/utils.R. Setsrepos(always) and, whenref == "source", setspkgType = "both"(NOT"source"). Returns the prior slot values in anoptions()-shaped list ready foron.exit(options(old), add = TRUE).pkgType = "source"atval_pipeline.R:286.val_build()owns pkgType now.on.exit(function() options(old))inval_build(). It constructed a function value and threw it away, never restoring options — and lackedadd = TRUE, silently wiping the earlieron.exit(options(old_cfg), add = TRUE).Regression tests
Six tests in
tests/testthat/test-apply_val_build_options.R:ref = "source"setsrepos+pkgType = "both";ref = "remote"setsreposonly, leavespkgTypealone).options(old)fully undoes the mutation).apply_val_build_options(ref = "source", ...)NEVER setspkgType = "source". Forces any future attempt to flip this back to the intuitive-but-wrong direction to fail loudly.val_pipeline.Rpost-prep options block does NOT setpkgType.val_build.Rsets options AFTERconfigure_riskmetric_offline_if_requested()(kept AFTER for scope isolation, though the value now — not the ordering — is what actually carries the fix).What this PR intentionally does NOT touch
val_prep_pipeline.R:210andval_decision.R:287also setpkgType = "source"unconditionally, but their side effects are contained byval_prep_pipeline()'s ownon.exit(options(old), add = TRUE)guard (seeR/val_prep_pipeline.R:142). They don't leak intoval_pipeline()'s post-prep scope, so they aren't the root cause. Can be revisited in a follow-up if desired — but the same "preferboth" logic likely applies whereverpkgTypeis set in a val.pipeline scope.Verification
devtools::test(filter = "apply_val_build_options")→ 23/23 pass.devtools::test(filter = "val_")→ 219/219 pass.devtools::test()(last full-run baseline) → 1088 pass, 0 fail, 1 pre-existing skip.User-visible expected impact
val_pipeline()callers get higher covr_coverage on any package whose test-time deps included binary-only mirror entries. Aaron's 2026-08-25 run had ~600 packages below the 65% threshold; some will now land above threshold on the next run.val_build(ref = "source")behavior CHANGES: previously it setpkgType = "source", now it setspkgType = "both". Callers who invokedval_build()directly and depended on the oldpkgType = "source"side effect for downstream install behavior will see the same > 90% coverage improvement.ref = "remote"runs are unaffected —pkgTypeis left at the caller's value in either case.Companion dev script
dev/dev_rerun_low_covr.R(added on this branch): surgical driver for a caller's low-covr cohort — filters aqual_assessments × qual_metadata-merged snapshot to packages likely to benefit from the fix (defaultcovr_coverage > 40 AND runtime <= 15 min), reruns them viaval_build(replace = TRUE, prep = NULL, finalize = FALSE), and prints a before/after covr_coverage delta.config_pathdefaults to NULL (resolves to packagedinst/config.yml) so the script runs cleanly outside a launcher project.Version + NEWS
# val.pipeline 0.1.60heading, describing both prongs of the fix.Note: PR #180 also bumps to 0.1.60. Whichever merges first wins; whichever merges second needs a rebase-bump to 0.1.61.