Skip to content

Commit

Permalink
now error out on invalid config files
Browse files Browse the repository at this point in the history
  • Loading branch information
natemcintosh committed Nov 29, 2024
1 parent d8049a7 commit 5c3cee2
Show file tree
Hide file tree
Showing 8 changed files with 95 additions and 51 deletions.
21 changes: 17 additions & 4 deletions R/config.R
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,15 @@ Config <- S7::new_class(
#'
#' @param config_path A string specifying the path to the JSON configuration
#' file.
#' @param optional_fields A list of strings specifying the optional fields in
#' the JSON file. If a field is not present in the JSON file, and is marked as
#' optional, it will be set to either the empty type (e.g. `chr(0)`), or NULL.
#' If a field is not present in the JSON file, and is not marked as optional, an
#' error will be thrown.
#' @return An instance of the `Config` class populated with the data from the
#' JSON file.
#' @export
read_json_into_config <- function(config_path) {
read_json_into_config <- function(config_path, optional_fields) {
# First, our hard coded, flattened, map from strings to Classes. If any new
# subclasses are added above, they will also need to be added here. If we
# create a more automated way to do this, we can remove this.
Expand All @@ -211,11 +216,19 @@ read_json_into_config <- function(config_path) {

# Check what top level properties were not in the raw input
missing_properties <- setdiff(S7::prop_names(Config()), names(raw_input))
# Remove any optional fields from the missing properties, give info message
# about what is being given a default arg.
not_need_but_missing <- intersect(optional_fields, missing_properties)
if (length(not_need_but_missing) > 0) {
cli::cli_alert_info(
"Optional field{?s} not in config file: {.var {not_need_but_missing}}"
)
}
missing_properties <- setdiff(missing_properties, optional_fields)
# Error out if missing any fields
if (length(missing_properties) > 0) {
cli::cli_alert_info(c(
"The following expected propert{?y/ies} were not in the config file:",
"{.var {missing_properties}}"
cli::cli_abort(c(
"Propert{?y/ies} not in the config file: {.var {missing_properties}}"
))
}

Expand Down
2 changes: 1 addition & 1 deletion R/pipeline.R
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ orchestrate_pipeline <- function(config_path,
blob_storage_container = NULL,
output_dir = "/") {
config <- rlang::try_fetch(
read_json_into_config(config_path),
read_json_into_config(config_path, c("exclusions")),
error = function(con) {
cli::cli_warn("Bad config file",
parent = con,
Expand Down
80 changes: 40 additions & 40 deletions man/Config.Rd

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

8 changes: 7 additions & 1 deletion man/read_json_into_config.Rd

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

3 changes: 2 additions & 1 deletion tests/testthat/data/sample_config_no_exclusion.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,6 @@
"iter_sampling": 50,
"adapt_delta": 0.99,
"max_treedepth": 12
}
},
"config_version": "0.1.0"
}
3 changes: 2 additions & 1 deletion tests/testthat/data/sample_config_with_exclusion.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,6 @@
"iter_sampling": 50,
"adapt_delta": 0.99,
"max_treedepth": 12
}
},
"config_version": "0.1.0"
}
4 changes: 4 additions & 0 deletions tests/testthat/data/v_bad_config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"job_id": "6183da58-89bc-455f-8562-4f607257a876",
"task_id": "bc0c3eb3-7158-4631-a2a9-86b97357f97e"
}
25 changes: 22 additions & 3 deletions tests/testthat/test-pipeline.R
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ test_that("Bad config throws warning and returns failure", {
blob_storage_container = blob_storage_container,
output_dir = output_dir
),
class = "Run_failed"
class = "Bad_config"
)
expect_false(pipeline_success)
})
Expand Down Expand Up @@ -71,7 +71,7 @@ test_that("Pipeline run produces expected outputs with exclusions", {
test_that("Process pipeline produces expected outputs and returns success", {
# Arrange
config_path <- test_path("data", "sample_config_with_exclusion.json")
config <- read_json_into_config(config_path)
config <- read_json_into_config(config_path, c("exclusions"))
# Read from locally
output_dir <- "pipeline_test"
on.exit(unlink(output_dir, recursive = TRUE))
Expand All @@ -95,7 +95,7 @@ test_that("Process pipeline produces expected outputs and returns success", {
test_that("Runs on config from generator as of 2024-11-26", {
# Arrange
config_path <- test_path("data", "CA_COVID-19.json")
config <- read_json_into_config(config_path)
config <- read_json_into_config(config_path, c("exclusions"))
# Read from locally
output_dir <- "pipeline_test"
on.exit(unlink(output_dir, recursive = TRUE))
Expand All @@ -115,3 +115,22 @@ test_that("Runs on config from generator as of 2024-11-26", {
config@task_id
)
})

test_that("Warning and exit for bad config file", {
# Arrange
config_path <- test_path("data", "v_bad_config.json")
# Read from locally
output_dir <- "pipeline_test"
on.exit(unlink(output_dir, recursive = TRUE))

# Act
expect_warning(
pipeline_success <- orchestrate_pipeline(
config_path = config_path,
blob_storage_container = NULL,
output_dir = output_dir
),
class = "Bad_config"
)
expect_false(pipeline_success)
})

0 comments on commit 5c3cee2

Please sign in to comment.