jidea-56 Metadata endpoint #5

EmmaLRussell · 2024-08-06T15:15:23Z

This branch adds the first cut of a metadata endpoint to allow the web app to get going with parameters metadata.

I was getting a bit bogged down with attempting to implement the versioning part of this, so have spun that out into a separate ticket (jidea-62) - but I've included a description of the intended approach in the README here. For now, we always read in a hardcoded metadata version (0.1.0), and this version is also returned in the response.

This branch implements:

metadata schema
/metadata endpoint which:
reads metadata from json file
injects country names from daedalus package - this will eventually include ISO ids from daedalus too, but for now country ids are just the names
injects hardcoded pathogen options - these will eventually come from daedalus too

I am very bad at R - please let me know anything that could have been done more sensibly in a different way!

To see the endpoint in action:

docker pull mrcide/daedalus.api:jidea-56
docker run --rm -d -p 8001:8001 --name dapi mrcide/daedalus.api:jidea-56
curl -s http://localhost:8001/metadata | jq

then docker kill dapi to tear down

EmmaLRussell · 2024-08-08T10:00:17Z

R/util.R

+system_file <- function(...) {
+  tryCatch({
+    system.file(..., mustWork = TRUE, package = "daedalus.api")
+  }, error = function(e) {
+    stop(sprintf("Failed to locate file from args\n%s",
+                 paste(list(...), collapse = " ")))
+  })
+}


Handy method stolen from hintr!

david-mears-2

1 comment and 1 question. Looks good as far as I can tell without my glaRses on!

david-mears-2 · 2024-08-08T12:24:52Z

inst/json/metadata_0.1.0.json

