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 5585 submit endpoint #6

Merged
merged 21 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 19 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
34 changes: 32 additions & 2 deletions R/api.R
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,28 @@
##' @param log_level Logging level to use. Sensible options are "off",
##' "info" and "all".
##'
##' @param skip_queue_creation Skip queue creation, this is primarily
##' used for tests where we can't establish a Redis connection.
##'
##' @return A [porcelain::porcelain] object. Notably this does *not*
##' start the server
##'
##' @export
api <- function(root, validate = NULL, log_level = "info") {
api <- function(
root, validate = NULL, log_level = "info",
skip_queue_creation = FALSE
) {
logger <- porcelain::porcelain_logger(log_level)

# Set ORDERLY_RUNNER_QUEUE_ID to specify existing queue id
if (skip_queue_creation) {
queue <- NULL
} else {
queue <- Queue$new(root)
}

api <- porcelain::porcelain$new(validate = validate, logger = logger)
api$include_package_endpoints(state = list(root = root))
api$include_package_endpoints(state = list(root = root, queue = queue))
api
}

Expand Down Expand Up @@ -70,3 +84,19 @@ report_parameters <- function(root, ref, name) {
)
})
}

##' @porcelain
##' POST /report/run => json(report_run_response)
##' state root :: root
##' state queue :: queue
##' body data :: json(report_run_request)
submit_report_run <- function(root, queue, data) {
data <- jsonlite::parse_json(data)
job_id <- queue$submit(
data$name,
branch = data$branch,
ref = data$hash,
parameters = data$parameters
)
list(job_id = scalar(job_id))
}
10 changes: 10 additions & 0 deletions R/porcelain.R

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

9 changes: 9 additions & 0 deletions R/queue.R
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,15 @@ Queue <- R6::R6Class("Queue", #nolint
controller = self$controller)
},

#' @description
#' Submit a job the Redis queue for runner to run.
#'
#' @param reportname Name of orderly report.
#' @param parameters Parameters to run the report with (default NULL)
#' @param branch Name of git branch to checkout the repository
#' (default master)
#' @param ref Git commit-ish value (e.g HEAD or commit hash or branch name).
#' We reset hard to this ref and run the report. (default HEAD)
submit = function(reportname, parameters = NULL,
branch = "master", ref = "HEAD") {
run_args <- list(
Expand Down
27 changes: 27 additions & 0 deletions inst/schema/report_run_request.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
"properties": {
"name": {
"type": "string"
},
"branch": {
"type": "string"
},
"hash": {
"type": "string"
},
Comment on lines +8 to +13
Copy link
Member

Choose a reason for hiding this comment

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

why do we have both branch and hash, and not ref? Probably this is a good idea though tbh

What is the interpretation of branch names? Do we add main/ to them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the runner checks out to that branch and resets head to the hash, we would have a different behaviour if we used a ref right? would we not get a detached head if someone specified just a hash

there will be some extra work in the UI if we want people to be able to specify a commit hash on a branch instead of just a commit hash but that can come later

branch names are just what we do a git checkout to before resetting to the specified hash, im not sure what you mean by adding main/ to them but we dont do anything like that, just pass branchname into git checkout via gert

"parameters": {
"oneOf": [
{ "type": "null" },
{
"type": "object",
"properties": {},
"additionalProperties": true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"properties": {},
"additionalProperties": true
"additionalProperties": {
"type": ["boolean", "integer", "number", "string"]
}

we can be stricter here, and the properties key does nothing in the current version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
]
}
},
"required": ["name", "branch", "hash", "parameters"],
"additionalProperties": false
}
11 changes: 11 additions & 0 deletions inst/schema/report_run_response.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
"properties": {
"job_id": {
"type": "string"
}
},
"required": ["job_id"],
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to return as a string rather than an object? if an object any reason not to use jobId as that's probably closer to what Packit uses? And any reason we can't use task not job as that's what rrq uses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no reason other than i was speculating that we may want to add other things that return from this endpoint, and having an object returned means an easier future refactor in kotlin if another field is needed

yep this can definitely be camelcase

job_id was only used because I believe the sequence diagram uses job id and that percolated through to the tickets, i can change it to task!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have changed to taskId

"additionalProperties": false
}
26 changes: 26 additions & 0 deletions man/Queue.Rd

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

5 changes: 4 additions & 1 deletion man/api.Rd

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

2 changes: 1 addition & 1 deletion tests/testthat/examples/data/data.R
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
orderly2::orderly_artefact("Some data", "data.rds")
orderly2::orderly_artefact(description = "Some data", "data.rds")
d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10))
Copy link
Member

Choose a reason for hiding this comment

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

sorry, this is for the greater good...

