refactor(dataProcess): Stabilize Child Process RAM usage for parallel processing - #218
refactor(dataProcess): Stabilize Child Process RAM usage for parallel processing#218tonywu1999 wants to merge 15 commits into
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
R/MSstatsSummarizeWithMultipleCores.R (2)
795-799: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass
drop = TRUEtosplitto avoid empty protein slots.
splitdefaults todrop = FALSE. With factor keys and a two-elementsplit_keys, it emits one element for every PROTEIN × LABEL combination, including combinations with no rows. Each empty group is packed, serialized to a worker, and returnslist(NULL, NULL)from the step-3 early exit at line 373.meta$PROTEINfor those slots isNA_character_.♻️ Proposed change
- protein_indices <- split(seq_len(nrow(input)), split_keys) + protein_indices <- split(seq_len(nrow(input)), split_keys, drop = TRUE)Run the following script to confirm the key column types and how downstream code consumes the result names:
#!/bin/bash # Check whether PROTEIN/LABEL are factors and how summarization results are consumed. rg -nP -C 5 'MSstatsSummarizeWithMultipleCores\s*\(' --glob 'R/*.R' echo '--- LABEL / PROTEIN factor coercion ---' rg -nP -C 3 '(PROTEIN|LABEL)\s*:=\s*factor' --glob 'R/*.R'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 795 - 799, Update the split call in MSstatsSummarizeWithMultipleCores around protein_indices to pass drop = TRUE, preventing empty PROTEIN × LABEL groups from being created and dispatched. Preserve the existing split_keys construction, protein_ids naming, and num_proteins calculation.
138-184: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftThe packed layout always allocates five dense FL×R matrices, including constant-fill ones.
mat_ABUandmat_anombecome all-NA matrices when their source columns are absent, andmat_censbecomes all-zero. The packed vector still reservesFL*Rdoubles for each of them..MSstatsSummarizeSingleTMPV2then skips ABUNDANCE (line 350) and ANOMALYSCORES (line 353) outright.For the TMP path this inflates the per-protein payload by about 2.5x over the three matrices actually read. That payload is what
bplapplyserializes to every worker, so it directly raises the peak RAM this PR aims to stabilize.Consider storing a per-slot presence bitmap in
metaand omitting absent or constant-fill sections frompacked. Keep the section offsets derived from the bitmap so.reconstructProteinDTV3and.MSstatsSummarizeSingleTMPV2stay in sync with the layout comment at lines 86-99.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 138 - 184, Update the packing logic around packed and the per-slot metadata to record presence for ABUNDANCE and ANOMALYSCORES, and omit their all-NA sections; omit the constant-fill censored section when appropriate as well. Derive section offsets from this bitmap and update .reconstructProteinDTV3 and .MSstatsSummarizeSingleTMPV2 to read only present sections while preserving synchronization with the documented layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@man/MSstatsSummarizeWithMultipleCores.Rd`:
- Around line 14-19: Update the documentation for
MSstatsSummarizeWithMultipleCores by adding \arguments{} entries for
aft_iterations, verbose, BPPARAM, and track_memory, matching the existing
parameter conventions. Replace the vague \value{} text with the concrete
returned list structure, including each element’s type and naming contract.
In `@R/MSstatsSummarizeWithMultipleCores.R`:
- Around line 422-423: Remove the unnecessary MSstats namespace qualifiers from
both worker calls: in R/MSstatsSummarizeWithMultipleCores.R lines 422-423,
update .fitSurvival invocation to use .fitSurvival directly; at line 532, update
median_polish_summary invocation to use median_polish_summary directly. No other
changes are needed; .warmupV6Worker already attaches MSstats for socket workers.
- Around line 697-781: Update the roxygen documentation for
MSstatsSummarizeWithMultipleCores to add `@param` entries for aft_iterations,
BPPARAM, and track_memory, and either connect verbose to the existing progress
or memory-reporting behavior or remove it from the function signature since it
is unused. Replace user-facing references to MSstatsSummarizeWithMultipleCoresV5
and “V6” with MSstatsSummarizeWithMultipleCores, then regenerate the
corresponding Rd file.
- Around line 515-536: The wide-matrix construction currently retains unobserved
LABEL/RUN combinations, causing all-NA rows to reach median_polish_summary.
Update the TMP preparation around wide_mat and median_polish_summary to retain
only observed label-run pairs, then build result_labels and result_runs from the
same filtered row set rather than every label-run combination. Preserve the
existing label/run ordering for observed pairs.
- Around line 18-42: Update .peakRSS_MB to avoid Rcpp compilation and POSIX-only
getrusage calls on unsupported platforms, returning NA_real_ when no safe
peak-RSS mechanism is available. Store any compiled helper in a package-local
environment rather than .GlobalEnv, and ensure failures in compilation or lookup
are caught so .reportWorkerPeakV6() and .printMemReport() continue with “n/a”
output.
---
Nitpick comments:
In `@R/MSstatsSummarizeWithMultipleCores.R`:
- Around line 795-799: Update the split call in
MSstatsSummarizeWithMultipleCores around protein_indices to pass drop = TRUE,
preventing empty PROTEIN × LABEL groups from being created and dispatched.
Preserve the existing split_keys construction, protein_ids naming, and
num_proteins calculation.
- Around line 138-184: Update the packing logic around packed and the per-slot
metadata to record presence for ABUNDANCE and ANOMALYSCORES, and omit their
all-NA sections; omit the constant-fill censored section when appropriate as
well. Derive section offsets from this bitmap and update .reconstructProteinDTV3
and .MSstatsSummarizeSingleTMPV2 to read only present sections while preserving
synchronization with the documented layout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9884130-c99e-4499-a07f-53ce312d7d8f
📒 Files selected for processing (9)
DESCRIPTIONNAMESPACER/MSstatsSummarizeWithMultipleCores.RR/dataProcess.Rman/MSstatsSummarizeWithMultipleCores.Rdman/dot-MSstatsSummarizeSingleTMPV2.Rdman/dot-buildProteinSlotV3.Rdman/dot-buildSummarizeWorkerV6.Rdman/dot-reconstructProteinDTV3.Rd
💤 Files with no reviewable changes (1)
- R/dataProcess.R
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
man/dot-build_summarize_worker.Rd (1)
8-16: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis Rd file has no
\argumentssection.
\usagedeclares six arguments, and no\itemdocuments them.R CMD checkreports "Undocumented arguments in documentation object '.build_summarize_worker'". Add@paramtags foruse_TMP,impute,censored_symbol,remove50missing,aft_iterations, andequal_varianceinR/MSstatsSummarizeWithMultipleCores.R, then regenerate this file. An@keywords internalfunction is still checked for argument documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@man/dot-build_summarize_worker.Rd` around lines 8 - 16, Add roxygen2 `@param` entries for use_TMP, impute, censored_symbol, remove50missing, aft_iterations, and equal_variance in .build_summarize_worker within MSstatsSummarizeWithMultipleCores.R, then regenerate the .build_summarize_worker Rd documentation so its \arguments section matches the six parameters declared by \usage.R/MSstatsSummarizeWithMultipleCores.R (3)
20-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.peak_rss_mbstill writes into.GlobalEnvand still compiles C++ at runtime without a fallback.Lines 36 and 62 assign compiled helpers into
.GlobalEnv.R CMD checkflags writes to the global environment. Line 50 also callsRcpp::cppFunctionwith<sys/resource.h>, which requires a C++ toolchain at run time. If compilation fails,track_memory = TRUEaborts the whole summarization. Store the helpers in a package-local environment and wrap compilation intryCatchthat returnsNA_real_, so.print_memory_reportprints"n/a".This repeats an earlier review comment that was marked as addressed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 20 - 65, Update .peak_rss_mb to store compiled helper functions in a package-local environment instead of .GlobalEnv, and wrap both Rcpp::cppFunction calls and helper invocation in tryCatch. Return NA_real_ whenever compilation or measurement fails, preserving the existing platform-specific memory paths so .print_memory_report can display “n/a” without aborting summarization.
608-640: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWorker errors pass silently into
results.
SnowfastParam(stop.on.error = FALSE)makesbplapplyreturnconditionobjects in place of failed elements instead of raising. Line 639 collects those objects and line 640 names them, so a partially failed run looks like a successful one.MSstatsSummarizeWithSingleCorehas no equivalent behavior, andMSstatsSummarizationOutputinR/dataProcess.Rreceives the mixed list. Inspect the results withBiocParallel::bpok()after dispatch, then log or raise for the failed slots.🛡️ Proposed check
results <- BiocParallel::bplapply(protein_records, worker_fn, BPPARAM = BPPARAM) names(results) <- protein_ids + failed <- !BiocParallel::bpok(results) + if (any(failed)) { + msg <- paste0(sum(failed), " protein slot(s) failed during summarization: ", + paste(utils::head(protein_ids[failed], 10L), collapse = ", ")) + getOption("MSstatsLog")("ERROR", msg) + stop(msg) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 608 - 640, After the main `BiocParallel::bplapply` call that assigns `results`, inspect the returned slots with `BiocParallel::bpok()` before naming or passing them onward. Detect any failed elements and raise an error (including the failed slot details) so `MSstatsSummarizeWithMultipleCores` cannot return a partially successful results list; leave successful results unchanged.
386-397: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failed summarization results before indexing
result[[idx]].
MSstatsSummarizeSingleTMPreturnslist(NULL, NULL)andMSstatsSummarizeSingleLinearcan returnlist(NULL, survival), but the worker unconditionally indexesresult[[1]]andresult[[2]]. Add a length/null guard before the level-normalization loop so unsummarizable proteins pass through without a subscript error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 386 - 397, Add a guard in the result-normalization flow before the loop over result indices to return unsummarizable outputs safely when the summarization result is missing or too short. Ensure both list(NULL, NULL) and list(NULL, survival) outcomes avoid invalid result[[idx]] access, while valid results continue through RUN, FEATURE, and cen normalization unchanged.
🧹 Nitpick comments (1)
R/MSstatsSummarizeWithMultipleCores.R (1)
642-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe memory report contains a single checkpoint.
memory_checkpointsreceives only"parent peak (main)", so the "Delta" column that.print_memory_reportrenders is always empty..current_rss_mbis defined at line 5 but never called. Record checkpoints after packing and after dispatch with.current_rss_mb(), or simplify the report.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/MSstatsSummarizeWithMultipleCores.R` around lines 642 - 655, Update the memory-tracking flow around .print_memory_report to record multiple checkpoints: call .current_rss_mb() after packing and again after dispatch, storing both values in memory_checkpoints alongside the existing parent peak checkpoint. Preserve the worker peak collection and report generation while ensuring the resulting report has meaningful Delta values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@man/dot-build_summarize_worker.Rd`:
- Around line 8-16: Add roxygen2 `@param` entries for use_TMP, impute,
censored_symbol, remove50missing, aft_iterations, and equal_variance in
.build_summarize_worker within MSstatsSummarizeWithMultipleCores.R, then
regenerate the .build_summarize_worker Rd documentation so its \arguments
section matches the six parameters declared by \usage.
In `@R/MSstatsSummarizeWithMultipleCores.R`:
- Around line 20-65: Update .peak_rss_mb to store compiled helper functions in a
package-local environment instead of .GlobalEnv, and wrap both Rcpp::cppFunction
calls and helper invocation in tryCatch. Return NA_real_ whenever compilation or
measurement fails, preserving the existing platform-specific memory paths so
.print_memory_report can display “n/a” without aborting summarization.
- Around line 608-640: After the main `BiocParallel::bplapply` call that assigns
`results`, inspect the returned slots with `BiocParallel::bpok()` before naming
or passing them onward. Detect any failed elements and raise an error (including
the failed slot details) so `MSstatsSummarizeWithMultipleCores` cannot return a
partially successful results list; leave successful results unchanged.
- Around line 386-397: Add a guard in the result-normalization flow before the
loop over result indices to return unsummarizable outputs safely when the
summarization result is missing or too short. Ensure both list(NULL, NULL) and
list(NULL, survival) outcomes avoid invalid result[[idx]] access, while valid
results continue through RUN, FEATURE, and cen normalization unchanged.
---
Nitpick comments:
In `@R/MSstatsSummarizeWithMultipleCores.R`:
- Around line 642-655: Update the memory-tracking flow around
.print_memory_report to record multiple checkpoints: call .current_rss_mb()
after packing and again after dispatch, storing both values in
memory_checkpoints alongside the existing parent peak checkpoint. Preserve the
worker peak collection and report generation while ensuring the resulting report
has meaningful Delta values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 331dce4b-6553-4982-9276-2d3a501e71ff
📒 Files selected for processing (5)
R/MSstatsSummarizeWithMultipleCores.Rman/MSstatsSummarizeWithMultipleCores.Rdman/dot-build_summarize_worker.Rdman/dot-pack_protein_slot.Rdman/dot-unpack_protein_slot.Rd
🚧 Files skipped from review as they are similar to previous changes (1)
- man/MSstatsSummarizeWithMultipleCores.Rd
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesParallel summarization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MSstatsSummarizeWithMultipleCores
participant BiocParallelWorkers
participant MSstatsSummarization
Caller->>MSstatsSummarizeWithMultipleCores: submit input and parallel options
MSstatsSummarizeWithMultipleCores->>BiocParallelWorkers: dispatch packed protein records
BiocParallelWorkers->>MSstatsSummarization: reconstruct records and summarize
MSstatsSummarization-->>BiocParallelWorkers: return per-protein result
BiocParallelWorkers-->>MSstatsSummarizeWithMultipleCores: return named results and memory data
MSstatsSummarizeWithMultipleCores-->>Caller: return per-protein-slot list
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/benchmark_summarize_perf_selevsek.R`:
- Around line 14-24: Release result_1core after the single-core timing completes
and before invoking MSstatsSummarizeWithMultipleCores for time_4core, using the
script’s existing cleanup or garbage-collection mechanism if available. Preserve
the single-core measurement while ensuring result_1core is no longer reachable
when the parallel run starts.
In `@benchmark/config.slurm`:
- Line 30: Update the benchmark setup in config.slurm to install or load the PR
branch’s checked-out Vitek-Lab/MSstats source instead of the devel branch,
ensuring R/MSstatsSummarizeWithMultipleCores.R is available before the scripts
in R_SCRIPTS run.
In `@R/utils_censored.R`:
- Around line 87-88: Update the `.setCensoredByThreshold` documentation for
`remove50missing` to match the helper’s actual behavior: do not claim it removes
features when the helper does not read or apply that option, and instead state
that filtering occurs before this call if that is the intended contract.
In `@R/utils_summarization.R`:
- Around line 3-4: Update the remove50missing parameter documentation in the
summarization function to say proteins with at least 50% missing values are
excluded, matching the inclusive input$prop_features <= 0.5 condition.
Regenerate man/dot-isSummarizable.Rd so the generated documentation reflects
this wording.
In `@src/peak_rss.cpp`:
- Around line 20-21: Update peak_rss_mb() to initialize the rusage struct before
calling getrusage, check the call’s return value, and return NA_REAL immediately
when it fails; only read ru.ru_maxrss after a successful call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b7a378a-2f67-489b-9834-16d567fd5568
📒 Files selected for processing (19)
R/MSstatsSummarizeWithMultipleCores.RR/RcppExports.RR/utils_censored.RR/utils_summarization.Rbenchmark/benchmark_summarize_perf_selevsek.Rbenchmark/config.slurmman/MSstatsSummarizeSingleTMP.Rdman/MSstatsSummarizeWithMultipleCores.Rdman/MSstatsSummarizeWithSingleCore.Rdman/dot-build_summarize_worker.Rdman/dot-getNonMissingFilterStats.Rdman/dot-isSummarizable.Rdman/dot-pack_protein_slot.Rdman/dot-runTukey.Rdman/dot-setCensoredByThreshold.Rdman/reexports.Rdsrc/Makevars.winsrc/RcppExports.cppsrc/peak_rss.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- man/dot-pack_protein_slot.Rd
- man/dot-build_summarize_worker.Rd
- R/MSstatsSummarizeWithMultipleCores.R
tonywu1999
changed the title
| # - 4 cores should reduce wall time by at least 25% vs. 1 core | ||
| # - peak RSS per worker should stay under ~1GB (see the memory report below) | ||
|
|
||
| input <- data.table::fread("/projects/VitekLab/Data/MS/selevsek/before_summarization.csv") |
There was a problem hiding this comment.
a bit of docs would help. Goal + output at least
| @@ -0,0 +1,401 @@ | |||
| .peak_rss_mb <- function() { | |||
| if (.Platform$OS.type != "windows" && file.exists("/proc/self/status")) { | |||
| @@ -0,0 +1,401 @@ | |||
| .peak_rss_mb <- function() { | |||
| if (.Platform$OS.type != "windows" && file.exists("/proc/self/status")) { | |||
| ln <- grep("^VmHWM:", readLines("/proc/self/status"), value = TRUE) | |||
There was a problem hiding this comment.
what's this regular expression?
| previous_value <- checkpoint_value | ||
| } | ||
| if (!is.null(worker_peak_mb)) { | ||
| observed_peaks <- worker_peak_mb[!is.na(worker_peak_mb)] |
There was a problem hiding this comment.
"observed_peaks" is a bit too general in the MS context (-;
meaning unclear (-;
| #' @return data.table compatible with \code{MSstatsSummarizeSingleTMP} / | ||
| #' \code{MSstatsSummarizeSingleLinear} | ||
| #' @keywords internal | ||
| .unpack_protein_slot <- function(packed, meta) { |
There was a problem hiding this comment.
is relying on integer indices here safe?
|
|
||
| payload_mb <- sum(vapply(protein_records, | ||
| function(r) length(r$packed), integer(1))) * 8 / 1024^2 | ||
| getOption("MSstatsLog")("INFO", |
There was a problem hiding this comment.
is this still valid after log4r issues with CRAN?
Btw this package may come back soon, there was a github release a couple day ago I think
| protein_dt, impute_, censored_symbol_, | ||
| remove50missing_, aft_iterations_) | ||
| } else { | ||
| MSstatsSummarizeSingleLinear( |
There was a problem hiding this comment.
do we still need the linear summarization option?
@tonywu1999 @devonjkohler
2 participants
Motivation and solution
Parallel summarization could cause unstable worker memory usage because workers received large data objects. This PR introduces a packed-data workflow that limits task payloads and stabilizes RAM usage. Workers reconstruct per-protein records and perform TMP or linear summarization.
Changes
MSstatsSummarizeWithMultipleCores().BPPARAM,track_memory, andmax_proteins_per_workerarguments.matter(for data transfer serialization and garbage collection), andBiocParallel(for bplapply in-memory parallelization management) imports.Coding guidelines
Tests
Out of Scope