refactor: schema validation

[csadorf]

No description provided.

[csadorf]

@vyasr This is ready for review.

[vyasr]

Thanks Simon! This is a definite improvement. The use of references is very helpful. I do worry a little bit about some of the reference names being too generic and overlapping (for instance, Requirement vs Requirements given that they're used in different package lists, or that Matrices is not an array of Matrix). It might be better to use longer, more precise names at the expense of brevity. I'm also OK with waiting to rename until we put the PR into the main branch up for review, though, so that we can get a fresh set of eyes on it.

[vyasr]

Just FYI, due to the use of semantic-release in maintaining this package, I think the merge of this PR will immediately make this link out of date since the sha won't be v1.0.0 but instead v1.1.0 (assuming we treat this PR as a new feature). We'll need to update this URL accordingly when we merge my PR into the main branch.

[csadorf]

Could we automatically update this URL to match the release version?

[vyasr]

@ajschmidt8 you have more familiarity with what we can do with semantic-release, do you have any recommendations on how we could handle this? Specifically, what we want is for the version number in the "$id" URL to be updated to match the current version during a release. My concern was that the PR to update the version number would itself trigger a new version, thus immediately rendering the version in the URL out of date.

[ajschmidt8]

@vyasr, @csadorf, yes. we can do that with https://github.com/semantic-release/git.

We'll have to figure out how to configure it, but it will commit "release assets" back to the repository (with a [skip ci] tag) after a release has occurred.

In this case, schema.json would be categorized as a "release asset" since it depends on the release version.

[vyasr]

@csadorf I have one other request with this PR. Could you modify some of the test dependencies.yaml files to invalidate them and show some examples of the output? I am not sure how the refactoring of the schema into components affects the generated error output, and I want to make sure that it's still readable and user-friendly for RAPIDS devs who need to modify dependencies.yaml files without having any knowledge of jsonschema. It's fine for developers of dfg to need to know how this works, but I wouldn't want to reduce the readability of the error output for others. I assume this isn't an issue (or if it is, it's one we can mitigate), just want to confirm.

[csadorf]

@csadorf I have one other request with this PR. Could you modify some of the test dependencies.yaml files to invalidate them and show some examples of the output? I am not sure how the refactoring of the schema into components affects the generated error output, and I want to make sure that it's still readable and user-friendly for RAPIDS devs who need to modify dependencies.yaml files without having any knowledge of jsonschema. It's fine for developers of dfg to need to know how this works, but I wouldn't want to reduce the readability of the error output for others. I assume this isn't an issue (or if it is, it's one we can mitigate), just want to confirm.

I ran some tests and here is an example for the new error message:

jsonschema.exceptions.ValidationError: 'abc' is not of type 'object'

Failed validating 'type' in schema['properties']['dependencies']['patternProperties']['.*']['properties']['specific']['items']['properties']['matrices']['items']:
    {'additionalProperties': False,
     'properties': {'matrix': {'oneOf': [{'patternProperties': {'.*': {'type': 'string'}},
                                          'type': 'object'},
                                         {'type': 'null'}]},
                    'packages': {'oneOf': [{'$ref': '#/$defs/requirements'},
                                           {'type': 'null'}]}},
     'requiredProperties': ['matrix', 'packages'],
     'type': 'object'}

On instance['dependencies']['build']['specific'][0]['matrices'][0]:
    'abc'

And the way it would look like previously:

jsonschema.exceptions.ValidationError: 'abc' is not of type 'object'

Failed validating 'type' in schema['properties']['dependencies']['patternProperties']['.*']['properties']['specific']['items']['properties']['matrices']['items']:
    {'properties': {'matrix': {'type': ['null', 'object']},
                    'packages': {'if': {'type': 'array'},
                                 'then': {'if': {'type': 'object'},
                                          'items': {'type': ['string',
                                                             'object']},
                                          'then': {'patternProperties': {'.*': {'if': {'type': 'array'},
                                                                                'then': {'items': {'type': 'string'}},
                                                                                'type': ['array',
                                                                                         'string']}}}},
                                 'type': ['null', 'array', 'string']}},
     'type': 'object'}

On instance['dependencies']['build']['specific'][0]['matrices'][0]:
    'abc'

The header message is the same and while the context is maybe slightly more obfuscated, I think it is actually more readable and it is easier to understand the context of the error.

[vyasr]

I agree, in this case it seems more readable. If there error was one level deeper in the packages key, would the {'$ref': '#/$defs/requirements'} be expanded into the definition?

[csadorf]

I agree, in this case it seems more readable. If there error was one level deeper in the packages key, would the {'$ref': '#/$defs/requirements'} be expanded into the definition?

I replaced one of the dependencies with a number (should be a string) and it is not:

jsonschema.exceptions.ValidationError: [1234, 'cuda-python>=11.5,<11.7.1'] is not valid under any of the given schemas

Failed validating 'oneOf' in schema['properties']['dependencies']['patternProperties']['.*']['properties']['specific']['items']['properties']['matrices']['items']['properties']['packages']:
    {'oneOf': [{'$ref': '#/$defs/requirements'}, {'type': 'null'}]}

On instance['dependencies']['build']['specific'][0]['matrices'][0]['packages']:
    [1234, 'cuda-python>=11.5,<11.7.1']

But I am not convinced that simply following all $refs makes the error message inherently more readable. If we are concerned about providing highly intuitive error feedback, I would instead suggest to actually use jsonschema's tooling to identify the most relevant error. In this example using

 validator = jsonschema.Draft7Validator(SCHEMA)
 print(best_match(validator.iter_errors(dependencies)))

will yield

1234 is not of type 'string'

Failed validating 'type' in schema[0]['items']:
    {'type': 'string'}

On instance[0]:
    1234

The best_match function actually accepts a relevance key so we could even tune this to our needs.

[csadorf]

@vyasr This should be ready for another review iteration.

[vyasr]

I wasn't aware of best_match, that is helpful. I'm happy with that result.

[vyasr]

These changes look great! I'm going to go ahead and merge this now, and we can handle any follow-ups in the PR merging my branch into the main repo.