hubverse-org / hubAdmin

Utilities for administering hubverse Infectious Disease Modeling Hubs
https://hubverse-org.github.io/hubAdmin/
Other
1 stars 2 forks source link

Validate hub config - do not catch wrong `compound taskid_set` if additional properties error #21

Closed LucieContamin closed 4 months ago

LucieContamin commented 5 months ago

Following my testing on https://github.com/LucieContamin/hub_test, I noticed another issue. I don't think it's necessary to fix it now, especially if we allow additional properties, but I thought it might be worth noting.

If I make a typo in the "compound_taskid_set" in the tasks.json file, I did not get any error message if I also have an error for additional properties. However, if I remove the additional properties, I have the compound_taskid_set value(s) 'scenario' not valid task id(s). error.

For example:

          "task_ids": {
            "origin_date": {
              "required": ["2024-04-28"],
              "optional": null
            },
             "scenario_id": {
              "required": ["A-2024-03-01", "B-2024-03-01", "C-2024-03-01", "D-2024-03-01", "E-2024-03-01", "F-2024-03-01"],
              "optional": null
            },
            "location": {
              "required": null,
              "optional": ["US", "01", "02", "04", "05", "06", "08", "09", "10", "11", "12", "13", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "44", "45", "46", "47", "48", "49", "50", "51", "53", "54", "55", "56", "60", "66", "69", "72", "74", "78"]
            },
            "target": {
              "required": ["inc death", "inc hosp"],
              "optional": ["cum death", "cum hosp"]
            },
            "horizon": {
              "required": [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52],
              "optional": null
            },
            "age_group": {
              "required": ["0-130", "65-130", "0-64"],
              "optional": null
            }
          },
          "output_type": {
            "sample": {
              "output_type_id_params": {
                "is_required": true,
                "type": "integer",
                "min_samples_per_task": 100,
                "max_samples_per_task": 300,
                "compound_taskid_set": ["origin_date", "scenario", "location", "target"]
              },
              "value" : {
                "type": "double",
                "minimum": 0
              }
            }
annakrystalli commented 5 months ago

Thanks for reporting @LucieContamin . That's by design. First the config needs to pass jsonvalidate validation against the schema (which it doesn't pass currently with additional properties) and then the more dynamic checks performed in R are run. It would be an execution error nightmare to try and develop and run dynamic validations on configs that we hadn't at least confirmed the basic structure was dependable.

Perhaps a note in the docs about this would be useful though?

LucieContamin commented 5 months ago

It makes total sense, thanks for the reply! Maybe a note in the docs is a good idea.