[r] Improve error printing in call_peaks_macs (#175)

r/NEWS.md

@@ -6,6 +6,11 @@
 
 Contributions welcome :)
 
+# BPCells 0.3.1 (in-progress main branch)
+
+## Bug-fixes
+- Fix error message printing when MACS crashes during `call_peaks_macs()` (pull request #175) 
+
 # BPCells 0.3.0 (12/21/2024)
 
 The BPCells 0.3.0 release covers 6 months of changes and 45 commits from 5 contributors. Notable improvements

r/R/atac_utils.R

@@ -576,21 +576,27 @@ call_peaks_macs <- function(fragments, path,
     if (length(file_names) != length(levels(cell_groups))) {
       warning("Number of shell files does not match number of clusters")
     }
-    parallel::mclapply(file_names, function(shell_file) {
+    macs_success <- parallel::mclapply(file_names, function(shell_file) {
       cluster <- gsub(".sh$", "", shell_file)
       if (verbose) log_progress(paste0("Running MACS for cluster: ", cluster))
       dir.create(file.path(path, "output", cluster), showWarnings = FALSE, recursive = TRUE)
       log_file <- paste0(path, "/output/", cluster, "/log.txt")
       macs_message <- system2("bash", sprintf("'%s'", file.path(path, "input", shell_file)),stdout = log_file, stderr = log_file, env = c("OMP_NUM_THREADS=1"))
       # Try detecting if macs failed before writing that cluster is finished
       if (macs_message != 0) {
-        stop(paste0(format(Sys.time(), "%Y-%m-%d %H:%M:%S"), " Error running MACS for cluster: ", cluster, "\n",
+        log_progress(paste0(" Error running MACS for cluster: ", cluster, "\n",
                     "MACS log file written to: ", log_file))
+        return(FALSE)
       } else if (verbose) {
         log_progress(paste0(" Finished running MACS for cluster: ", cluster))
         log_progress(paste0(" MACS log file written to: ", log_file))
       }
+      return(TRUE)
     }, mc.cores = threads, mc.preschedule = FALSE)
+    failures <- sum(!unlist(macs_success))
+    if (failures > 0) {
+      rlang::abort(c(sprintf("MACS calls encountered %d failures", failures), "See error logs listed above"))
+    }
   }
   # Read outputs
   if (step %in%  c("read-outputs", "all")) {

r/tests/testthat/test-atac_utils.R

@@ -333,4 +333,43 @@ test_that("macs_e2e_works", {
   )
   # Make sure the outputs are the same
   expect_equal(macs_read, macs_read_full_pipeline)
+})
+
+test_that("macs errors print when running in parallel", {
+  # The dummy macs script only works on a unix setup, and windows currently doesn't support
+  # multi-threading anyhow
+  skip_on_os("windows")
+  dir <- withr::local_tempdir()
+
+  # Make a dummy macs script that will register as valid but won't actually run
+  bad_macs_path <- file.path(dir, "bad_macs.sh")
+  writeLines(c(
+    "#!/bin/bash",
+    "if [[ $1 == '--version' ]]; then",
+    "    echo 'Bad macs demo'",
+    "else",
+    "    exit 1",
+    "fi"
+  ), bad_macs_path)
+  Sys.chmod(bad_macs_path, "0700")
+
+  frags <- tibble::tibble(chr="chr1", start=1:10, end=start+5, cell_id=rep(c("a","b"), 5)) %>% convert_to_fragments() 
+  call_peaks_macs(
+    frags, 
+    path=file.path(dir, "macs-test"), 
+    cell_groups=c(a="a_group", b="b_group"), 
+    step="prep-inputs",
+    macs_executable=bad_macs_path, 
+    threads=2
+  )
+  expect_error({
+    call_peaks_macs(
+      frags,
+      path=file.path(dir, "macs-test"), 
+      cell_groups=c(a="a_group", b="b_group"), 
+      step="run-macs",
+      macs_executable=bad_macs_path, 
+      threads=2
+    )
+  }, "MACS calls encountered .* failures")
 })