+      "defaultOption": "none",
+      "ordered": true,
+      "options": [
+        { "id": "none", "label": "None" },


This is more of a question than a comment, but it's possible in the future we'll need to annotate each such option with a description to explain what precisely is meant by e.g. 'Medium'. I presume it's OK to cross that bridge when we get to it.

Yeah, we can add a "description" field if required.

david-mears-2 · 2024-08-08T12:30:23Z

R/parameters.R

+# Allowed parameter ids which we can pass to the model
+expected_parameters <- c(
+  "country",
+  "pathogen",
+  "response",
+  "vaccine"
+)


Maybe a bit misleading to store data that is used only by a test, outside of the tests folder.

Yes, true - but I think we might expand on and make further user of this. For example, we might want to validate the parameters in run requests... (although maybe we just let the package do that?) Happy to move it into tests for now though.

I’ll let @pratikunterwegs opine.

I think it could live in tests for now and move to internal package data once the need becomes clear.

pratikunterwegs

Thanks @EmmaLRussell, looks good to me and I was able to follow the Readme instructions to pull and run the container. Just a few small suggestions here and there.

pratikunterwegs · 2024-08-08T12:56:50Z

R/util.R

+read_json <- function(filename) {
+  json <- jsonlite::fromJSON(system_file("json", filename))
+  json
+}


Could this be replaced by the {jsonlite} function of the same name, with simplifyVector = TRUE? Or alternatively, using jsonlite::fromJSON() directly? The wrapper being the same name as the {jsonlite} had me confused what it was doing (especially as the behaviour, e.g. simplifyVector is the opposite of what the {jsonlite} version does).

Ah good point. How about if I change the name to something not shared with jsonlite to make it clear this is a special for the API? read_local_json_file? I'm just thinking that neither fromJSON nor read_json are going to do exactly the same as their jsonlite equivalent since it's selecting the folder to read from.

read_local_json() sounds good!

pratikunterwegs · 2024-08-08T12:58:13Z

R/util.R

+system_file <- function(...) {
+  tryCatch({
+    system.file(..., mustWork = TRUE, package = "daedalus.api")
+  }, error = function(e) {
+    stop(sprintf("Failed to locate file from args\n%s",
+                 paste(list(...), collapse = " ")))
+  })
+}


I see why this function has been included, although it's over-writing a well known base function; the informative error is useful. Could we get a small comment explaining that?

pratikunterwegs · 2024-08-08T13:04:13Z

R/api.R

+  # JIDEA-62: we will read in correct metadata version according to
+  # model_version
+  metadata_file <- sprintf("metadata_%s.json", model_version)
+  response <- read_json(metadata_file)


I was initially confused as to where this function was coming from, but now I see it's redefined in this package; I've added a suggestion to switch to jsonlite::fromJSON() instead

pratikunterwegs · 2024-08-08T13:15:18Z

R/api.R

+metadata <- function() {
+  # JIDEA-62: we will use relevant model version - from qs if specified, else
+  # from latest available metadata file
+  model_version <- scalar("0.1.0")


Could this pull from the package version for now? The model structure and data versions are all bundled together in the package version at the moment anyway.

pratikunterwegs · 2024-08-08T13:25:58Z

tests/testthat/test-endpoints.R

+
+test_that("Can get metadata", {
+  endpoint <- daedalus_api_endpoint("GET", "/metadata")
+  res <- endpoint$run()


Could I suggest a no conditions expectation here:

Suggested change

res <- endpoint$run()

expect_no_condition(

res <- endpoint$run()

)

Good idea, and I've put it on the root call too..

Ha, that upsets the linter though: Avoid implicit assignments in function calls.
So have excluded those lines from linting!

Oh yes, a classic. The other linter complaints can usually be fixed with a styler::style_pkg() call

pratikunterwegs · 2024-08-08T13:27:54Z

tests/testthat/test-endpoints.R

+  lapply(res$data$parameters$id, function(id) {
+    expect_true(id %in% expected_parameters)
+  })


This could be simplified to an expect_identical(); if that proves too strict later it can always be changed to expect_setequal() or similar.

Suggested change

lapply(res$data$parameters$id, function(id) {

expect_true(id %in% expected_parameters)

})

expect_identical(

res$data$parameters$id,

expected_parameters

)

pratikunterwegs · 2024-08-08T13:34:17Z

tests/testthat/test-endpoints.R

+  lapply(seq_along(country_options), function(idx) {
+    daedalus_country <- scalar(daedalus_countries[[idx]])
+    expect_identical(country_options[[idx]]$id, daedalus_country)
+    expect_identical(country_options[[idx]]$label, daedalus_country)
+  })


Could I suggest sort of transposing this chunk so the outermost function always returns the expectations - no sweat really as this does what's needed. If you keep the original version, I don't think the scalar() call on daedalus_countries is needed.

Suggested change

lapply(seq_along(country_options), function(idx) {

daedalus_country <- scalar(daedalus_countries[[idx]])

expect_identical(country_options[[idx]]$id, daedalus_country)

expect_identical(country_options[[idx]]$label, daedalus_country)

})

expect_identical(

vapply(country_options, `[[`, FUN.VALUE = character(1), "id"),

daedalus_countries

)

expect_identical(

vapply(country_options, `[[`, FUN.VALUE = character(1), "label"),

daedalus_countries

)

I've gone for a version of this approach, but with syntax that I find easier to cope with! (more verbose though..)

# expect country ids to match those from daedalus expect_identical( vapply(country_options, function(option) { option$id }, character(1L)), daedalus_countries ) # expect country labels to match those from daedalus expect_identical( vapply(country_options, function(option) { option$label }, character(1L)), daedalus_countries )

david-mears-2 · 2024-08-08T17:52:06Z

Thank you, it’s awesome to have this in place!

pratikunterwegs

Thanks @EmmaLRussell looks good!

EmmaLRussell · 2024-08-09T08:53:26Z

Thanks both!

EmmaLRussell added 7 commits August 6, 2024 14:42

first attempt at schema and metadata files

3892fd7

working endpoint

c367bea

add modelVersion to response

57f9c3c

add countries from daedalus package to response

d50e5ff

Add harded pathogen options for now

7da9b95

use schema

60e4884

test

ca8cc41

EmmaLRussell commented Aug 8, 2024

View reviewed changes

EmmaLRussell added 3 commits August 8, 2024 11:00

lint

974e9c2

more linting

7fcc989

remove porcelain.R from lintr

3dc42b9

EmmaLRussell requested review from pratikunterwegs and david-mears-2 August 8, 2024 11:03

EmmaLRussell marked this pull request as ready for review August 8, 2024 11:03

david-mears-2 reviewed Aug 8, 2024

View reviewed changes

pratikunterwegs requested changes Aug 8, 2024

View reviewed changes

EmmaLRussell added 3 commits August 8, 2024 16:05

updates for PR review

bab11eb

lint

974dd4d

revert docker scripts

c3c6cf2

EmmaLRussell requested review from pratikunterwegs and david-mears-2 August 8, 2024 15:42

david-mears-2 approved these changes Aug 8, 2024

View reviewed changes

pratikunterwegs approved these changes Aug 9, 2024

View reviewed changes

EmmaLRussell merged commit b531d80 into main Aug 9, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jidea-56 Metadata endpoint #5

jidea-56 Metadata endpoint #5

EmmaLRussell commented Aug 6, 2024 •

edited

Loading

EmmaLRussell Aug 8, 2024

david-mears-2 left a comment

david-mears-2 Aug 8, 2024

EmmaLRussell Aug 8, 2024

david-mears-2 Aug 8, 2024

EmmaLRussell Aug 8, 2024

david-mears-2 Aug 8, 2024

pratikunterwegs Aug 9, 2024

pratikunterwegs left a comment

pratikunterwegs Aug 8, 2024

EmmaLRussell Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

EmmaLRussell Aug 8, 2024

EmmaLRussell Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

pratikunterwegs Aug 8, 2024

EmmaLRussell Aug 8, 2024

david-mears-2 commented Aug 8, 2024

pratikunterwegs left a comment

EmmaLRussell commented Aug 9, 2024

jidea-56 Metadata endpoint #5

jidea-56 Metadata endpoint #5

Conversation

EmmaLRussell commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

david-mears-2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratikunterwegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 commented Aug 8, 2024

pratikunterwegs left a comment

Choose a reason for hiding this comment

EmmaLRussell commented Aug 9, 2024

EmmaLRussell commented Aug 6, 2024 •

edited

Loading