Adapt to GAMBLR.data's SQLite backend; fix seq_type data-loss bugs; performance and example fixes#16
Open
rdmorin wants to merge 30 commits into
Open
Adapt to GAMBLR.data's SQLite backend; fix seq_type data-loss bugs; performance and example fixes#16rdmorin wants to merge 30 commits into
rdmorin wants to merge 30 commits into
Conversation
Without metadata the function silently returns SSMs for all samples, which can be unexpectedly large. A warning now directs the user to supply these_samples_metadata to limit results to relevant samples. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The > 1 threshold meant a seq_type with only one sample would be silently excluded from the result. Changed to > 0 so any non-empty set of sample IDs is included. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Defensive guard against future addition of mrna data to GAMBLR.open, consistent with GAMBLR.results behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Data-source messages in get_gambl_metadata, collate_results, and calc_mutation_frequency_bin_region now fire only once per session using an options() flag, preserving the safeguard for users who accidentally load GAMBLR.open over GAMBLR.results. The "processing bins" message is gated behind a new verbose = FALSE parameter. Explicit by = arguments added to three joins in calc_mutation_frequency_bin_region to suppress dplyr's "Joining with" messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All data-source messages now fire only once per session using options() flags, including bundled SSM, CN segment, and Manta SV messages. Startup logo and welcome banner also limited to once per session. Explicit by= added to joins in collate_results and get_ashm_count_matrix to suppress dplyr "Joining with" messages. library(dplyr) and library(GAMBLR.open) calls in examples wrapped in suppressPackageStartupMessages(). Fixed long-standing typo in id_ease warning: set_type -> seq_type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Companion doc update for the GAMBLR.data refactor that moves Study/ cohort membership off maf/ashm rows into a sample_study join table. No functional change needed here -- this function already only forwards this_study into get_ssm_from_db() and never reads maf.Study directly.
devtools::run_examples() (used by tools/logExampleOutputs.R) has document = TRUE by default -- every single run calls devtools::document() (regenerating every .Rd file + NAMESPACE via roxygen2) and reloads the package via load_all() twice (once explicitly, once again via on.exit()), all before a single example actually runs. That's a real, fixed cost on every run, unrelated to any actual example content or database query performance. This alternative skips all of that: man/*.Rd is already committed and up to date, so it reads examples directly from there via tools::parse_Rd() + tools::Rd2ex(), loads the package once (a plain library() call by default, or pkgload::load_all() via --load-all for uncommitted source changes), and times each example file individually. Output: a full log (GAMBLR_examples_output_fast.log, same shape as the original) plus a per-file timing TSV sorted slowest-first, so it's actually possible to see which examples dominate the runtime instead of only a single start/end timestamp for the whole suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
calc_mutation_frequency_bin_regions() looped over every region calling calc_mutation_frequency_bin_region() -> get_ssm_by_region(), each a separate database round trip. For the full aSHM region set (100+ regions) this was the single largest cost in the GAMBLR.open example suite (~140s of a ~240s total run, per tools/fast_example_timer.R's per-file timing). calc_mutation_frequency_bin_region() already supports a maf_data parameter that skips the database entirely in favour of in-memory subsetting via cool_overlaps() -- the plural function just never used it. Now it pre-fetches all mutations across every (correctly re-padded) region in one get_ssm_by_regions() call before the loop, so every per-region call takes that existing fast path instead. cool_overlaps()'s own overlap matching is already inclusive-boundary (start2 >= start1 & end2 <= end1), matching get_ssm_by_regions()'s 1bp-widened SQL boundary semantics exactly, so this doesn't change what gets counted -- verified with a stubbed-out control-flow test confirming the padding math matches process_regions()'s own formula and every per-region call receives the correct pre-fetched data, plus that the existing "caller already supplied maf_data" path is unchanged. Also replaced mclapply() with lapply(): once each iteration is just fast in-memory subsetting rather than a database call, there's little left to gain from forking, and mclapply()'s fork-per-worker model both multiplies peak memory (each fork copies the parent's memory) and forking a live DB connection across processes is a real correctness/contention risk this avoids entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_data The implementation already queries gambl_mutations.db via GAMBLR.data::get_ssm_from_db() (see the refactor earlier in this branch's history) -- the description/details text just never got updated to match when that change landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to dfaa8d9 (calc_mutation_frequency_bin_regions: bulk pre-fetch + mclapply -> lapply), which removed the last use of the parallel package but deliberately left NAMESPACE's now-stale import(parallel) alone rather than hand-edit a generated file, pending a real devtools::document() run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e_data.rda reads get_cn_segments()/get_sample_cn_segments() now query GAMBLR.data::get_cn_segments_from_db() instead of the bundled sample_data$<projection>$seg object, and their examples use get_gambl_metadata() instead of GAMBLR.data::sample_data$meta.
rdmorin
changed the title
# Conflicts: # GAMBLR_examples_output.log # R/get_ssm_by_regions.R
Covers the transitive Imports of all 6 GAMBLR packages (GAMBLR.data, GAMBLR.helpers, GAMBLR.utils, GAMBLR.viz, GAMBLR.predict, GAMBLR.open) as conda-forge/bioconda binaries, so installing the GAMBLR packages themselves on top requires no source compilation of dependencies -- intended for automated pipeline installs (e.g. Snakemake/LCR-modules). Only r-base is pinned (to 4.3, matching GAMBLR.data's CI); system libraries are left for the solver to pick consistently, rather than hand-pinned, per the lesson from GAMBLR.data's envs/r.yaml drifting out of sync with conda-forge's own gcc-14 migration. Validated via a full dry-run solve (355 resolved packages, all 52 explicitly listed deps confirmed present, no conflicts).
Dockerfile builds directly from envs/gamblr_collective.yaml (so the image can't drift out of sync with the conda env) plus install_gamblr.R, which installs the six GAMBLR packages from GitHub with dependencies = FALSE, trusting the conda-provided binaries. release_container.sh is the admin-only build+push script (mirrors GAMBLR.data's release_mutations_db.R: refuses to run with an uncommitted DESCRIPTION version bump, since the image tag is derived from it). Defaults to installing all six packages from master; ref env vars can override individual packages for testing open PR branches pre-merge, in which case the floating "latest" tag is deliberately left untouched. Since Apptainer/Singularity can pull a Docker image directly (apptainer pull docker://...), this one image covers both of LCR-modules' install modes -- no separate Apptainer definition needed. Full process documented in docker/RELEASING.md.
…ollective Runs docker/release_container.sh unchanged on GitHub's runners, avoiding the local Docker/disk-space/GHCR-credential requirements of running it by hand. workflow_dispatch-only (not triggered on tag/release push), matching the same maintainer-controlled trigger already chosen for the script itself -- a build+push only happens when someone deliberately runs it, either via the Actions UI or `gh workflow run`. Includes a disk-cleanup step since standard runners' ~14GB free disk is a known constraint against a 350+ package Bioconductor-heavy solve; not yet verified against a real build since local Docker wasn't available to test against.
rdmorin
added a commit
that referenced
this pull request
Jul 16, 2026
Adds only the workflow definition to master so GitHub's dispatch API recognizes it -- workflow_dispatch workflows must exist on the default branch to be runnable at all, even when dispatched against a different ref. This file has zero effect on its own (no automatic trigger); the actual Docker/install scripts it invokes remain on rmorin-dev pending PR #16. Added to test that PR's container build before merge.
…e with retry First test build (run 29517602438) failed installing GAMBLR.data: download.file() hit its default 60s timeout fetching the GitHub tarball for @rmorin-dev, failing at exactly 60.7s. Raise options(timeout) to 600s, and switch from a single install_github(vector) call to installing each package in order with retry-on-failure -- the vector form let GAMBLR.data's failure cascade into 5 more doomed downstream installs before the final check caught it, which is both slower and a less useful log to debug from.
rdmorin
commented
Jul 16, 2026
| @@ -0,0 +1,89 @@ | |||
| name: Release gamblr-collective container | |||
Contributor
Author
There was a problem hiding this comment.
Note: this is already tested and confirmed working. Entire build and installation of all GAMBLR packages (except results) completes in just over 12 minutes!
The Actions-triggered build was actually validated end-to-end (both open PRs installed successfully, 12m36s), including catching and fixing a real install_github() timeout bug along the way. The local script path is logically identical (same script, unchanged) but was never independently re-run, so present it as a documented fallback rather than a coequal option. Also documents the org-level package-visibility gotcha hit while testing: GHCR packages default to private regardless of the linked repo's visibility, and the morinlab org had public package creation disabled entirely at the org settings level, which had to be enabled before the per-package visibility toggle would work.
Kdreval
reviewed
Jul 16, 2026
| cli (>= 3.6.1), | ||
| dplyr, | ||
| GAMBLR.data (>= 1.3), | ||
| GAMBLR.data (>= 1.4), |
Contributor
There was a problem hiding this comment.
just flagging this that it will have to be bumped up to 1.5 once the .data PR is merged.
The 1.4 release marks the current version before the co-dependent PR with updates relevant here is merged, so requiring here 1.4 version will install the one at the current tag and therefore will be missing all of these updates
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
2 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
get_coding_ssm,get_cn_segments,get_ssm_by_regions, etc.) to querygambl_mutations.dbvia GAMBLR.data's new accessor functions instead of the bundledsample_dataobject;this_studynow resolves via GAMBLR.data'ssample_studytableget_all_coding_ssm, missing warning when no metadata supplied, mRNA seq_type incorrectly includedcalc_mutation_frequency_bin_regions()now bulk pre-fetches all region mutations in one call instead of one query per region; dropsmclapplyforlapply(memory-multiplication concerns)GAMBLR.data::sample_datadirectlytools/fast_example_timer.R, a leaner per-example-timed alternative todevtools::run_examples()GAMBLR.data (>= 1.4)options()flags),get_ssm_by_samplealias matching the GAMBLR.results API,id_easetypo fix,@examplescleanupTest plan
tools/fast_example_timer.Rrun against the full example suite: ~130s (GSC), ~33.8s (laptop, same DB/code)FL_Dreval/DLBCL_cell_linescoding-variant query correctness after the GAMBLR.data schema migrationget_coding_ssm()triggers GAMBLR.data's auto-download and returns correctly-tagged data end to end🤖 Generated with Claude Code