Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mrc-3167: Add function to cancel reports running on remote #314

Merged
merged 3 commits into from
Apr 26, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: orderly
Title: Lightweight Reproducible Reporting
Version: 1.4.6
Version: 1.4.7
Description: Order, create and store reports from R. By defining a
lightweight interface around the inputs and outputs of an
analysis, a lot of the repetitive work for reproducible research
Expand Down
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export(orderly_bundle_list)
export(orderly_bundle_pack)
export(orderly_bundle_pack_remote)
export(orderly_bundle_run)
export(orderly_cancel_remote)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be orderly_remote_cancel to mirror orderly_remote_status or stay orderly_cancel_remote to mirror orderly_run_remote? Naming things is hard

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have more functions which are orderly_x_remote, though they do tend to be when there is a local equivalent - which there isn't here. I think orderly_cancel_remote feels more natural wording to me so I am happy to stick with that, I don't mind switching it over if you prefer though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be orderly_remote_cancel to mirror orderly_remote_status or stay orderly_cancel_remote to mirror orderly_run_remote? Naming things is hard

export(orderly_cleanup)
export(orderly_commit)
export(orderly_config)
Expand Down
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# orderly 1.4.7

* Add `orderly_cancel_remote` which can be used to cancel one or more reports running on a remote via its key (mrc-3167)

# orderly 1.4.3

* Orderly now only records git information where a `.git` directory is found at the orderly root (vimc-4866)
Expand Down
26 changes: 26 additions & 0 deletions R/remote.R
Original file line number Diff line number Diff line change
Expand Up @@ -662,3 +662,29 @@ pull_info <- function(name, id, root, locate, remote, parameters) {
config = config,
remote = remote)
}


##' Cancel a report
##'
##' The action will depend on the status of the report:
##' * queued - report run will be deleted
##' * running - report run will be cancelled
##' * complete/errored - no effect
##'
##' @param keys The key or keys for the reports to cancel
##'
##' @inheritParams orderly_pull_dependencies
##'
##' @return List with names as report keys and values are lists containing
##' * `killed` - boolean TRUE if report successfully cancelled, FALSE
##' otherwise
##' * `message` - string detailing reason why cancellation failed
##'
##' @export
orderly_cancel_remote <- function(keys, root = NULL, locate = TRUE,
remote = NULL) {
remote <- get_remote(remote, orderly_config(root, locate))
out <- lapply(keys, remote$kill)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want to make this part of the "implements remote" thing, fall back here:

if (is.null(remote$kill)) {
  stop(sprintf("your remote (there's a way of getting the type I believe) does not implement kill")
} 

that's probably the tidy thing to do as we don't want to implement this for the path remote

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want to make this part of the "implements remote" thing, fall back here:

if (is.null(remote$kill)) {
  stop(sprintf("your remote (there's a way of getting the type I believe) does not implement kill")
} 

that's probably the tidy thing to do as we don't want to implement this for the path remote

names(out) <- keys
out
}
45 changes: 45 additions & 0 deletions man/orderly_cancel_remote.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions tests/testthat/test-remote.R
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,40 @@ test_that("Can't pull incompatible metadata", {
remote = dat$remote),
"Report was created with orderly more recent than this, upgrade!")
})


test_that("reports can be cancelled", {
path_local <- test_prepare_orderly_example("demo")

res_1 <- list(
killed = TRUE,
message = NULL
)
res_2 <- list(
killed = FALSE,
message = "Could not kill task, already complete"
)
mock_kill <- mockery::mock(res_1, res_2)
## Create a minimal remote class which will satisfy implements_remote
mock_remote <- R6::R6Class(
"orderly_mock_remote",
lock_objects = FALSE,
public = list(
list_reports = function() TRUE,
list_versions = function() TRUE,
pull = function() TRUE,
run = function() TRUE,
url_report = function() TRUE,
queue_status = function() TRUE,
kill = function(key) mock_kill()
)
)

remote <- mock_remote$new()
res <- orderly_cancel_remote(
c("fungiform_kiwi", "lithe_iaerismetalmark"), remote = remote)
mockery::expect_called(mock_kill, 2)
expect_equal(names(res), c("fungiform_kiwi", "lithe_iaerismetalmark"))
expect_equal(res$fungiform_kiwi, res_1)
expect_equal(res$lithe_iaerismetalmark, res_2)
})