From 0e50ffee5d1dc5e652ca88de0356a42ba5428ed5 Mon Sep 17 00:00:00 2001 From: seanthimons Date: Fri, 23 Oct 2026 10:03:35 -0400 Subject: [PATCH] fix(bootstrap): cap how long one replicate may run A perfectly separated resample can send the exact beta-Poisson optimiser searching for minutes, which held up the whole bootstrap indefinitely. Add replicate_time_limit (default 30 seconds, Inf for none) to the bootstrap and analysis functions. A replicate that overruns is recorded as not converged with the reason in its error column. The limit is passed to mirai workers with each batch. --- R/analysis.R | 9 +- R/bootstrap.R | 116 +++++++++++----- man/analyze_dose_response.Rd | 6 +- man/bootstrap_dose_response.Rd | 13 +- man/bootstrap_dose_response_async.Rd | 13 +- man/bootstrap_dose_response_models.Rd | 13 +- tests/testthat/test-bootstrap-time-limit.R | 154 +++++++++++++++++++++ 7 files changed, 287 insertions(+), 37 deletions(-) create mode 100644 tests/testthat/test-bootstrap-time-limit.R diff --git a/R/analysis.R b/R/analysis.R index e43ed6c..54d2beb 100644 --- a/R/analysis.R +++ b/R/analysis.R @@ -23,6 +23,8 @@ #' [bootstrap_dose_response()]. #' @param check_trend Warn when an increasing trend is not detected. #' @param alpha Significance level for trend and goodness-of-fit tests. +#' @param replicate_time_limit Seconds one bootstrap replicate may run before it +#' is recorded as not converged, passed to [bootstrap_dose_response()]. #' #' @return A `qdr_analysis` object containing standardized data, trend test, #' model fits, diagnostics, parsed assessment, comparison, and bootstrap @@ -39,13 +41,15 @@ analyze_dose_response <- function( compute = NULL, workers = NULL, check_trend = TRUE, - alpha = 0.05 + alpha = 0.05, + replicate_time_limit = 30 ) { data <- as_dose_response(data) resample <- match.arg(resample) backend <- match.arg(backend) validate_bootstrap_times(bootstrap_times, "bootstrap_times") validate_bootstrap_times(exact_bootstrap_times, "exact_bootstrap_times") + validate_replicate_time_limit(replicate_time_limit) fits <- fit_dose_response_models(data, models = models, check_trend = check_trend, alpha = alpha) # Bootstrap each fitted model with its own replicate count: the exact model is @@ -64,7 +68,8 @@ analyze_dose_response <- function( seed = model_seed, backend = backend, compute = compute, - workers = workers + workers = workers, + replicate_time_limit = replicate_time_limit ) }) bootstraps <- purrr::compact(bootstraps) diff --git a/R/bootstrap.R b/R/bootstrap.R index ff95179..c9238a1 100644 --- a/R/bootstrap.R +++ b/R/bootstrap.R @@ -20,6 +20,15 @@ #' @param workers Number of temporary local mirai daemons. `NULL` uses 75% of #' detected physical cores, with a minimum of one. Ignored when using existing #' daemons or the sequential backend. +#' @param replicate_time_limit Seconds one replicate's refit may run before it is +#' abandoned and recorded as not converged, with the reason in its `error` +#' column. The default is 30; use `Inf` for no limit. Some resamples, such as +#' one where every dose above a point responds and every dose below does not, +#' can send the exact beta-Poisson optimiser searching for minutes; the limit +#' keeps one such replicate from holding up the whole run. The limit is wall +#' time, so on a heavily loaded machine a slow replicate can time out that would +#' finish elsewhere. A finite limit also replaces any [setTimeLimit()] already +#' set in the process that runs the refit. #' #' @return A `qdr_bootstrap` tibble with one row per replicate. #' @export @@ -30,12 +39,13 @@ bootstrap_dose_response <- function( seed = NULL, backend = c("auto", "sequential", "mirai"), compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) { - inputs <- prepare_bootstrap(object, times, resample, seed, backend, compute, workers) + inputs <- prepare_bootstrap(object, times, resample, seed, backend, compute, workers, replicate_time_limit) backend <- inputs$backend if (backend == "sequential") { - estimates <- run_sequential_bootstrap(inputs$tasks, object) + estimates <- run_sequential_bootstrap(inputs$tasks, object, inputs$time_limit) return(new_qdr_bootstrap(estimates, object, inputs$resample, backend)) } collect_bootstrap(start_mirai_bootstrap(inputs, object)) @@ -56,9 +66,10 @@ bootstrap_dose_response_async <- function( resample = c("observed", "fitted"), seed = NULL, compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) { - inputs <- prepare_bootstrap(object, times, resample, seed, "mirai", compute, workers) + inputs <- prepare_bootstrap(object, times, resample, seed, "mirai", compute, workers, replicate_time_limit) start_mirai_bootstrap(inputs, object) } @@ -107,7 +118,22 @@ bootstrap_progress <- function(job) { list(done = as.integer(done), total = as.integer(total), finished = done == total) } -prepare_bootstrap <- function(object, times, resample, seed, backend, compute, workers) { +validate_replicate_time_limit <- function(limit) { + if (length(limit) != 1L || !is.numeric(limit) || is.na(limit) || limit <= 0) { + stop("`replicate_time_limit` must be a positive number of seconds, or Inf for no limit.", call. = FALSE) + } +} + +prepare_bootstrap <- function( + object, + times, + resample, + seed, + backend, + compute, + workers, + replicate_time_limit = 30 +) { check_qdr_fit(object) resample <- match.arg(resample, c("observed", "fitted")) backend <- match.arg(backend, c("auto", "sequential", "mirai")) @@ -124,6 +150,7 @@ prepare_bootstrap <- function(object, times, resample, seed, backend, compute, w ) { stop("`workers` must be NULL or a positive whole number.", call. = FALSE) } + validate_replicate_time_limit(replicate_time_limit) times <- as.integer(times) backend <- select_bootstrap_backend(backend, times) if (requested_backend == "auto" && backend == "sequential" && times > 1000L) { @@ -135,7 +162,8 @@ prepare_bootstrap <- function(object, times, resample, seed, backend, compute, w resample = resample, backend = backend, compute = compute, - workers = if (is.null(workers)) NULL else as.integer(workers) + workers = if (is.null(workers)) NULL else as.integer(workers), + time_limit = as.numeric(replicate_time_limit) ) } @@ -194,9 +222,9 @@ generate_bootstrap_tasks <- function(object, probability, times, seed) { if (is.null(seed)) generate() else withr::with_seed(seed, generate()) } -run_sequential_bootstrap <- function(tasks, object) { +run_sequential_bootstrap <- function(tasks, object, time_limit = Inf) { purrr::map_dfr(tasks, function(task) { - bootstrap_refit(object, task$sample_data, task$replicate) + bootstrap_refit(object, task$sample_data, task$replicate, time_limit) }) } @@ -224,7 +252,7 @@ start_mirai_bootstrap <- function(inputs, object) { mirai::mirai_map( batches, bootstrap_worker(), - .args = list(original_fit = object, dev_path = dev_path), + .args = list(original_fit = object, dev_path = dev_path, time_limit = inputs$time_limit), .compute = compute ), error = function(cnd) { @@ -252,12 +280,12 @@ start_mirai_bootstrap <- function(inputs, object) { # free variables from that frame. Source references are removed because under # pkgload::load_all() they would carry the whole source file with each batch. bootstrap_worker <- function() { - worker <- function(batch, original_fit, dev_path) { + worker <- function(batch, original_fit, dev_path, time_limit = Inf) { if (!is.null(dev_path) && !pkgload::is_dev_package("singlehit")) { pkgload::load_all(dev_path, quiet = TRUE) } dplyr::bind_rows(lapply(batch, function(task) { - singlehit:::bootstrap_refit(original_fit, task$sample_data, task$replicate) + singlehit:::bootstrap_refit(original_fit, task$sample_data, task$replicate, time_limit) })) } worker <- utils::removeSource(worker) @@ -290,10 +318,46 @@ refit_sample <- function(original_fit, sample_data) { fit_core(data, original_fit$model, start = original_fit$coefficients) } -bootstrap_refit <- function(original_fit, sample_data, replicate) { +# Refit one replicate. A refit that runs past `time_limit` seconds is abandoned +# and recorded as not converged. The limit is enforced with setTimeLimit(), which +# interrupts R-level code and fires once. fit_core() wraps the optimiser in +# tryCatch(), which swallows the interruption and then stops with its own "could +# not be fit" error, so the failure row names the limit whenever the time is up, +# and a replicate that returns after the limit is treated as timed out too. +bootstrap_refit <- function(original_fit, sample_data, replicate, time_limit = Inf) { + started <- proc.time()[["elapsed"]] + timed_out <- function() is.finite(time_limit) && proc.time()[["elapsed"]] - started >= time_limit + failed_row <- function(message) { + parameters <- stats::setNames( + as.list(rep(NA_real_, length(model_parameters(original_fit$model)))), + model_parameters(original_fit$model) + ) + values <- c( + list( + model = original_fit$model, + replicate = replicate, + converged = FALSE, + log_lik = NA_real_, + deviance = NA_real_ + ), + parameters, + list(ed10 = NA_real_, ed50 = NA_real_, error = message) + ) + tibble::as_tibble_row(values) + } + limit_message <- sprintf("The refit took longer than the %s-second replicate time limit.", format(time_limit)) + if (is.finite(time_limit)) { + setTimeLimit(elapsed = time_limit, transient = TRUE) + on.exit(setTimeLimit(cpu = Inf, elapsed = Inf, transient = FALSE), add = TRUE) + } tryCatch( { fit <- suppressWarnings(refit_sample(original_fit, sample_data)) + # The limit only covers the refit; disarm it before the work that follows. + setTimeLimit(cpu = Inf, elapsed = Inf, transient = FALSE) + if (timed_out()) { + return(failed_row(limit_message)) + } values <- c( list( model = original_fit$model, @@ -312,22 +376,10 @@ bootstrap_refit <- function(original_fit, sample_data, replicate) { tibble::as_tibble_row(values) }, error = function(cnd) { - parameters <- stats::setNames( - as.list(rep(NA_real_, length(model_parameters(original_fit$model)))), - model_parameters(original_fit$model) - ) - values <- c( - list( - model = original_fit$model, - replicate = replicate, - converged = FALSE, - log_lik = NA_real_, - deviance = NA_real_ - ), - parameters, - list(ed10 = NA_real_, ed50 = NA_real_, error = conditionMessage(cnd)) - ) - tibble::as_tibble_row(values) + # Disarm first: a limit firing inside this handler would escape it. + setTimeLimit(cpu = Inf, elapsed = Inf, transient = FALSE) + message <- if (timed_out()) limit_message else conditionMessage(cnd) + failed_row(message) } ) } @@ -346,7 +398,8 @@ bootstrap_dose_response_models <- function( seed = NULL, backend = c("auto", "sequential", "mirai"), compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) { fits <- as_fit_list(object) resample <- match.arg(resample) @@ -360,7 +413,8 @@ bootstrap_dose_response_models <- function( seed = model_seed, backend = backend, compute = compute, - workers = workers + workers = workers, + replicate_time_limit = replicate_time_limit ) }) } diff --git a/man/analyze_dose_response.Rd b/man/analyze_dose_response.Rd index 9390703..f6e331f 100644 --- a/man/analyze_dose_response.Rd +++ b/man/analyze_dose_response.Rd @@ -15,7 +15,8 @@ analyze_dose_response( compute = NULL, workers = NULL, check_trend = TRUE, - alpha = 0.05 + alpha = 0.05, + replicate_time_limit = 30 ) } \arguments{ @@ -49,6 +50,9 @@ compare the exact model without bootstrapping it.} \item{check_trend}{Warn when an increasing trend is not detected.} \item{alpha}{Significance level for trend and goodness-of-fit tests.} + +\item{replicate_time_limit}{Seconds one bootstrap replicate may run before it +is recorded as not converged, passed to \code{\link[=bootstrap_dose_response]{bootstrap_dose_response()}}.} } \value{ A \code{qdr_analysis} object containing standardized data, trend test, diff --git a/man/bootstrap_dose_response.Rd b/man/bootstrap_dose_response.Rd index d515b56..98656c9 100644 --- a/man/bootstrap_dose_response.Rd +++ b/man/bootstrap_dose_response.Rd @@ -11,7 +11,8 @@ bootstrap_dose_response( seed = NULL, backend = c("auto", "sequential", "mirai"), compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) } \arguments{ @@ -35,6 +36,16 @@ backend.} \item{workers}{Number of temporary local mirai daemons. \code{NULL} uses 75\% of detected physical cores, with a minimum of one. Ignored when using existing daemons or the sequential backend.} + +\item{replicate_time_limit}{Seconds one replicate's refit may run before it is +abandoned and recorded as not converged, with the reason in its \code{error} +column. The default is 30; use \code{Inf} for no limit. Some resamples, such as +one where every dose above a point responds and every dose below does not, +can send the exact beta-Poisson optimiser searching for minutes; the limit +keeps one such replicate from holding up the whole run. The limit is wall +time, so on a heavily loaded machine a slow replicate can time out that would +finish elsewhere. A finite limit also replaces any \code{\link[=setTimeLimit]{setTimeLimit()}} already +set in the process that runs the refit.} } \value{ A \code{qdr_bootstrap} tibble with one row per replicate. diff --git a/man/bootstrap_dose_response_async.Rd b/man/bootstrap_dose_response_async.Rd index 0d3ffd9..be6753f 100644 --- a/man/bootstrap_dose_response_async.Rd +++ b/man/bootstrap_dose_response_async.Rd @@ -10,7 +10,8 @@ bootstrap_dose_response_async( resample = c("observed", "fitted"), seed = NULL, compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) } \arguments{ @@ -28,6 +29,16 @@ backend.} \item{workers}{Number of temporary local mirai daemons. \code{NULL} uses 75\% of detected physical cores, with a minimum of one. Ignored when using existing daemons or the sequential backend.} + +\item{replicate_time_limit}{Seconds one replicate's refit may run before it is +abandoned and recorded as not converged, with the reason in its \code{error} +column. The default is 30; use \code{Inf} for no limit. Some resamples, such as +one where every dose above a point responds and every dose below does not, +can send the exact beta-Poisson optimiser searching for minutes; the limit +keeps one such replicate from holding up the whole run. The limit is wall +time, so on a heavily loaded machine a slow replicate can time out that would +finish elsewhere. A finite limit also replaces any \code{\link[=setTimeLimit]{setTimeLimit()}} already +set in the process that runs the refit.} } \value{ A \code{qdr_bootstrap_job} for \code{\link[=collect_bootstrap]{collect_bootstrap()}}. diff --git a/man/bootstrap_dose_response_models.Rd b/man/bootstrap_dose_response_models.Rd index aa06dfd..5f53951 100644 --- a/man/bootstrap_dose_response_models.Rd +++ b/man/bootstrap_dose_response_models.Rd @@ -11,7 +11,8 @@ bootstrap_dose_response_models( seed = NULL, backend = c("auto", "sequential", "mirai"), compute = NULL, - workers = NULL + workers = NULL, + replicate_time_limit = 30 ) } \arguments{ @@ -35,6 +36,16 @@ backend.} \item{workers}{Number of temporary local mirai daemons. \code{NULL} uses 75\% of detected physical cores, with a minimum of one. Ignored when using existing daemons or the sequential backend.} + +\item{replicate_time_limit}{Seconds one replicate's refit may run before it is +abandoned and recorded as not converged, with the reason in its \code{error} +column. The default is 30; use \code{Inf} for no limit. Some resamples, such as +one where every dose above a point responds and every dose below does not, +can send the exact beta-Poisson optimiser searching for minutes; the limit +keeps one such replicate from holding up the whole run. The limit is wall +time, so on a heavily loaded machine a slow replicate can time out that would +finish elsewhere. A finite limit also replaces any \code{\link[=setTimeLimit]{setTimeLimit()}} already +set in the process that runs the refit.} } \value{ A named list of \code{qdr_bootstrap} objects. diff --git a/tests/testthat/test-bootstrap-time-limit.R b/tests/testthat/test-bootstrap-time-limit.R new file mode 100644 index 0000000..7f55685 --- /dev/null +++ b/tests/testthat/test-bootstrap-time-limit.R @@ -0,0 +1,154 @@ +slow_task <- function() { + fit <- fit_dose_response(ward_fixture(), "beta_poisson") + list(fit = fit, task = generate_bootstrap_tasks(fit, fit$data$response, 1L, 7L)[[1]]) +} + +test_that("a refit that runs past the limit becomes a non-converged row with the reason", { + s <- slow_task() + local_mocked_bindings(refit_sample = function(...) repeat Sys.sleep(0.02)) + + started <- proc.time()[["elapsed"]] + row <- bootstrap_refit(s$fit, s$task$sample_data, 5L, time_limit = 0.3) + + expect_lt(proc.time()[["elapsed"]] - started, 5) + expect_false(row$converged) + expect_equal(row$replicate, 5L) + expect_match(row$error, "0.3-second replicate time limit", fixed = TRUE) + expect_true(is.na(row$ed50)) +}) + +test_that("a refit that swallows the interruption and finishes late still counts as timed out", { + s <- slow_task() + real <- refit_sample(s$fit, s$task$sample_data) + # The exact model's optimiser tries several starting values inside tryCatch(), + # which can absorb the first interruption and carry on. + local_mocked_bindings(refit_sample = function(...) { + tryCatch(repeat Sys.sleep(0.02), error = function(e) NULL) + real + }) + + row <- bootstrap_refit(s$fit, s$task$sample_data, 1L, time_limit = 0.3) + + expect_false(row$converged) + expect_match(row$error, "replicate time limit") +}) + +test_that("a fast refit under a finite limit matches the unlimited result and clears the limit", { + s <- slow_task() + unlimited <- bootstrap_refit(s$fit, s$task$sample_data, 1L) + limited <- bootstrap_refit(s$fit, s$task$sample_data, 1L, time_limit = 30) + expect_equal(limited, unlimited) + expect_true(limited$converged) +}) + +test_that("the limit does not leak into later work in the same process", { + s <- slow_task() + real <- refit_sample(s$fit, s$task$sample_data) + # An instant refit, so the limit is still armed when bootstrap_refit() returns. + local_mocked_bindings(refit_sample = function(...) real) + bootstrap_refit(s$fit, s$task$sample_data, 1L, time_limit = 0.2) + for (i in 1:12) Sys.sleep(0.05) + expect_true(TRUE) +}) + +test_that("the limit cannot fire while a failure row is being built", { + s <- slow_task() + real_parameters <- model_parameters + local_mocked_bindings( + refit_sample = function(...) stop("optimiser failed"), + # Slow work inside the error handler, past the limit. + model_parameters = function(model) { + for (i in 1:10) Sys.sleep(0.03) + real_parameters(model) + } + ) + row <- bootstrap_refit(s$fit, s$task$sample_data, 1L, time_limit = 0.1) + expect_equal(row$error, "optimiser failed") + expect_false(row$converged) +}) + +test_that("the limit covers the refit only, not the work after it", { + s <- slow_task() + real <- refit_sample(s$fit, s$task$sample_data) + real_effective_dose <- effective_dose + local_mocked_bindings( + refit_sample = function(...) real, + effective_dose = function(...) { + for (i in 1:10) Sys.sleep(0.03) + real_effective_dose(...) + } + ) + row <- bootstrap_refit(s$fit, s$task$sample_data, 1L, time_limit = 0.1) + expect_true(row$converged) + expect_true(is.na(row$error)) +}) + +test_that("the sequential path and the mirai worker apply the limit to every replicate", { + s <- slow_task() + tasks <- generate_bootstrap_tasks(s$fit, s$fit$data$response, 2L, 7L) + local_mocked_bindings(refit_sample = function(...) repeat Sys.sleep(0.02)) + + sequential <- run_sequential_bootstrap(tasks, s$fit, time_limit = 0.2) + batched <- bootstrap_worker()(tasks, s$fit, NULL, 0.2) + + expect_false(any(sequential$converged)) + expect_match(sequential$error, "time limit") + expect_equal(tibble::as_tibble(batched), tibble::as_tibble(sequential)) +}) + +test_that("replicate_time_limit is validated, and Inf means no limit", { + fit <- fit_dose_response(ward_fixture(), "beta_poisson") + for (bad in list(0, -1, NA_real_, "30", c(10, 20), NULL)) { + expect_error( + prepare_bootstrap(fit, 3L, "observed", 1L, "sequential", NULL, NULL, bad), + "replicate_time_limit" + ) + } + # With no bootstraps requested, only the early check in analyze_dose_response() + # can reject the value. + expect_error( + analyze_dose_response(ward_fixture(), bootstrap_times = 0L, exact_bootstrap_times = 0L, replicate_time_limit = 0), + "replicate_time_limit" + ) + expect_equal(prepare_bootstrap(fit, 3L, "observed", 1L, "sequential", NULL, NULL)$time_limit, 30) + expect_equal(prepare_bootstrap(fit, 3L, "observed", 1L, "sequential", NULL, NULL, Inf)$time_limit, Inf) +}) + +test_that("the limit is sent to the mirai daemons with each batch", { + skip_if_not_installed("mirai") + fit <- fit_dose_response(ward_fixture(), "beta_poisson") + inputs <- prepare_bootstrap(fit, 3L, "observed", 42L, "mirai", NULL, NULL, 12) + captured <- NULL + local_mocked_bindings( + daemons_set = function(...) TRUE, + mirai_map = function(.x, .f, .args, ...) { + captured <<- .args + list() + }, + .package = "mirai" + ) + start_mirai_bootstrap(inputs, fit) + expect_equal(captured$time_limit, 12) +}) + +test_that("the limit is passed through the public bootstrap and analysis functions", { + fit <- fit_dose_response(ward_fixture(), "beta_poisson") + seen <- c() + local_mocked_bindings( + run_sequential_bootstrap = function(tasks, object, time_limit = Inf) { + seen <<- c(seen, time_limit) + purrr::map_dfr(tasks, function(task) bootstrap_refit(object, task$sample_data, task$replicate, time_limit)) + } + ) + bootstrap_dose_response(fit, times = 2L, backend = "sequential", replicate_time_limit = 11) + bootstrap_dose_response_models(list(beta_poisson = fit), times = 2L, backend = "sequential", replicate_time_limit = 12) + analyze_dose_response( + ward_fixture(), + models = c("exponential", "beta_poisson"), + bootstrap_times = 2L, + backend = "sequential", + replicate_time_limit = 13 + ) + expect_equal(seen[1:2], c(11, 12)) + expect_true(all(seen[3:4] == 13)) +})