Refactored multicore summarisation to distribute pre-split data

[tonywu1999]

• MSstatsSummarizeWithMultipleCores: replaced protein_indices = split(seq_len(nrow(input)), ...) (each worker re-indexed into the broadcast input) with protein_data = split(input, ...) so each worker receives only its own protein's rows. Dropped input and protein_indices from clusterExport accordingly.
• Removed the per-100-proteins progress log inside the worker function — counting completed proteins under parLapply without coordination is unreliable; the startup "Number of proteins to process: N" cat is sufficient.
• Fixed an undefined-variable regression on the same change path: num_proteins = length(protein_indices) would have thrown object 'protein_indices' not found on the first multicore invocation. Now reads length(protein_data).
• Added inst/tinytest/test_dataProcess.R multicore-parity block asserting single-core and multi-core linear ProteinLevelData agree on row count, protein set, LogIntensities, and Variance.
• Added benchmark/profile_dataprocess_peak.R that drives dataProcess() under profmem::profmem() and reports peak RSS via ps::ps_memory_info().
• DESCRIPTION: added callr, ps, profmem to Suggests so the benchmark script is runnable with a checkout. None become hard dependencies.

See MSstats-ai/todos/active/TODO-MS-20260514_fix-memory-bugs.md

Motivation and Context

Please include relevant motivation and context of the problem along with a short summary of the solution.

Changes

Please provide a detailed bullet point list of your changes.

Testing

Please describe any unit tests you added or modified to verify your changes.

Checklist Before Requesting a Review

• I have read the MSstats contributing guidelines
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules
• I have run the devtools::document() command after my changes and committed the added files

Motivation and context

Multicore summarisation previously broadcast the full input to workers and passed row indices, causing unnecessary memory usage and introducing an undefined protein_indices regression. Worker progress logging was also unreliable.

The implementation now distributes pre-split per-protein data directly to workers, reducing the exported payload and improving multicore reliability.

Changes

• Refactored multicore summarisation to split input into per-protein datasets.
• Passed protein datasets directly to summarisation workers instead of numeric indices.
• Removed unnecessary exports of input and protein_indices.
• Removed unreliable per-100-protein worker progress logging.
• Updated protein counting to use protein_data.
• Added benchmark/profile_dataprocess_peak.R for stage-by-stage memory profiling.
• Added optional callr, ps, and profmem dependencies to Suggests.

Tests

• Added multicore parity tests for dataProcess().
• Compared single- and multi-core results for:
  • Row counts
  • Protein sets
  • LogIntensities
  • Variance, when available

Coding guideline violations

• None identified.

[coderabbitai]

📝 Walkthrough

Walkthrough

Multicore protein summarization now pre-splits data before worker execution. A linear-summary parity test was added, and a standalone benchmark replays dataProcess stages to report peak memory. Suggested packages support the profiling workflow.

Changes

Data processing and profiling

Layer / File(s)	Summary
Pre-split parallel summarization R/dataProcess.R, inst/tinytest/test_dataProcess.R	Parallel workers receive per-protein data directly with a reduced export payload; multicore linear summaries are compared against single-core results.
Peak-memory profiling workflow benchmark/profile_dataprocess_peak.R, DESCRIPTION	A configurable benchmark replays pipeline stages, records GC memory checkpoints, and adds profiling-related suggested packages.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• Memory issue when using dataProcess function with multicore #177 — The parallel summarization refactor addresses copying and exporting the full input during multicore processing.

Possibly related PRs

• Vitek-Lab/MSstats#193 — Both changes adjust protein-based splitting in MSstatsSummarizeWithMultipleCores().

Suggested labels: Review effort 2/5

Suggested reviewers: mstaniak

Poem

