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

double check setup for target_metadata.target_keys #97

Open
elray1 opened this issue Aug 28, 2024 · 1 comment · Fixed by #108
Open

double check setup for target_metadata.target_keys #97

elray1 opened this issue Aug 28, 2024 · 1 comment · Fixed by #108
Assignees

Comments

@elray1
Copy link
Contributor

elray1 commented Aug 28, 2024

I believe the specification here is incorrect because an array of values was provided rather than a single value, but this config file is passing validations. Following my understanding and the documentation examples, I believe the correct specification should be along the lines of:

                            "target_keys": {
                                "target": "wk inc flu hosp"
                            },

I've filed this as an issue on schemas rather than hubAdmin because I believe this could probably be fixed by being more specific in the schema for this field? Currently, we just say type: ["object", null], here, but there might be a way to say that the values of the object should be strings?

@annakrystalli
Copy link
Member

That's indeed a bug and is related also to issue reported by @matthewcornell in cdcepi/FluSight-forecast-hub#1094 (comment)

Initially we left it vague because we don't know a priory the names of properties a target_keys object might contain and I wasn't sure of a way to specify the schema in such a situation.

It's further complicated by the fact that the array collapses to a vector when the config is read in to R so everything works in R.

Having thought about it more though, this is exactly the correct use for additionalProperties!!

See initial experiments which work!

text_invalid <- '{
    "target_keys": {
        "target": [
            "wk flu hosp rate change"
        ]
    }
}'
text_valid <- '{
    "target_keys": {
        "target": "wk flu hosp rate change"
    }
}'
text_valid_null <- '{
    "target_keys": null
}'

schema_text_vague <- '
{
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "$id": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.1/tasks-schema.json",
    "type": "object",
    "properties": {
        "target_keys": {
            "type": [
                "object",
                "null"
                ]

        }

    }
}
'
schema_vague <- jsonvalidate::json_schema$new(schema_text_vague)
schema_vague$validate(text_invalid) # FALSE NEGATIVE
#> [1] TRUE
schema_vague$validate(text_valid)
#> [1] TRUE
schema_vague$validate(text_valid_null)
#> [1] TRUE


schema_text_specific <- '
{
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "$id": "https://raw.githubusercontent.com/hubverse-org/schemas/main/v3.0.1/tasks-schema.json",
    "title": "Schema for Modeling Hub model task definitions",
    "description": "This is the schema of the tasks.json configuration file that defines the tasks within a modeling hub.",
    "type": "object",
    "properties": {
        "target_keys": {
            "type": [
                "object",
                "null"
                ],
            "additionalProperties": {
                "type": "string"

            }
        }
    }
}
'

schema <- jsonvalidate::json_schema$new(schema_text_specific)
schema$validate(text_invalid)
#> [1] FALSE
schema$validate(text_valid)
#> [1] TRUE
schema$validate(text_valid_null)
#> [1] TRUE

Created on 2024-10-16 with reprex v2.1.0

I'll implement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Reviewed/Ready to Merge
Development

Successfully merging a pull request may close this issue.

2 participants