saveRDS(d, "data.rds")
2 changes: 1 addition & 1 deletion tests/testthat/examples/git-clean/git-clean.R
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
orderly2::orderly_artefact("Some data", "data.rds")
orderly2::orderly_artefact(description = "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"))
Expand Down
33 changes: 25 additions & 8 deletions tests/testthat/helper-orderly-runner.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
orderly_runner_endpoint <- function(method, path, root, validate = TRUE) {
porcelain::porcelain_package_endpoint("orderly.runner", method, path,
state = list(root = root),
validate = validate)
orderly_runner_endpoint <- function(
method, path, root,
validate = TRUE,
skip_queue_creation = FALSE
) {
if (skip_queue_creation) {
queue <- NULL
} else {
queue <- Queue$new(root)
}
porcelain::porcelain_package_endpoint(
"orderly.runner", method, path,
state = list(root = root, queue = queue),
validate = validate
)
}


Expand Down Expand Up @@ -46,10 +57,11 @@ test_prepare_orderly_example <- function(examples, ...) {

test_prepare_orderly_remote_example <- function(examples, ...) {
path_remote <- test_prepare_orderly_example(examples, ...)
helper_add_git(path_remote)
helper_add_git(path_remote, orderly_gitignore = TRUE)
path_local <- tempfile()
withr::defer_parent(unlink(path_local, recursive = TRUE))
gert::git_clone(path_remote, path_local)
orderly2::orderly_init(root = path_local, force = TRUE)
list(
remote = path_remote,
local = path_local
Expand All @@ -67,8 +79,11 @@ copy_examples <- function(examples, path_src) {
}


helper_add_git <- function(path, add = ".") {
helper_add_git <- function(path, add = ".", orderly_gitignore = FALSE) {
gert::git_init(path)
if (orderly_gitignore) {
orderly2::orderly_gitignore_update("(root)", root = path)
}
sha <- git_add_and_commit(path, add)
branch <- gert::git_branch(repo = path)
url <- "https://example.com/git"
Expand Down Expand Up @@ -104,8 +119,10 @@ 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)
start_queue_with_workers <- function(
root, n_workers, env = parent.frame(), queue_id = NULL
) {
q <- new_queue_quietly(root, queue_id = queue_id)
worker_manager <- start_queue_workers_quietly(n_workers, q$controller,
env = env)
make_worker_dirs(root, worker_manager$id)
Expand Down
66 changes: 60 additions & 6 deletions tests/testthat/test-api.R
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
test_that("root data returns sensible, validated, data", {
## Just hello world for the package really
endpoint <- orderly_runner_endpoint("GET", "/", NULL)
endpoint <- orderly_runner_endpoint("GET", "/", NULL,
skip_queue_creation = TRUE)
res <- endpoint$run()
expect_true(res$validated)
expect_true(all(c("orderly2", "orderly.runner") %in%
Expand All @@ -11,7 +12,7 @@ test_that("root data returns sensible, validated, data", {

test_that("Can construct the api", {
root <- create_temporary_root(use_file_store = TRUE)
obj <- api(root)
obj <- api(root, skip_queue_creation = TRUE)
result <- evaluate_promise(value <- obj$request("GET", "/")$status)
expect_equal(value, 200)
logs <- lapply(strsplit(result$output, "\n")[[1]], jsonlite::parse_json)
Expand All @@ -22,7 +23,10 @@ test_that("Can construct the api", {

test_that("can list orderly reports", {
repo <- test_prepare_orderly_remote_example(c("data", "parameters"))
endpoint <- orderly_runner_endpoint("GET", "/report/list", repo$local)
endpoint <- orderly_runner_endpoint(
"GET", "/report/list",
repo$local, skip_queue_creation = TRUE
)

res <- endpoint$run(gert::git_branch(repo$local))
expect_equal(res$status_code, 200)
Expand Down Expand Up @@ -61,8 +65,10 @@ test_that("can list orderly reports", {

test_that("can get parameters for a report", {
repo <- test_prepare_orderly_remote_example(c("data", "parameters"))
endpoint <- orderly_runner_endpoint("GET", "/report/<name:string>/parameters",
repo$local)
endpoint <- orderly_runner_endpoint(
"GET", "/report/<name:string>/parameters",
repo$local, skip_queue_creation = TRUE
)

res <- endpoint$run("HEAD", "data")
expect_equal(res$status_code, 200)
Expand All @@ -73,6 +79,54 @@ test_that("can get parameters for a report", {
expect_equal(res$data, list(
list(name = scalar("a"), value = NULL),
list(name = scalar("b"), value = scalar("2")),
list(name = scalar("c"), value = NULL))
list(name = scalar("c"), value = NULL)
))
})

test_that("can run orderly reports", {
skip_if_no_redis()

queue_id <- "orderly.runner:cute-animal"
repo <- test_prepare_orderly_example(c("data", "parameters"))
gert::git_init(repo)
orderly2::orderly_gitignore_update("(root)", root = repo)
git_add_and_commit(repo)
queue <- Queue$new(repo, queue_id = queue_id)
worker_manager <- start_queue_workers_quietly(
1, queue$controller
)
make_worker_dirs(repo, worker_manager$id)

endpoint <- withr::with_envvar(
c(ORDERLY_RUNNER_QUEUE_ID = queue_id),
orderly_runner_endpoint("POST", "/report/run", repo)
)

req <- list(
name = scalar("data"),
branch = scalar(gert::git_branch(repo = repo)),
hash = scalar(gert::git_commit_id(repo = repo)),
parameters = scalar(NULL)
)

res <- endpoint$run(jsonlite::toJSON(req))
rrq::rrq_task_wait(res$data$job_id, controller = queue$controller)
expect_equal(
rrq::rrq_task_status(res$data$job_id, controller = queue$controller),
"COMPLETE"
)

req <- list(
name = scalar("parameters"),
branch = scalar(gert::git_branch(repo = repo)),
hash = scalar(gert::git_commit_id(repo = repo)),
parameters = list(a = scalar(1), c = scalar(3))
)

res <- endpoint$run(jsonlite::toJSON(req))
rrq::rrq_task_wait(res$data$job_id, controller = queue$controller)
expect_equal(
rrq::rrq_task_status(res$data$job_id, controller = queue$controller),
"COMPLETE"
)
})
Loading
Loading