From 9a98d7e7eb569da8e576acba15cbc4bf3e5113d9 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 22 Feb 2024 17:28:06 +0000 Subject: [PATCH 01/36] update lintr, add packages and add redis script --- .lintr | 3 +-- DESCRIPTION | 14 ++++++++++---- scripts/redis | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 6 deletions(-) create mode 100755 scripts/redis diff --git a/.lintr b/.lintr index f2526d5..76ecb6b 100644 --- a/.lintr +++ b/.lintr @@ -1,7 +1,6 @@ -linters: with_defaults( +linters: linters_with_defaults( object_length_linter = NULL, object_usage_linter = NULL, - todo_comment_linter = NULL, cyclocomp_linter = NULL ) exclusions: list("tests/testthat.R", "R/cpp11.R") diff --git a/DESCRIPTION b/DESCRIPTION index c1e7bd1..e3da05e 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -13,20 +13,26 @@ Roxygen: list(markdown = TRUE, roclets = c("rd", "namespace", "porcelain::porcel URL: https://github.com/mrc-ide/orderly.runner BugReports: https://github.com/mrc-ide/orderly.runner/issues Imports: + cli, docopt, - gert (>= 2.0.1), + gert, + ids, jsonlite, orderly2, porcelain, + R6, + redux, + rrq, + withr Suggests: fs, httr, lgr, mockery, - testthat (>= 3.0.0), - withr + testthat (>= 3.0.0) Config/testthat/edition: 3 Remotes: mrc-ide/orderly2, reside-ic/porcelain, - r-lib/gert + r-lib/gert, + mrc-ide/rrq diff --git a/scripts/redis b/scripts/redis new file mode 100755 index 0000000..202021e --- /dev/null +++ b/scripts/redis @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +set -e +CONTAINER_NAME=orderly.server_redis +if [ "$1" = "start" ]; then + docker run --rm -d --name=$CONTAINER_NAME -p 127.0.0.1:6379:6379 redis +elif [ "$1" = "stop" ]; then + docker stop $CONTAINER_NAME +elif [ "$1" = "restart" ]; then + docker stop $CONTAINER_NAME || true + docker run --rm -d --name=$CONTAINER_NAME -p 127.0.0.1:6379:6379 redis +else + echo "Usage: redis " + exit 1 +fi \ No newline at end of file From c8cb377977d86658accf0b589c237cbcf2f3d370 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 22 Feb 2024 17:28:33 +0000 Subject: [PATCH 02/36] add queue R6 class --- R/queue.R | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 R/queue.R diff --git a/R/queue.R b/R/queue.R new file mode 100644 index 0000000..7c2f5bd --- /dev/null +++ b/R/queue.R @@ -0,0 +1,76 @@ +#' Object for managing running jobs on the redis queue +#' +#' @keywords internal +Queue <- R6::R6Class("Queue", #nolint + cloneable = FALSE, + public = list( + #' @field root Orderly root + root = NULL, + #' @field config Orderly config + config = NULL, + + #' @description + #' Create object, read configuration and setup redis connection. + #' + #' @param root Orderly root. + #' @param queue_id ID of an existing queue to connect to, creates a new one + #' if NULL (default NULL) + #' @param workers Number of workers to spawn (default 1) + #' @param cleanup_on_exit If TRUE workers are killed on exit (defaults + #' to TRUE if workers is > 0) + initialize = function(root, queue_id = NULL, workers = 1, + cleanup_on_exit = workers > 0) { + private$cleanup_on_exit <- cleanup_on_exit + self$root <- root + self$config <- orderly2::orderly_config(self$root) + if (!runner_has_git(self$root)) { + cli::cli_abort(paste("Not starting server as orderly", + "root is not version controlled.")) + } + + # Connecting to Redis + private$con <- redux::hiredis() + + # Create queue + private$queue_id <- queue_id %||% orderly_queue_id() + private$queue <- rrq::rrq_controller$new(private$queue_id, + private$con) + worker_config <- rrq::rrq_worker_config(heartbeat_period = 10) + private$queue$worker_config_save("localhost", worker_config) + private$start_workers(workers) + }, + + # Just until we add queue status for testing + number_of_workers = function() { + private$queue$worker_len() + }, + + finalize = function() { + if (private$cleanup_on_exit && !is.null(private$con)) { + private$queue$worker_stop(type = "kill") + private$queue$destroy(delete = TRUE) + } + } + ), + private = list( + cleanup_on_exit = NULL, + con = NULL, + queue = NULL, + queue_id = NULL, + start_workers = function(workers) { + if (workers > 0L) { + rrq::rrq_worker_spawn(private$queue, workers) + } + invisible() + } + ), +) + +runner_has_git <- function(path) { + nzchar(Sys.which("git")) && file.exists(file.path(path, ".git")) +} + +orderly_queue_id <- function() { + id <- Sys.getenv("ORDERLY_SERVER_QUEUE_ID", "") + if (nzchar(id)) id else sprintf("orderly.runner:%s", ids::random_id()) +} From 1e1a990e01143a7fac384a72e1c76b31e1c27682 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 22 Feb 2024 17:28:42 +0000 Subject: [PATCH 03/36] tests --- tests/testthat/helper-orderly-runner.R | 13 +++++- tests/testthat/test-queue.R | 56 ++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 tests/testthat/test-queue.R diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index c3da15d..8ba55ab 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -11,7 +11,6 @@ create_temporary_root <- function(...) { suppressMessages(orderly2::orderly_init(path, ...)) } - test_prepare_orderly_example <- function(examples, ...) { tmp <- tempfile() withr::defer_parent(unlink(tmp, recursive = TRUE)) @@ -42,3 +41,15 @@ helper_add_git <- function(path) { gert::git_remote_add(url, repo = path) list(path = path, user = user, branch = branch, sha = sha, url = url) } + +new_queue_quietly <- function(root, ...) { + suppressMessages(Queue$new(root, ...)) +} + +skip_if_no_redis <- function() { + available <- redux::redis_available() + if (!available) { + testthat::skip("Skipping test as redis is not available") + } + invisible(available) +} diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R new file mode 100644 index 0000000..0481c2a --- /dev/null +++ b/tests/testthat/test-queue.R @@ -0,0 +1,56 @@ +test_that("Can bring up queue", { + skip_if_no_redis() + + root <- create_temporary_root(use_file_store = TRUE) + gert::git_init(root) + q <- new_queue_quietly(root) + expect_equal(q$root, root) + expect_equal(q$identity, NULL) + expect_equal(q$number_of_workers(), 1) + expect_equal(q$config$core$use_file_store, TRUE) +}) + + +test_that("Errors if not git repo", { + root <- create_temporary_root() + expect_error(Queue$new(root), #nolint + paste("Not starting server as orderly root", + "is not version controlled.")) +}) + + +test_that("Can connect to existing queue with queue_id", { + skip_if_no_redis() + + root <- create_temporary_root() + gert::git_init(root) + queue_id <- ids::random_id() + q1 <- new_queue_quietly(root, queue_id = queue_id, workers = 2) + q2 <- new_queue_quietly(root, queue_id = queue_id, workers = 1) + expect_equal(q1$number_of_workers(), 3) + expect_equal(q2$number_of_workers(), 3) +}) + + +test_that("Uses ORDERLY_SERVER_QUEUE_ID if it exists", { + skip_if_no_redis() + + root <- create_temporary_root() + gert::git_init(root) + id <- ids::random_id() + q <- withr::with_envvar( + c(ORDERLY_SERVER_QUEUE_ID = id), + new_queue_quietly(root) + ) + expect_equal(q$.__enclos_env__$private$queue_id, id) +}) + + +test_that("Generated namespaced id if no ids exist", { + skip_if_no_redis() + + root <- create_temporary_root() + gert::git_init(root) + q <- new_queue_quietly(root) + expect_match(q$.__enclos_env__$private$queue_id, "orderly.runner") +}) From e3417aee533f61bec6b81bd4fa35940c9378c14f Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 22 Feb 2024 17:32:04 +0000 Subject: [PATCH 04/36] documentation to finalizer --- R/queue.R | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/R/queue.R b/R/queue.R index 7c2f5bd..528c2f3 100644 --- a/R/queue.R +++ b/R/queue.R @@ -10,7 +10,7 @@ Queue <- R6::R6Class("Queue", #nolint config = NULL, #' @description - #' Create object, read configuration and setup redis connection. + #' Create object, read configuration and setup Redis connection. #' #' @param root Orderly root. #' @param queue_id ID of an existing queue to connect to, creates a new one @@ -45,6 +45,8 @@ Queue <- R6::R6Class("Queue", #nolint private$queue$worker_len() }, + #' @description stop workers and destroy queue if + #' cleanup_on_exit is TRUE and Redis connection available finalize = function() { if (private$cleanup_on_exit && !is.null(private$con)) { private$queue$worker_stop(type = "kill") From 255f58e5f93cfba4e135d335f433d1978f14db1c Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 22 Feb 2024 17:32:35 +0000 Subject: [PATCH 05/36] add docs for Queue --- man/Queue.Rd | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 man/Queue.Rd diff --git a/man/Queue.Rd b/man/Queue.Rd new file mode 100644 index 0000000..00a6784 --- /dev/null +++ b/man/Queue.Rd @@ -0,0 +1,74 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/queue.R +\name{Queue} +\alias{Queue} +\title{Object for managing running jobs on the redis queue} +\description{ +Object for managing running jobs on the redis queue + +Object for managing running jobs on the redis queue +} +\keyword{internal} +\section{Public fields}{ +\if{html}{\out{
}} +\describe{ +\item{\code{root}}{Orderly root} + +\item{\code{config}}{Orderly config} +} +\if{html}{\out{
}} +} +\section{Methods}{ +\subsection{Public methods}{ +\itemize{ +\item \href{#method-Queue-new}{\code{Queue$new()}} +\item \href{#method-Queue-number_of_workers}{\code{Queue$number_of_workers()}} +\item \href{#method-Queue-finalize}{\code{Queue$finalize()}} +} +} +\if{html}{\out{
}} +\if{html}{\out{}} +\if{latex}{\out{\hypertarget{method-Queue-new}{}}} +\subsection{Method \code{new()}}{ +Create object, read configuration and setup Redis connection. +\subsection{Usage}{ +\if{html}{\out{
}}\preformatted{Queue$new(root, queue_id = NULL, workers = 1, cleanup_on_exit = workers > 0)}\if{html}{\out{
}} +} + +\subsection{Arguments}{ +\if{html}{\out{
}} +\describe{ +\item{\code{root}}{Orderly root.} + +\item{\code{queue_id}}{ID of an existing queue to connect to, creates a new one +if NULL (default NULL)} + +\item{\code{workers}}{Number of workers to spawn (default 1)} + +\item{\code{cleanup_on_exit}}{If TRUE workers are killed on exit (defaults +to TRUE if workers is > 0)} +} +\if{html}{\out{
}} +} +} +\if{html}{\out{
}} +\if{html}{\out{}} +\if{latex}{\out{\hypertarget{method-Queue-number_of_workers}{}}} +\subsection{Method \code{number_of_workers()}}{ +\subsection{Usage}{ +\if{html}{\out{
}}\preformatted{Queue$number_of_workers()}\if{html}{\out{
}} +} + +} +\if{html}{\out{
}} +\if{html}{\out{}} +\if{latex}{\out{\hypertarget{method-Queue-finalize}{}}} +\subsection{Method \code{finalize()}}{ +stop workers and destroy queue if +cleanup_on_exit is TRUE and Redis connection available +\subsection{Usage}{ +\if{html}{\out{
}}\preformatted{Queue$finalize()}\if{html}{\out{
}} +} + +} +} From fd2e375fbb343fb789cf25e8a9e667df5facee64 Mon Sep 17 00:00:00 2001 From: Mantra Date: Tue, 27 Feb 2024 17:26:02 +0000 Subject: [PATCH 06/36] refactor object so only has view on queue and does not manage workers --- R/queue.R | 46 +++++++------------------- tests/testthat/helper-orderly-runner.R | 10 ++++++ tests/testthat/test-queue.R | 25 ++++++++------ 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/R/queue.R b/R/queue.R index 528c2f3..8162844 100644 --- a/R/queue.R +++ b/R/queue.R @@ -8,6 +8,8 @@ Queue <- R6::R6Class("Queue", #nolint root = NULL, #' @field config Orderly config config = NULL, + #' @field controller RRQ controller + controller = NULL, #' @description #' Create object, read configuration and setup Redis connection. @@ -15,12 +17,7 @@ Queue <- R6::R6Class("Queue", #nolint #' @param root Orderly root. #' @param queue_id ID of an existing queue to connect to, creates a new one #' if NULL (default NULL) - #' @param workers Number of workers to spawn (default 1) - #' @param cleanup_on_exit If TRUE workers are killed on exit (defaults - #' to TRUE if workers is > 0) - initialize = function(root, queue_id = NULL, workers = 1, - cleanup_on_exit = workers > 0) { - private$cleanup_on_exit <- cleanup_on_exit + initialize = function(root, queue_id = NULL) { self$root <- root self$config <- orderly2::orderly_config(self$root) if (!runner_has_git(self$root)) { @@ -28,42 +25,23 @@ Queue <- R6::R6Class("Queue", #nolint "root is not version controlled.")) } - # Connecting to Redis - private$con <- redux::hiredis() - # Create queue - private$queue_id <- queue_id %||% orderly_queue_id() - private$queue <- rrq::rrq_controller$new(private$queue_id, - private$con) + self$controller <- rrq::rrq_controller2( + queue_id %||% orderly_queue_id() + ) worker_config <- rrq::rrq_worker_config(heartbeat_period = 10) - private$queue$worker_config_save("localhost", worker_config) - private$start_workers(workers) + rrq::rrq_worker_config_save2("localhost", worker_config, + controller = self$controller) }, # Just until we add queue status for testing number_of_workers = function() { - private$queue$worker_len() + rrq::rrq_worker_len(self$controller) }, - #' @description stop workers and destroy queue if - #' cleanup_on_exit is TRUE and Redis connection available + #' @description Destroy queue finalize = function() { - if (private$cleanup_on_exit && !is.null(private$con)) { - private$queue$worker_stop(type = "kill") - private$queue$destroy(delete = TRUE) - } - } - ), - private = list( - cleanup_on_exit = NULL, - con = NULL, - queue = NULL, - queue_id = NULL, - start_workers = function(workers) { - if (workers > 0L) { - rrq::rrq_worker_spawn(private$queue, workers) - } - invisible() + rrq::rrq_destroy(self$controller) } ), ) @@ -73,6 +51,6 @@ runner_has_git <- function(path) { } orderly_queue_id <- function() { - id <- Sys.getenv("ORDERLY_SERVER_QUEUE_ID", "") + id <- Sys.getenv("ORDERLY_RUNNER_QUEUE_ID", "") if (nzchar(id)) id else sprintf("orderly.runner:%s", ids::random_id()) } diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 8ba55ab..b33103e 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -46,6 +46,16 @@ new_queue_quietly <- function(root, ...) { suppressMessages(Queue$new(root, ...)) } +start_queue_workers_quietly <- function(n_workers, controller) { + suppressMessages( + rrq::rrq_worker_spawn2(n_workers, controller = controller) + ) +} + +kill_queue_workers <- function(controller) { + rrq::rrq_worker_stop(controller = controller) +} + skip_if_no_redis <- function() { available <- redux::redis_available() if (!available) { diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index 0481c2a..8b2440d 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -5,9 +5,13 @@ test_that("Can bring up queue", { gert::git_init(root) q <- new_queue_quietly(root) expect_equal(q$root, root) - expect_equal(q$identity, NULL) - expect_equal(q$number_of_workers(), 1) expect_equal(q$config$core$use_file_store, TRUE) + expect_equal(q$controller, rrq::rrq_controller2(q$controller$queue_id)) + + expect_equal(q$number_of_workers(), 0) + start_queue_workers_quietly(1, q$controller) + expect_equal(q$number_of_workers(), 1) + kill_queue_workers(q$controller) }) @@ -25,24 +29,25 @@ test_that("Can connect to existing queue with queue_id", { root <- create_temporary_root() gert::git_init(root) queue_id <- ids::random_id() - q1 <- new_queue_quietly(root, queue_id = queue_id, workers = 2) - q2 <- new_queue_quietly(root, queue_id = queue_id, workers = 1) - expect_equal(q1$number_of_workers(), 3) - expect_equal(q2$number_of_workers(), 3) + q1 <- new_queue_quietly(root, queue_id = queue_id) + start_queue_workers_quietly(1, q1$controller) + q2 <- new_queue_quietly(root, queue_id = queue_id) + expect_equal(q1$number_of_workers(), 1) + expect_equal(q2$number_of_workers(), 1) }) -test_that("Uses ORDERLY_SERVER_QUEUE_ID if it exists", { +test_that("Uses ORDERLY_RUNNER_QUEUE_ID if it exists", { skip_if_no_redis() root <- create_temporary_root() gert::git_init(root) id <- ids::random_id() q <- withr::with_envvar( - c(ORDERLY_SERVER_QUEUE_ID = id), + c(ORDERLY_RUNNER_QUEUE_ID = id), new_queue_quietly(root) ) - expect_equal(q$.__enclos_env__$private$queue_id, id) + expect_equal(q$controller$queue_id, id) }) @@ -52,5 +57,5 @@ test_that("Generated namespaced id if no ids exist", { root <- create_temporary_root() gert::git_init(root) q <- new_queue_quietly(root) - expect_match(q$.__enclos_env__$private$queue_id, "orderly.runner") + expect_match(q$controller$queue_id, "orderly.runner") }) From e767d0c7dd49bcfb01b11698801bd56a4cca1a71 Mon Sep 17 00:00:00 2001 From: Mantra Date: Tue, 27 Feb 2024 17:27:27 +0000 Subject: [PATCH 07/36] add redis to workflow --- .github/workflows/test-coverage.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/test-coverage.yaml b/.github/workflows/test-coverage.yaml index 2c5bb50..b6400f7 100644 --- a/.github/workflows/test-coverage.yaml +++ b/.github/workflows/test-coverage.yaml @@ -26,6 +26,10 @@ jobs: extra-packages: any::covr needs: coverage + - name: Start Redis + run: | + ./scripts/redis + - name: Test coverage run: | covr::codecov( From 0699a33102f21854bf613c191804b7aa58dcedfd Mon Sep 17 00:00:00 2001 From: Mantra Date: Wed, 28 Feb 2024 10:40:16 +0000 Subject: [PATCH 08/36] actually start redis --- .github/workflows/test-coverage.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-coverage.yaml b/.github/workflows/test-coverage.yaml index b6400f7..6c08828 100644 --- a/.github/workflows/test-coverage.yaml +++ b/.github/workflows/test-coverage.yaml @@ -28,7 +28,7 @@ jobs: - name: Start Redis run: | - ./scripts/redis + ./scripts/redis start - name: Test coverage run: | From 62a98e6f3b43b0fbf46a7f2a4d394e49bcc88138 Mon Sep 17 00:00:00 2001 From: Mantra Date: Wed, 28 Feb 2024 10:48:03 +0000 Subject: [PATCH 09/36] update docs --- man/Queue.Rd | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/man/Queue.Rd b/man/Queue.Rd index 00a6784..9a10d7f 100644 --- a/man/Queue.Rd +++ b/man/Queue.Rd @@ -15,6 +15,8 @@ Object for managing running jobs on the redis queue \item{\code{root}}{Orderly root} \item{\code{config}}{Orderly config} + +\item{\code{controller}}{RRQ controller} } \if{html}{\out{}} } @@ -32,7 +34,7 @@ Object for managing running jobs on the redis queue \subsection{Method \code{new()}}{ Create object, read configuration and setup Redis connection. \subsection{Usage}{ -\if{html}{\out{
}}\preformatted{Queue$new(root, queue_id = NULL, workers = 1, cleanup_on_exit = workers > 0)}\if{html}{\out{
}} +\if{html}{\out{
}}\preformatted{Queue$new(root, queue_id = NULL)}\if{html}{\out{
}} } \subsection{Arguments}{ @@ -42,11 +44,6 @@ Create object, read configuration and setup Redis connection. \item{\code{queue_id}}{ID of an existing queue to connect to, creates a new one if NULL (default NULL)} - -\item{\code{workers}}{Number of workers to spawn (default 1)} - -\item{\code{cleanup_on_exit}}{If TRUE workers are killed on exit (defaults -to TRUE if workers is > 0)} } \if{html}{\out{}} } @@ -64,8 +61,7 @@ to TRUE if workers is > 0)} \if{html}{\out{}} \if{latex}{\out{\hypertarget{method-Queue-finalize}{}}} \subsection{Method \code{finalize()}}{ -stop workers and destroy queue if -cleanup_on_exit is TRUE and Redis connection available +Destroy queue \subsection{Usage}{ \if{html}{\out{
}}\preformatted{Queue$finalize()}\if{html}{\out{
}} } From cb5de4ac823970541e94f35659dab58729efedc6 Mon Sep 17 00:00:00 2001 From: Mantra Date: Wed, 28 Feb 2024 12:08:30 +0000 Subject: [PATCH 10/36] fix cleanup --- R/queue.R | 2 +- tests/testthat/helper-orderly-runner.R | 8 +++----- tests/testthat/test-queue.R | 1 - 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/R/queue.R b/R/queue.R index 8162844..9e9ff4f 100644 --- a/R/queue.R +++ b/R/queue.R @@ -41,7 +41,7 @@ Queue <- R6::R6Class("Queue", #nolint #' @description Destroy queue finalize = function() { - rrq::rrq_destroy(self$controller) + rrq::rrq_destroy(controller = self$controller) } ), ) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index b33103e..8ff2f86 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -46,14 +46,12 @@ new_queue_quietly <- function(root, ...) { suppressMessages(Queue$new(root, ...)) } -start_queue_workers_quietly <- function(n_workers, controller) { +start_queue_workers_quietly <- function(n_workers, + controller, env = parent.frame()) { suppressMessages( rrq::rrq_worker_spawn2(n_workers, controller = controller) ) -} - -kill_queue_workers <- function(controller) { - rrq::rrq_worker_stop(controller = controller) + withr::defer(rrq::rrq_worker_stop(controller = controller), env = env) } skip_if_no_redis <- function() { diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index 8b2440d..ea8b8a7 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -11,7 +11,6 @@ test_that("Can bring up queue", { expect_equal(q$number_of_workers(), 0) start_queue_workers_quietly(1, q$controller) expect_equal(q$number_of_workers(), 1) - kill_queue_workers(q$controller) }) From d696102e0db84fdb632dae53ebdb7cc695db6d84 Mon Sep 17 00:00:00 2001 From: Mantra Date: Tue, 5 Mar 2024 00:18:22 +0000 Subject: [PATCH 11/36] add runner and submit function to queue --- R/queue.R | 14 ++++++ R/runner.R | 43 ++++++++++++++++++ .../parameters/{orderly.R => parameters.R} | 0 tests/testthat/test-runner.R | 45 +++++++++++++++++++ 4 files changed, 102 insertions(+) create mode 100644 R/runner.R rename tests/testthat/examples/parameters/{orderly.R => parameters.R} (100%) create mode 100644 tests/testthat/test-runner.R diff --git a/R/queue.R b/R/queue.R index 9e9ff4f..06271a9 100644 --- a/R/queue.R +++ b/R/queue.R @@ -34,6 +34,20 @@ Queue <- R6::R6Class("Queue", #nolint controller = self$controller) }, + submit = function(reportname, parameters = NULL, + branch = "master", ref = "HEAD") { + run_args <- list( + self$root, + reportname, + parameters, + branch, + ref + ) + rrq::rrq_task_create_call(runner_run, run_args, + separate_process = TRUE, + controller = self$controller) + }, + # Just until we add queue status for testing number_of_workers = function() { rrq::rrq_worker_len(self$controller) diff --git a/R/runner.R b/R/runner.R new file mode 100644 index 0000000..2000a8e --- /dev/null +++ b/R/runner.R @@ -0,0 +1,43 @@ +runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { + ## Setup + # working dir + worker_id <- Sys.getenv("WORKER_ID") + worker_path <- file.path(orderly_root, ".packit", "workers", worker_id) + + # ? PASS in repo/path or repo open + # point head to correct ref + gert::git_fetch(repo = worker_path) + gert::git_branch_checkout(branch, repo = worker_path) + gert::git_reset_hard(ref, repo = worker_path) + + ## Run + withr::with_envvar( + c(ORDERLY_REPORT_SRC = file.path(worker_path, "src", reportname)), + orderly2::orderly_run(reportname, parameters = parameters, + root = orderly_root, ...) + ) + + ## Cleanup + # gert does not have git clean but this should achieve the same thing + res <- tryCatch( + gert::git_stash_save( + include_untracked = TRUE, + include_ignored = TRUE + ), + error = function(e) NULL + ) + if (!is.null(res)) { + gert::git_stash_drop() + } + # however git ignores all directories, only cares about files so we may + # have empty directories left, TAF::rmdir removes all empty directories + # ? use fs::dir_walk() + # ? helper function called git_clean + # ? investigate git_clean function + TAF::rmdir(".", recursive = TRUE) +} + +# only on worker startup +clone_orderly_repo <- function(orderly_root, worker_root) { + suppressMessages(gert::git_clone(orderly_root, path = worker_root)) +} diff --git a/tests/testthat/examples/parameters/orderly.R b/tests/testthat/examples/parameters/parameters.R similarity index 100% rename from tests/testthat/examples/parameters/orderly.R rename to tests/testthat/examples/parameters/parameters.R diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R new file mode 100644 index 0000000..67373b4 --- /dev/null +++ b/tests/testthat/test-runner.R @@ -0,0 +1,45 @@ +test_that("runner runs as expected", { + orderly_root <- test_prepare_orderly_example("data") + gert::git_init(orderly_root) + gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) + gert::git_commit("first commit", repo = orderly_root) + + worker_id <- "cutie-patootie" + make_worker_dirs(orderly_root, worker_id) + worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) + + suppressMessages(withr::with_envvar( + c(WORKER_ID = worker_id), + runner_run(orderly_root, "data", NULL, "master", "HEAD", echo = FALSE) + )) + + # report has been run with data in archive + expect_equal(length(list.files(file.path(orderly_root, "archive"))), 1) + # cleanup has deleted draft folder + expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) +}) + +test_that("runner runs as expected with parameters", { + orderly_root <- test_prepare_orderly_example("parameters") + gert::git_init(orderly_root) + gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) + gert::git_commit("first commit", repo = orderly_root) + + worker_id <- "cutie-patootie" + make_worker_dirs(orderly_root, worker_id) + worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) + + parameters <- list(a = -1, b = -2, c = -3) + suppressMessages(withr::with_envvar( + c(WORKER_ID = worker_id), + runner_run(orderly_root, "parameters", parameters, + "master", "HEAD", echo = FALSE) + )) + + report_archive <- file.path(orderly_root, "archive", "parameters") + rds_path <- file.path(report_archive, list.files(report_archive), "data.rds") + output <- readRDS(rds_path) + + expect_equal(output, parameters) + expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) +}) From 855877db052d27ed6de9dddbcaf4645a0336ec5e Mon Sep 17 00:00:00 2001 From: Mantra Date: Tue, 5 Mar 2024 00:36:23 +0000 Subject: [PATCH 12/36] change env var names to match other PRs --- R/runner.R | 9 +++++---- tests/testthat/helper-orderly-runner.R | 14 ++++++++++++++ tests/testthat/test-runner.R | 5 +++-- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/R/runner.R b/R/runner.R index 2000a8e..3ad92bd 100644 --- a/R/runner.R +++ b/R/runner.R @@ -1,7 +1,7 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { ## Setup # working dir - worker_id <- Sys.getenv("WORKER_ID") + worker_id <- Sys.getenv("RRQ_WORKER_ID") worker_path <- file.path(orderly_root, ".packit", "workers", worker_id) # ? PASS in repo/path or repo open @@ -12,7 +12,7 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { ## Run withr::with_envvar( - c(ORDERLY_REPORT_SRC = file.path(worker_path, "src", reportname)), + c(ORDERLY_SRC_ROOT = file.path(worker_path, "src", reportname)), orderly2::orderly_run(reportname, parameters = parameters, root = orderly_root, ...) ) @@ -22,12 +22,13 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { res <- tryCatch( gert::git_stash_save( include_untracked = TRUE, - include_ignored = TRUE + include_ignored = TRUE, + repo = worker_path ), error = function(e) NULL ) if (!is.null(res)) { - gert::git_stash_drop() + gert::git_stash_drop(repo = worker_path) } # however git ignores all directories, only cares about files so we may # have empty directories left, TAF::rmdir removes all empty directories diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 8ff2f86..cf43787 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -46,6 +46,20 @@ new_queue_quietly <- function(root, ...) { suppressMessages(Queue$new(root, ...)) } +make_worker_dirs <- function(orderly_root, ids) { + print(orderly_root) + packit_path <- file.path(orderly_root, ".packit") + dir.create(packit_path) + workers <- file.path(packit_path, "workers") + dir.create(workers) + lapply(ids, function(id) { + worker_path <- file.path(workers, id) + dir.create(worker_path) + gert::git_clone(orderly_root, path = worker_path) + }) + +} + start_queue_workers_quietly <- function(n_workers, controller, env = parent.frame()) { suppressMessages( diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index 67373b4..3e96376 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -9,7 +9,7 @@ test_that("runner runs as expected", { worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) suppressMessages(withr::with_envvar( - c(WORKER_ID = worker_id), + c(RRQ_WORKER_ID = worker_id), runner_run(orderly_root, "data", NULL, "master", "HEAD", echo = FALSE) )) @@ -17,6 +17,7 @@ test_that("runner runs as expected", { expect_equal(length(list.files(file.path(orderly_root, "archive"))), 1) # cleanup has deleted draft folder expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) + browser() }) test_that("runner runs as expected with parameters", { @@ -31,7 +32,7 @@ test_that("runner runs as expected with parameters", { parameters <- list(a = -1, b = -2, c = -3) suppressMessages(withr::with_envvar( - c(WORKER_ID = worker_id), + c(RRQ_WORKER_ID = worker_id), runner_run(orderly_root, "parameters", parameters, "master", "HEAD", echo = FALSE) )) From 3a6de8b6bf0f94a607d2c0888339dff4c325de41 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 11:15:11 +0000 Subject: [PATCH 13/36] got worker running with queue --- R/runner.R | 81 +++++++++++++++----------- tests/testthat/helper-orderly-runner.R | 3 +- tests/testthat/test-queue.R | 16 +++++ tests/testthat/test-runner.R | 4 +- 4 files changed, 68 insertions(+), 36 deletions(-) diff --git a/R/runner.R b/R/runner.R index 3ad92bd..2b64bae 100644 --- a/R/runner.R +++ b/R/runner.R @@ -1,44 +1,59 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { - ## Setup - # working dir + # Helper functions + # ================== + point_head_to_ref <- function(worker_path, branch, ref) { + gert::git_fetch(repo = worker_path) + gert::git_branch_checkout(branch, repo = worker_path) + gert::git_reset_hard(ref, repo = worker_path) + } + + add_dir_parent_if_empty <- function(files_to_delete, path) { + contained_files <- list.files(path, full.names = TRUE) + if (length(setdiff(contained_files, files_to_delete)) > 0) { + return(files_to_delete) + } + add_dir_parent_if_empty(c(files_to_delete, path), dirname(path)) + } + + get_empty_dirs <- function(worker_path) { + dirs <- fs::dir_ls(worker_path, recurse = TRUE, type = "directory") + Reduce(add_dir_parent_if_empty, c(list(character()), dirs)) + } + + git_clean <- function(worker_path) { + # gert does not have git clean but this should achieve the same thing + res <- tryCatch( + gert::git_stash_save( + include_untracked = TRUE, + include_ignored = TRUE, + repo = worker_path + ), + error = function(e) NULL + ) + if (!is.null(res)) { + gert::git_stash_drop(repo = worker_path) + } + # however git ignores all directories, only cares about files, so we may + # have empty directories left + unlink(get_empty_dirs(worker_path), recursive = TRUE) + } + # ================== + + + # Actual runner code + # ================== + # Setup worker_id <- Sys.getenv("RRQ_WORKER_ID") worker_path <- file.path(orderly_root, ".packit", "workers", worker_id) + point_head_to_ref(worker_path, branch, ref) - # ? PASS in repo/path or repo open - # point head to correct ref - gert::git_fetch(repo = worker_path) - gert::git_branch_checkout(branch, repo = worker_path) - gert::git_reset_hard(ref, repo = worker_path) - - ## Run + # Run withr::with_envvar( c(ORDERLY_SRC_ROOT = file.path(worker_path, "src", reportname)), orderly2::orderly_run(reportname, parameters = parameters, root = orderly_root, ...) ) - ## Cleanup - # gert does not have git clean but this should achieve the same thing - res <- tryCatch( - gert::git_stash_save( - include_untracked = TRUE, - include_ignored = TRUE, - repo = worker_path - ), - error = function(e) NULL - ) - if (!is.null(res)) { - gert::git_stash_drop(repo = worker_path) - } - # however git ignores all directories, only cares about files so we may - # have empty directories left, TAF::rmdir removes all empty directories - # ? use fs::dir_walk() - # ? helper function called git_clean - # ? investigate git_clean function - TAF::rmdir(".", recursive = TRUE) -} - -# only on worker startup -clone_orderly_repo <- function(orderly_root, worker_root) { - suppressMessages(gert::git_clone(orderly_root, path = worker_root)) + # Cleanup + git_clean(worker_path) } diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index cf43787..6ef4c8e 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -62,10 +62,11 @@ make_worker_dirs <- function(orderly_root, ids) { start_queue_workers_quietly <- function(n_workers, controller, env = parent.frame()) { - suppressMessages( + worker_manager <- suppressMessages( rrq::rrq_worker_spawn2(n_workers, controller = controller) ) withr::defer(rrq::rrq_worker_stop(controller = controller), env = env) + worker_manager } skip_if_no_redis <- function() { diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index ea8b8a7..0a908c4 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -58,3 +58,19 @@ test_that("Generated namespaced id if no ids exist", { q <- new_queue_quietly(root) expect_match(q$controller$queue_id, "orderly.runner") }) + +test_that("Can submit task", { + skip_if_no_redis() + + root <- test_prepare_orderly_example("data") + gert::git_init(root) + gert::git_add(c("src", "orderly_config.yml"), repo = root) + gert::git_commit("first commit", repo = root) + + q <- new_queue_quietly(root) + worker_manager <- start_queue_workers_quietly(1, q$controller) + make_worker_dirs(root, worker_manager$id) + + task_id <- q$submit("data") + browser() +}) diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index 3e96376..f2f858d 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -4,7 +4,7 @@ test_that("runner runs as expected", { gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) gert::git_commit("first commit", repo = orderly_root) - worker_id <- "cutie-patootie" + worker_id <- "worker1" make_worker_dirs(orderly_root, worker_id) worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) @@ -26,7 +26,7 @@ test_that("runner runs as expected with parameters", { gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) gert::git_commit("first commit", repo = orderly_root) - worker_id <- "cutie-patootie" + worker_id <- "worker1" make_worker_dirs(orderly_root, worker_id) worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) From 4979cd5e24ff04e946f87795cbcc7ab591a5a2d4 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 16:20:37 +0000 Subject: [PATCH 14/36] queue tests done --- tests/testthat/helper-orderly-runner.R | 14 ++++++ tests/testthat/test-queue.R | 59 +++++++++++++++++++++++++- tests/testthat/test-runner.R | 1 - 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 6ef4c8e..7d80f10 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -76,3 +76,17 @@ skip_if_no_redis <- function() { } invisible(available) } + +expect_worker_task_complete <- function(task_id, controller, n_tries) { + is_completed <- FALSE + for (i in seq_len(n_tries)) { + is_completed <- rrq::rrq_task_status( + task_id, controller = controller + ) == "COMPLETE" + if (is_completed == TRUE) { + break + } + Sys.sleep(1) + } + expect_equal(is_completed, TRUE) +} diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index 0a908c4..05a718f 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -59,6 +59,7 @@ test_that("Generated namespaced id if no ids exist", { expect_match(q$controller$queue_id, "orderly.runner") }) + test_that("Can submit task", { skip_if_no_redis() @@ -72,5 +73,61 @@ test_that("Can submit task", { make_worker_dirs(root, worker_manager$id) task_id <- q$submit("data") - browser() + expect_worker_task_complete(task_id, q$controller, 10) +}) + + +test_that("Can submit 2 tasks on different branches", { + skip_if_no_redis() + + root <- test_prepare_orderly_example("data") + gert::git_init(root) + gert::git_add(c("src", "orderly_config.yml"), repo = root) + gert::git_commit("first commit", repo = root) + + gert::git_branch_create("branch1", repo = root) + gert::git_branch_checkout("branch1", repo = root) + write.table("test", file = file.path(root, "test.txt")) + gert::git_add("test.txt", repo = root) + gert::git_commit("branch1 commit", repo = root) + + q <- new_queue_quietly(root) + worker_manager <- start_queue_workers_quietly(2, q$controller) + make_worker_dirs(root, worker_manager$id) + + task_id1 <- q$submit("data", branch = "master") + task_id2 <- q$submit("data", branch = "branch1") + expect_worker_task_complete(task_id1, q$controller, 10) + expect_worker_task_complete(task_id2, q$controller, 10) + + worker_id2 <- rrq::rrq_task_info(task_id2, controller = q$controller)$worker + worker2_txt <- file.path(root, ".packit", "workers", worker_id2, "test.txt") + expect_equal(file.exists(worker2_txt), TRUE) +}) + + +test_that("Can submit 2 tasks on different commit hashes", { + skip_if_no_redis() + + root <- test_prepare_orderly_example("data") + gert::git_init(root) + gert::git_add(c("src", "orderly_config.yml"), repo = root) + sha1 <- gert::git_commit("first commit", repo = root) + + write.table("test", file = file.path(root, "test.txt")) + gert::git_add("test.txt", repo = root) + sha2 <- gert::git_commit("second commit", repo = root) + + q <- new_queue_quietly(root) + worker_manager <- start_queue_workers_quietly(2, q$controller) + make_worker_dirs(root, worker_manager$id) + + task_id1 <- q$submit("data", ref = sha1) + task_id2 <- q$submit("data", ref = sha2) + expect_worker_task_complete(task_id1, q$controller, 10) + expect_worker_task_complete(task_id2, q$controller, 10) + + worker_id2 <- rrq::rrq_task_info(task_id2, controller = q$controller)$worker + worker2_txt <- file.path(root, ".packit", "workers", worker_id2, "test.txt") + expect_equal(file.exists(worker2_txt), TRUE) }) diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index f2f858d..288fac6 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -17,7 +17,6 @@ test_that("runner runs as expected", { expect_equal(length(list.files(file.path(orderly_root, "archive"))), 1) # cleanup has deleted draft folder expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) - browser() }) test_that("runner runs as expected with parameters", { From 6b34b859957d509bde012f846561d490bc8a2724 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 16:21:55 +0000 Subject: [PATCH 15/36] remove debug --- tests/testthat/helper-orderly-runner.R | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 7d80f10..313efe4 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -47,7 +47,6 @@ new_queue_quietly <- function(root, ...) { } make_worker_dirs <- function(orderly_root, ids) { - print(orderly_root) packit_path <- file.path(orderly_root, ".packit") dir.create(packit_path) workers <- file.path(packit_path, "workers") From b1a83c99d86cda5a72d93ec756e112066e456cb7 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 16:35:59 +0000 Subject: [PATCH 16/36] clean up diff --- DESCRIPTION | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index abac113..b092671 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -34,5 +34,5 @@ Config/testthat/edition: 3 Remotes: mrc-ide/orderly2, reside-ic/porcelain, - r-lib/gert, - mrc-ide/rrq + mrc-ide/rrq, + r-lib/gert From ff7a8a342a7c37abd779ac04cd3ad52a9ec6f597 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 17:11:51 +0000 Subject: [PATCH 17/36] fix tests by using git helpers --- scripts/redis | 2 +- tests/testthat/helper-orderly-runner.R | 9 +++++---- tests/testthat/test-git.R | 6 +++--- tests/testthat/test-queue.R | 27 ++++++++------------------ tests/testthat/test-runner.R | 8 ++------ 5 files changed, 19 insertions(+), 33 deletions(-) diff --git a/scripts/redis b/scripts/redis index 202021e..7510fd8 100755 --- a/scripts/redis +++ b/scripts/redis @@ -1,6 +1,6 @@ #!/usr/bin/env bash set -e -CONTAINER_NAME=orderly.server_redis +CONTAINER_NAME=orderly.runner_redis if [ "$1" = "start" ]; then docker run --rm -d --name=$CONTAINER_NAME -p 127.0.0.1:6379:6379 redis elif [ "$1" = "stop" ]; then diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index dccbee8..203a324 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -67,9 +67,9 @@ copy_examples <- function(examples, path_src) { } -helper_add_git <- function(path) { +helper_add_git <- function(path, add = ".") { gert::git_init(path) - gert::git_add(".", repo = path) + gert::git_add(add, repo = path) user <- "author " sha <- gert::git_commit("initial", author = user, committer = user, repo = path) @@ -135,9 +135,10 @@ initialise_git_repo <- function() { } -create_new_commit <- function(path, new_file = "new", message = "new message") { +create_new_commit <- function(path, new_file = "new", message = "new message", + add = ".") { writeLines("new file", file.path(path, new_file)) - gert::git_add(".", repo = path) + gert::git_add(add, repo = path) user <- "author " gert::git_commit(message, author = user, committer = user, repo = path) } diff --git a/tests/testthat/test-git.R b/tests/testthat/test-git.R index 516cc73..0b44c9f 100644 --- a/tests/testthat/test-git.R +++ b/tests/testthat/test-git.R @@ -34,13 +34,13 @@ test_that("can get files which have been modified", { expect_equal(git_get_modified(log$commit[[2]], repo = repo$local), character(0)) expect_equal(git_get_modified(log$commit[[1]], repo = repo$local), - "src/parameters/orderly.R") + "src/parameters/parameters.R") expect_equal(git_get_modified(log$commit[[1]], relative = "src/", repo = repo$local), - "parameters/orderly.R") + "parameters/parameters.R") expect_equal(git_get_modified(log$commit[[1]], base = log$commit[[2]], repo = repo$local), - "src/parameters/orderly.R") + "src/parameters/parameters.R") expect_equal(git_get_modified(log$commit[[2]], base = log$commit[[1]], repo = repo$local), character(0)) diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index 05a718f..dd9ad10 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -64,9 +64,7 @@ test_that("Can submit task", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - gert::git_init(root) - gert::git_add(c("src", "orderly_config.yml"), repo = root) - gert::git_commit("first commit", repo = root) + helper_add_git(root, c("src", "orderly_config.yml")) q <- new_queue_quietly(root) worker_manager <- start_queue_workers_quietly(1, q$controller) @@ -81,22 +79,18 @@ test_that("Can submit 2 tasks on different branches", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - gert::git_init(root) - gert::git_add(c("src", "orderly_config.yml"), repo = root) - gert::git_commit("first commit", repo = root) + helper_add_git(root, c("src", "orderly_config.yml")) - gert::git_branch_create("branch1", repo = root) - gert::git_branch_checkout("branch1", repo = root) - write.table("test", file = file.path(root, "test.txt")) - gert::git_add("test.txt", repo = root) - gert::git_commit("branch1 commit", repo = root) + gert::git_branch_create("branch", repo = root) + gert::git_branch_checkout("branch", repo = root) + create_new_commit(root, new_file = "test.txt", add = "test.txt") q <- new_queue_quietly(root) worker_manager <- start_queue_workers_quietly(2, q$controller) make_worker_dirs(root, worker_manager$id) task_id1 <- q$submit("data", branch = "master") - task_id2 <- q$submit("data", branch = "branch1") + task_id2 <- q$submit("data", branch = "branch") expect_worker_task_complete(task_id1, q$controller, 10) expect_worker_task_complete(task_id2, q$controller, 10) @@ -110,13 +104,8 @@ test_that("Can submit 2 tasks on different commit hashes", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - gert::git_init(root) - gert::git_add(c("src", "orderly_config.yml"), repo = root) - sha1 <- gert::git_commit("first commit", repo = root) - - write.table("test", file = file.path(root, "test.txt")) - gert::git_add("test.txt", repo = root) - sha2 <- gert::git_commit("second commit", repo = root) + sha1 <- helper_add_git(root, c("src", "orderly_config.yml"))$sha + sha2 <- create_new_commit(root, new_file = "test.txt", add = "test.txt") q <- new_queue_quietly(root) worker_manager <- start_queue_workers_quietly(2, q$controller) diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index 288fac6..73c2118 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -1,8 +1,6 @@ test_that("runner runs as expected", { orderly_root <- test_prepare_orderly_example("data") - gert::git_init(orderly_root) - gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) - gert::git_commit("first commit", repo = orderly_root) + helper_add_git(orderly_root, c("src", "orderly_config.yml")) worker_id <- "worker1" make_worker_dirs(orderly_root, worker_id) @@ -21,9 +19,7 @@ test_that("runner runs as expected", { test_that("runner runs as expected with parameters", { orderly_root <- test_prepare_orderly_example("parameters") - gert::git_init(orderly_root) - gert::git_add(c("src", "orderly_config.yml"), repo = orderly_root) - gert::git_commit("first commit", repo = orderly_root) + helper_add_git(orderly_root, c("src", "orderly_config.yml")) worker_id <- "worker1" make_worker_dirs(orderly_root, worker_id) From 0fab7222fed3c389eed54a967d6e38d7f85c3ff6 Mon Sep 17 00:00:00 2001 From: Mantra Date: Thu, 7 Mar 2024 17:36:47 +0000 Subject: [PATCH 18/36] fix test --- tests/testthat/test-api.R | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-api.R b/tests/testthat/test-api.R index 4ecc4d2..c78f199 100644 --- a/tests/testthat/test-api.R +++ b/tests/testthat/test-api.R @@ -32,8 +32,13 @@ test_that("can list orderly reports", { ## Add a report on a 2nd branch gert::git_branch_create("other", repo = repo$local) - fs::dir_copy(file.path(repo$local, "src", "parameters"), + fs::dir_copy(file.path(repo$local, "src", "parameters"), file.path(repo$local, "src", "parameters2")) + # have to rename to parameters2.R to recognise it as orderly report + file.rename( + file.path(repo$local, "src", "parameters2", "parameters.R"), + file.path(repo$local, "src", "parameters2", "parameters2.R") + ) gert::git_add(".", repo = repo$local) sha <- gert::git_commit("Add report data2", repo = repo$local, author = "Test User ") From f48f338cd5d33ceb18d3669f93243d4da693cb7b Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 13:49:00 +0000 Subject: [PATCH 19/36] rob's comments --- R/runner.R | 80 ++++++++++++-------------- tests/testthat/helper-orderly-runner.R | 23 ++++---- tests/testthat/test-queue.R | 12 +--- 3 files changed, 52 insertions(+), 63 deletions(-) diff --git a/R/runner.R b/R/runner.R index 2b64bae..24395b3 100644 --- a/R/runner.R +++ b/R/runner.R @@ -1,47 +1,4 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { - # Helper functions - # ================== - point_head_to_ref <- function(worker_path, branch, ref) { - gert::git_fetch(repo = worker_path) - gert::git_branch_checkout(branch, repo = worker_path) - gert::git_reset_hard(ref, repo = worker_path) - } - - add_dir_parent_if_empty <- function(files_to_delete, path) { - contained_files <- list.files(path, full.names = TRUE) - if (length(setdiff(contained_files, files_to_delete)) > 0) { - return(files_to_delete) - } - add_dir_parent_if_empty(c(files_to_delete, path), dirname(path)) - } - - get_empty_dirs <- function(worker_path) { - dirs <- fs::dir_ls(worker_path, recurse = TRUE, type = "directory") - Reduce(add_dir_parent_if_empty, c(list(character()), dirs)) - } - - git_clean <- function(worker_path) { - # gert does not have git clean but this should achieve the same thing - res <- tryCatch( - gert::git_stash_save( - include_untracked = TRUE, - include_ignored = TRUE, - repo = worker_path - ), - error = function(e) NULL - ) - if (!is.null(res)) { - gert::git_stash_drop(repo = worker_path) - } - # however git ignores all directories, only cares about files, so we may - # have empty directories left - unlink(get_empty_dirs(worker_path), recursive = TRUE) - } - # ================== - - - # Actual runner code - # ================== # Setup worker_id <- Sys.getenv("RRQ_WORKER_ID") worker_path <- file.path(orderly_root, ".packit", "workers", worker_id) @@ -57,3 +14,40 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { # Cleanup git_clean(worker_path) } + +point_head_to_ref <- function(worker_path, branch, ref) { + gert::git_fetch(repo = worker_path) + gert::git_branch_checkout(branch, repo = worker_path) + gert::git_reset_hard(ref, repo = worker_path) +} + +add_dir_parent_if_empty <- function(files_to_delete, path) { + contained_files <- list.files(path, full.names = TRUE) + if (length(setdiff(contained_files, files_to_delete)) > 0) { + return(files_to_delete) + } + add_dir_parent_if_empty(c(files_to_delete, path), dirname(path)) +} + +get_empty_dirs <- function(worker_path) { + dirs <- fs::dir_ls(worker_path, recurse = TRUE, type = "directory") + Reduce(add_dir_parent_if_empty, c(list(character()), dirs)) +} + +git_clean <- function(worker_path) { + # gert does not have git clean but this should achieve the same thing + res <- tryCatch( + gert::git_stash_save( + include_untracked = TRUE, + include_ignored = TRUE, + repo = worker_path + ), + error = function(e) NULL + ) + if (!is.null(res)) { + gert::git_stash_drop(repo = worker_path) + } + # however git ignores all directories, only cares about files, so we may + # have empty directories left + unlink(get_empty_dirs(worker_path), recursive = TRUE) +} diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 203a324..dffcf72 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -105,6 +105,14 @@ start_queue_workers_quietly <- function(n_workers, worker_manager } +start_queue_with_workers <- function(root, n_workers, env = parent.frame()) { + q <- new_queue_quietly(root) + worker_manager <- start_queue_workers_quietly(n_workers, q$controller, + env = env) + make_worker_dirs(root, worker_manager$id) + q +} + skip_if_no_redis <- function() { available <- redux::redis_available() if (!available) { @@ -114,17 +122,10 @@ skip_if_no_redis <- function() { } expect_worker_task_complete <- function(task_id, controller, n_tries) { - is_completed <- FALSE - for (i in seq_len(n_tries)) { - is_completed <- rrq::rrq_task_status( - task_id, controller = controller - ) == "COMPLETE" - if (is_completed == TRUE) { - break - } - Sys.sleep(1) - } - expect_equal(is_completed, TRUE) + is_task_successful <- rrq::rrq_task_wait( + task_id, controller = controller, timeout = n_tries + ) + expect_equal(is_task_successful, TRUE) } initialise_git_repo <- function() { diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index dd9ad10..1153ca7 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -66,9 +66,7 @@ test_that("Can submit task", { root <- test_prepare_orderly_example("data") helper_add_git(root, c("src", "orderly_config.yml")) - q <- new_queue_quietly(root) - worker_manager <- start_queue_workers_quietly(1, q$controller) - make_worker_dirs(root, worker_manager$id) + q <- start_queue_with_workers(root, 1) task_id <- q$submit("data") expect_worker_task_complete(task_id, q$controller, 10) @@ -85,9 +83,7 @@ test_that("Can submit 2 tasks on different branches", { gert::git_branch_checkout("branch", repo = root) create_new_commit(root, new_file = "test.txt", add = "test.txt") - q <- new_queue_quietly(root) - worker_manager <- start_queue_workers_quietly(2, q$controller) - make_worker_dirs(root, worker_manager$id) + q <- start_queue_with_workers(root, 2) task_id1 <- q$submit("data", branch = "master") task_id2 <- q$submit("data", branch = "branch") @@ -107,9 +103,7 @@ test_that("Can submit 2 tasks on different commit hashes", { sha1 <- helper_add_git(root, c("src", "orderly_config.yml"))$sha sha2 <- create_new_commit(root, new_file = "test.txt", add = "test.txt") - q <- new_queue_quietly(root) - worker_manager <- start_queue_workers_quietly(2, q$controller) - make_worker_dirs(root, worker_manager$id) + q <- start_queue_with_workers(root, 2) task_id1 <- q$submit("data", ref = sha1) task_id2 <- q$submit("data", ref = sha2) From 74d2ac04984d09d39c51fa0989e6122ad6a7f8ea Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 14:12:11 +0000 Subject: [PATCH 20/36] cov, git clean test --- tests/testthat/examples/git-clean/git-clean.R | 5 +++++ tests/testthat/test-runner.R | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 tests/testthat/examples/git-clean/git-clean.R diff --git a/tests/testthat/examples/git-clean/git-clean.R b/tests/testthat/examples/git-clean/git-clean.R new file mode 100644 index 0000000..5115f7b --- /dev/null +++ b/tests/testthat/examples/git-clean/git-clean.R @@ -0,0 +1,5 @@ +orderly2::orderly_artefact("Some data", "data.rds") +d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10)) +write.table("test", file = file.path("..", "..", "inside_draft.txt")) +write.table("test", file = file.path("..", "..", "..", "outside_draft.txt")) +saveRDS(d, "data.rds") diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index 73c2118..9bbab0c 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -39,3 +39,24 @@ test_that("runner runs as expected with parameters", { expect_equal(output, parameters) expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) }) + +test_that("git clean clears unnecessary files", { + # git-clean.R spawns a file in draft folder and one in worker root folder + # and there will also be an empty folder draft/git-clean so we test + # all components of git_clean + orderly_root <- test_prepare_orderly_example("git-clean") + helper_add_git(orderly_root, c("src", "orderly_config.yml")) + + worker_id <- "worker1" + make_worker_dirs(orderly_root, worker_id) + worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) + + suppressMessages(withr::with_envvar( + c(RRQ_WORKER_ID = worker_id), + runner_run(orderly_root, "git-clean", NULL, "master", "HEAD", echo = FALSE) + )) + + expect_equal(length(list.files(file.path(orderly_root, "archive"))), 1) + expect_equal(file.exists(file.path(worker_root, "draft")), FALSE) + expect_equal(file.exists(file.path(worker_root, "outside_draft.txt")), FALSE) +}) From 4bc17a23e280c46d69486257ecda739fb258ca95 Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 14:24:48 +0000 Subject: [PATCH 21/36] fix tests referencing orderly.R --- tests/testthat/test-reports.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-reports.R b/tests/testthat/test-reports.R index 2d9fbe2..acba6df 100644 --- a/tests/testthat/test-reports.R +++ b/tests/testthat/test-reports.R @@ -3,7 +3,7 @@ test_that("can get orderly script name", { git_info <- helper_add_git(root) expect_equal(get_orderly_script_path("data", "HEAD", root), "src/data/data.R") expect_equal(get_orderly_script_path("parameters", "HEAD", root), - "src/parameters/orderly.R") + "src/parameters/parameters.R") file.copy(file.path(root, "src", "data", "data.R"), file.path(root, "src", "data", "orderly.R")) @@ -28,7 +28,7 @@ test_that("can get report parameters", { c = NULL)) ## Works with a specific git hash - params_src <- file.path(root, "src", "parameters", "orderly.R") + params_src <- file.path(root, "src", "parameters", "parameters.R") contents <- readLines(params_src) contents <- c("orderly2::orderly_parameters(a = 'default', b = 2, c = NULL)", contents[-1]) From 7e09a01f0f310c72fa047b55a6ad1a7c9c1cb319 Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 14:28:28 +0000 Subject: [PATCH 22/36] fix helper function --- tests/testthat/helper-orderly-runner.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index e68428e..a3b41a3 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -149,9 +149,9 @@ git_add_and_commit <- function(path, add = ".") { } -create_new_commit <- function(path, new_file = "new") { +create_new_commit <- function(path, new_file = "new", add = ".") { writeLines("new file", file.path(path, new_file)) - git_add_and_commit(path) + git_add_and_commit(path, add) } From 70533e19e61f282f05434c95590efda56fc9932c Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 15:45:00 +0000 Subject: [PATCH 23/36] try to fix test --- .github/workflows/R-CMD-check.yaml | 4 ++++ tests/testthat/test-runner.R | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 758f866..7d7c23c 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -47,6 +47,10 @@ jobs: extra-packages: any::rcmdcheck needs: check + - name: Start Redis + run: | + ./scripts/redis start + - uses: r-lib/actions/check-r-package@v2 with: upload-snapshots: true diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index 9bbab0c..f52867d 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -2,7 +2,7 @@ test_that("runner runs as expected", { orderly_root <- test_prepare_orderly_example("data") helper_add_git(orderly_root, c("src", "orderly_config.yml")) - worker_id <- "worker1" + worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) @@ -21,7 +21,7 @@ test_that("runner runs as expected with parameters", { orderly_root <- test_prepare_orderly_example("parameters") helper_add_git(orderly_root, c("src", "orderly_config.yml")) - worker_id <- "worker1" + worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) @@ -47,7 +47,7 @@ test_that("git clean clears unnecessary files", { orderly_root <- test_prepare_orderly_example("git-clean") helper_add_git(orderly_root, c("src", "orderly_config.yml")) - worker_id <- "worker1" + worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) worker_root <- file.path(orderly_root, ".packit", "workers", worker_id) From 441f844443b6bd00301c442ef3ed06e5f962820a Mon Sep 17 00:00:00 2001 From: Mantra Date: Fri, 8 Mar 2024 15:55:50 +0000 Subject: [PATCH 24/36] tmate --- .github/workflows/R-CMD-check.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 7d7c23c..a1517d9 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -51,6 +51,8 @@ jobs: run: | ./scripts/redis start + - name: Setup tmate session + uses: mxschmitt/action-tmate@v3 - uses: r-lib/actions/check-r-package@v2 with: upload-snapshots: true From 6264648f4cf28bf7a87156ddce393792d225fba7 Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:14:31 +0000 Subject: [PATCH 25/36] add user name and email to worker git config --- tests/testthat/helper-orderly-runner.R | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index a3b41a3..949ec18 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -88,6 +88,8 @@ make_worker_dirs <- function(orderly_root, ids) { lapply(ids, function(id) { worker_path <- file.path(workers, id) dir.create(worker_path) + gert::git_config_set("user.name", id) + gert::git_config_set("user.email", id) gert::git_clone(orderly_root, path = worker_path) }) From b1daffd93637ff0fb1c56ca2922853d03efb4228 Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:17:26 +0000 Subject: [PATCH 26/36] remove tmate --- .github/workflows/R-CMD-check.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index a1517d9..7d7c23c 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -51,8 +51,6 @@ jobs: run: | ./scripts/redis start - - name: Setup tmate session - uses: mxschmitt/action-tmate@v3 - uses: r-lib/actions/check-r-package@v2 with: upload-snapshots: true From c7d03e6d4cd3b3251b4ee0116b20764d6561848f Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:19:09 +0000 Subject: [PATCH 27/36] test --- R/runner.R | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/R/runner.R b/R/runner.R index 24395b3..68660af 100644 --- a/R/runner.R +++ b/R/runner.R @@ -36,6 +36,16 @@ get_empty_dirs <- function(worker_path) { git_clean <- function(worker_path) { # gert does not have git clean but this should achieve the same thing + withCallingHandlers( + gert::git_stash_save( + include_untracked = TRUE, + include_ignored = TRUE, + repo = worker_path + ), + error = function(e) { + browser() + } + ) res <- tryCatch( gert::git_stash_save( include_untracked = TRUE, From 17a32aad64d453778983a9c72a0e5a71871c9397 Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:22:45 +0000 Subject: [PATCH 28/36] error handling --- R/runner.R | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/R/runner.R b/R/runner.R index 68660af..868a5b8 100644 --- a/R/runner.R +++ b/R/runner.R @@ -36,24 +36,22 @@ get_empty_dirs <- function(worker_path) { git_clean <- function(worker_path) { # gert does not have git clean but this should achieve the same thing - withCallingHandlers( + res <- withCallingHandlers( gert::git_stash_save( include_untracked = TRUE, include_ignored = TRUE, repo = worker_path ), error = function(e) { - browser() + # we don't need to rethrow the error here since it doesn't break any + # further report runs + if (e$message != "cannot stash changes - there is nothing to stash.") { + # TODO add logger here + message(e$message) + } + NULL } ) - res <- tryCatch( - gert::git_stash_save( - include_untracked = TRUE, - include_ignored = TRUE, - repo = worker_path - ), - error = function(e) NULL - ) if (!is.null(res)) { gert::git_stash_drop(repo = worker_path) } From f4655eb40f8384ea1311b98ebdcf9cca6861c8db Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:25:24 +0000 Subject: [PATCH 29/36] refactor git stash for better error handling --- R/runner.R | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/R/runner.R b/R/runner.R index 868a5b8..3902709 100644 --- a/R/runner.R +++ b/R/runner.R @@ -36,12 +36,15 @@ get_empty_dirs <- function(worker_path) { git_clean <- function(worker_path) { # gert does not have git clean but this should achieve the same thing - res <- withCallingHandlers( - gert::git_stash_save( - include_untracked = TRUE, - include_ignored = TRUE, - repo = worker_path - ), + withCallingHandlers( + { + gert::git_stash_save( + include_untracked = TRUE, + include_ignored = TRUE, + repo = worker_path + ) + gert::git_stash_drop(repo = worker_path) + }, error = function(e) { # we don't need to rethrow the error here since it doesn't break any # further report runs @@ -52,9 +55,6 @@ git_clean <- function(worker_path) { NULL } ) - if (!is.null(res)) { - gert::git_stash_drop(repo = worker_path) - } # however git ignores all directories, only cares about files, so we may # have empty directories left unlink(get_empty_dirs(worker_path), recursive = TRUE) From 8c88f395106e4ac373f6b9c26f03d811d99a65d1 Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:30:30 +0000 Subject: [PATCH 30/36] add repo arg --- tests/testthat/helper-orderly-runner.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 949ec18..56b52cc 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -88,9 +88,9 @@ make_worker_dirs <- function(orderly_root, ids) { lapply(ids, function(id) { worker_path <- file.path(workers, id) dir.create(worker_path) - gert::git_config_set("user.name", id) - gert::git_config_set("user.email", id) gert::git_clone(orderly_root, path = worker_path) + gert::git_config_set("user.name", id, repo = worker_path) + gert::git_config_set("user.email", id, repo = worker_path) }) } From 53db6cb4320adeefb2b53f92abeda1098fad6d9b Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 16:48:04 +0000 Subject: [PATCH 31/36] revert to tryCatch for now --- R/runner.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/runner.R b/R/runner.R index 3902709..3cc3103 100644 --- a/R/runner.R +++ b/R/runner.R @@ -36,7 +36,7 @@ get_empty_dirs <- function(worker_path) { git_clean <- function(worker_path) { # gert does not have git clean but this should achieve the same thing - withCallingHandlers( + tryCatch( { gert::git_stash_save( include_untracked = TRUE, From 070805a5b9c4d316324f58f3f674ee96ec56a5cf Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 18:25:04 +0000 Subject: [PATCH 32/36] not running redis on mac --- .github/workflows/R-CMD-check.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 7d7c23c..dd5ae8b 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -49,7 +49,11 @@ jobs: - name: Start Redis run: | - ./scripts/redis start + if [ ${{ matrix.config.os }} == 'macos-latest' ]; then + echo "Not starting redis on Mac" + else + ./scripts/redis start + fi - uses: r-lib/actions/check-r-package@v2 with: From 11b747b12e7399ff3ad94dbd630a57fdad505ad3 Mon Sep 17 00:00:00 2001 From: boring_grebe Date: Fri, 8 Mar 2024 18:34:04 +0000 Subject: [PATCH 33/36] better if --- .github/workflows/R-CMD-check.yaml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index dd5ae8b..a808d97 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -48,12 +48,9 @@ jobs: needs: check - name: Start Redis + if: ${{ matrix.config.os != 'macos-latest' }} run: | - if [ ${{ matrix.config.os }} == 'macos-latest' ]; then - echo "Not starting redis on Mac" - else - ./scripts/redis start - fi + ./scripts/redis start - uses: r-lib/actions/check-r-package@v2 with: From b69faaceafec9116522117eb08ecc685b54a86a6 Mon Sep 17 00:00:00 2001 From: M-Kusumgar Date: Fri, 8 Mar 2024 18:40:34 +0000 Subject: [PATCH 34/36] change identity to me From 7f8f52f67362a4a48a3ef2177efb9925f45bf154 Mon Sep 17 00:00:00 2001 From: Rob Ashton Date: Fri, 3 May 2024 17:59:17 +0100 Subject: [PATCH 35/36] Update to work with latest rrq, make branch tests work with any default branch --- DESCRIPTION | 2 +- R/queue.R | 4 ++-- tests/testthat/helper-orderly-runner.R | 6 +++--- tests/testthat/test-queue.R | 16 ++++++++-------- tests/testthat/test-runner.R | 14 ++++++++------ 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 2e8b3dc..34add46 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -22,7 +22,7 @@ Imports: porcelain, R6, redux, - rrq, + rrq (>= 0.7.15), withr Suggests: fs, diff --git a/R/queue.R b/R/queue.R index 06271a9..09a37f4 100644 --- a/R/queue.R +++ b/R/queue.R @@ -26,11 +26,11 @@ Queue <- R6::R6Class("Queue", #nolint } # Create queue - self$controller <- rrq::rrq_controller2( + self$controller <- rrq::rrq_controller( queue_id %||% orderly_queue_id() ) worker_config <- rrq::rrq_worker_config(heartbeat_period = 10) - rrq::rrq_worker_config_save2("localhost", worker_config, + rrq::rrq_worker_config_save("localhost", worker_config, controller = self$controller) }, diff --git a/tests/testthat/helper-orderly-runner.R b/tests/testthat/helper-orderly-runner.R index 56b52cc..20cda70 100644 --- a/tests/testthat/helper-orderly-runner.R +++ b/tests/testthat/helper-orderly-runner.R @@ -20,7 +20,7 @@ new_queue_quietly <- function(root, ...) { start_queue_workers_quietly <- function(n_workers, controller, env = parent.frame()) { suppressMessages( - rrq::rrq_worker_spawn2(n_workers, controller = controller) + rrq::rrq_worker_spawn(n_workers, controller = controller) ) withr::defer(rrq::rrq_worker_stop(controller = controller), env = env) } @@ -98,7 +98,7 @@ make_worker_dirs <- function(orderly_root, ids) { start_queue_workers_quietly <- function(n_workers, controller, env = parent.frame()) { worker_manager <- suppressMessages( - rrq::rrq_worker_spawn2(n_workers, controller = controller) + rrq::rrq_worker_spawn(n_workers, controller = controller) ) withr::defer(rrq::rrq_worker_stop(controller = controller), env = env) worker_manager @@ -124,7 +124,7 @@ expect_worker_task_complete <- function(task_id, controller, n_tries) { is_task_successful <- rrq::rrq_task_wait( task_id, controller = controller, timeout = n_tries ) - expect_equal(is_task_successful, TRUE) + expect_true(is_task_successful) } initialise_git_repo <- function() { diff --git a/tests/testthat/test-queue.R b/tests/testthat/test-queue.R index 1153ca7..cb3e078 100644 --- a/tests/testthat/test-queue.R +++ b/tests/testthat/test-queue.R @@ -6,7 +6,7 @@ test_that("Can bring up queue", { q <- new_queue_quietly(root) expect_equal(q$root, root) expect_equal(q$config$core$use_file_store, TRUE) - expect_equal(q$controller, rrq::rrq_controller2(q$controller$queue_id)) + expect_equal(q$controller, rrq::rrq_controller(q$controller$queue_id)) expect_equal(q$number_of_workers(), 0) start_queue_workers_quietly(1, q$controller) @@ -64,11 +64,11 @@ test_that("Can submit task", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - helper_add_git(root, c("src", "orderly_config.yml")) + git_info <- helper_add_git(root, c("src", "orderly_config.yml")) q <- start_queue_with_workers(root, 1) - task_id <- q$submit("data") + task_id <- q$submit("data", branch = git_info$branch) expect_worker_task_complete(task_id, q$controller, 10) }) @@ -77,7 +77,7 @@ test_that("Can submit 2 tasks on different branches", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - helper_add_git(root, c("src", "orderly_config.yml")) + git_info <- helper_add_git(root, c("src", "orderly_config.yml")) gert::git_branch_create("branch", repo = root) gert::git_branch_checkout("branch", repo = root) @@ -85,7 +85,7 @@ test_that("Can submit 2 tasks on different branches", { q <- start_queue_with_workers(root, 2) - task_id1 <- q$submit("data", branch = "master") + task_id1 <- q$submit("data", branch = git_info$branch) task_id2 <- q$submit("data", branch = "branch") expect_worker_task_complete(task_id1, q$controller, 10) expect_worker_task_complete(task_id2, q$controller, 10) @@ -100,13 +100,13 @@ test_that("Can submit 2 tasks on different commit hashes", { skip_if_no_redis() root <- test_prepare_orderly_example("data") - sha1 <- helper_add_git(root, c("src", "orderly_config.yml"))$sha + git_info <- helper_add_git(root, c("src", "orderly_config.yml")) sha2 <- create_new_commit(root, new_file = "test.txt", add = "test.txt") q <- start_queue_with_workers(root, 2) - task_id1 <- q$submit("data", ref = sha1) - task_id2 <- q$submit("data", ref = sha2) + task_id1 <- q$submit("data", ref = git_info$sha, branch = git_info$branch) + task_id2 <- q$submit("data", ref = sha2, branch = git_info$branch) expect_worker_task_complete(task_id1, q$controller, 10) expect_worker_task_complete(task_id2, q$controller, 10) diff --git a/tests/testthat/test-runner.R b/tests/testthat/test-runner.R index f52867d..0de32bf 100644 --- a/tests/testthat/test-runner.R +++ b/tests/testthat/test-runner.R @@ -1,6 +1,6 @@ test_that("runner runs as expected", { orderly_root <- test_prepare_orderly_example("data") - helper_add_git(orderly_root, c("src", "orderly_config.yml")) + git_info <- helper_add_git(orderly_root, c("src", "orderly_config.yml")) worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) @@ -8,7 +8,8 @@ test_that("runner runs as expected", { suppressMessages(withr::with_envvar( c(RRQ_WORKER_ID = worker_id), - runner_run(orderly_root, "data", NULL, "master", "HEAD", echo = FALSE) + runner_run(orderly_root, "data", NULL, git_info$branch, + "HEAD", echo = FALSE) )) # report has been run with data in archive @@ -19,7 +20,7 @@ test_that("runner runs as expected", { test_that("runner runs as expected with parameters", { orderly_root <- test_prepare_orderly_example("parameters") - helper_add_git(orderly_root, c("src", "orderly_config.yml")) + git_info <- helper_add_git(orderly_root, c("src", "orderly_config.yml")) worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) @@ -29,7 +30,7 @@ test_that("runner runs as expected with parameters", { suppressMessages(withr::with_envvar( c(RRQ_WORKER_ID = worker_id), runner_run(orderly_root, "parameters", parameters, - "master", "HEAD", echo = FALSE) + git_info$branch, "HEAD", echo = FALSE) )) report_archive <- file.path(orderly_root, "archive", "parameters") @@ -45,7 +46,7 @@ test_that("git clean clears unnecessary files", { # and there will also be an empty folder draft/git-clean so we test # all components of git_clean orderly_root <- test_prepare_orderly_example("git-clean") - helper_add_git(orderly_root, c("src", "orderly_config.yml")) + git_info <- helper_add_git(orderly_root, c("src", "orderly_config.yml")) worker_id <- ids::adjective_animal() make_worker_dirs(orderly_root, worker_id) @@ -53,7 +54,8 @@ test_that("git clean clears unnecessary files", { suppressMessages(withr::with_envvar( c(RRQ_WORKER_ID = worker_id), - runner_run(orderly_root, "git-clean", NULL, "master", "HEAD", echo = FALSE) + runner_run(orderly_root, "git-clean", NULL, git_info$branch, + "HEAD", echo = FALSE) )) expect_equal(length(list.files(file.path(orderly_root, "archive"))), 1) From ca6ec008ec89da115b7574437368476d2b3268c7 Mon Sep 17 00:00:00 2001 From: M-Kusumgar Date: Thu, 1 Aug 2024 16:48:26 +0100 Subject: [PATCH 36/36] cleanup before report run --- R/runner.R | 3 +++ 1 file changed, 3 insertions(+) diff --git a/R/runner.R b/R/runner.R index 3cc3103..a08694a 100644 --- a/R/runner.R +++ b/R/runner.R @@ -4,6 +4,9 @@ runner_run <- function(orderly_root, reportname, parameters, branch, ref, ...) { worker_path <- file.path(orderly_root, ".packit", "workers", worker_id) point_head_to_ref(worker_path, branch, ref) + # Initial cleanup + git_clean(worker_path) + # Run withr::with_envvar( c(ORDERLY_SRC_ROOT = file.path(worker_path, "src", reportname)),