A bunny split proteins in a neat little row,
Sent small bits to workers, not one giant load.
A memory meter hopped stage by stage,
While tests checked the answers on every page.
“Less clutter!” cried Rabbit, and bounded away.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description has useful change details but is missing the required filled-out Motivation, Testing, and checklist sections.	Add a filled Motivation and Context section, a Testing section, and complete the checklist items; keep the changes as a concise bullet list.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: multicore summarisation now uses pre-split data.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch MSstats/work/20260514_multicore-and-benchmark

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Failed to generate code suggestions for PR

[coderabbitai]

Actionable comments posted: 4

🤖 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/profile_dataprocess_peak.R`:
- Around line 115-122: The checkpoint function currently reports only R Vcells
usage and must collect the promised broader memory metrics. Update checkpoint
and its stage instrumentation to record process RSS via ps and largest
allocations via profmem, using callr where isolation or monitoring is needed;
otherwise narrow the script’s peak-memory claims to Vcells only.

In `@inst/tinytest/test_dataProcess.R`:
- Around line 126-148: Update the comparison block after sorting linear_single
and linear_multi to assert matching as.character(RUN) values alongside Protein
before comparing LogIntensities or Variance; when LABEL exists in both
summaries, assert matching LABEL values as well. Keep the existing ordering and
numeric comparisons unchanged.

In `@R/dataProcess.R`:
- Around line 240-253: Register an on.exit cleanup immediately after creating
the cluster used by the summarized_results parallel branches, ensuring
parallel::stopCluster(cl) runs even when either parallel::parLapply call fails.
Preserve the existing successful execution and outer tryCatch behavior.
- Around line 246-252: Update both the parallel `MSstatsSummarizeSingleLinear`
call inside `parallel::parLapply` and the single-core call in the same
data-processing flow to forward the exported `equal_variance` value as the
summarizer’s `equalFeatureVar` argument. Add coverage exercising
`equalFeatureVar = FALSE` and confirming matching behavior between parallel and
single-core paths.

🪄 Autofix (Beta)

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

Run ID: 427804df-b1e5-4276-b082-a0e322d5ee83

📥 Commits

Reviewing files that changed from the base of the PR and between 68bc8a8 and 25dcfac.

📒 Files selected for processing (4)

• DESCRIPTION
• R/dataProcess.R
• benchmark/profile_dataprocess_peak.R
• inst/tinytest/test_dataProcess.R

[tonywu1999]

Tested this on Selevsek-2015-Spectronaut dataset and monitored memory usage with top

Original parallelization (both commit ID f5259ee, 3fa0bd1)

PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND                                                      
1068023 wu.anth+  20   0 5815748   4.5g  91596 S   1.0   1.8   4:03.69 rsession                                                     
1069064 wu.anth+  20   0 2737596   2.0g  25576 R 189.7   0.8   6:42.00 R                                                            
1069082 wu.anth+  20   0 2720920   2.0g  25560 R 185.4   0.8   7:36.60 R

Parallelization from this PR:

Here, the 2nd process doesn't get opened for a long time (opens 12 minutes later), and the first process
is overloaded w.r.t. RAM (I requested only 8GB of RAM)

1175020 wu.anth+  20   0   15.4g  14.8g  25612 S   0.0   5.9  20:05.46 R                                                            
1175038 wu.anth+  20   0   15.1g  14.5g  25492 R 358.6   5.8  11:42.64 R                                                            
1123263 wu.anth+  20   0 5043492   3.9g  92300 S   0.0   1.5  22:49.67 rsession 

While the approach seems to ensure the input object isn't copied across workers, the practical implications suggest protein_data = split(input, list(input$PROTEIN, input$LABEL)) is still being passed to workers, one chunk at a time, leading to computational time and memory spikes. protein_data while not seemingly taking up more space in memory, was computationally expensive to compute.

[tonywu1999]

My recommendation is to close this PR, I don't think this is the right approach for parallelization.

People have been complaining about parallelization crashing due to memory spikes. I believe these memory spikes are confounded with memory spikes from MSstatsBalancedDesign, so we need to think more about this. The parallelization may actually not be broken.