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

More helpful error message when schema validation fails #160

Open
jbusecke opened this issue Dec 11, 2023 · 2 comments
Open

More helpful error message when schema validation fails #160

jbusecke opened this issue Dec 11, 2023 · 2 comments

Comments

@jbusecke
Copy link
Contributor

Over in leap-stc/data-management#75 i encountered a rather cryptic error:

***"message": "Error during running: ", "exc_info": "Traceback (most recent call last):\n  File \"/usr/local/bin/pangeo-forge-runner\", line 8, in <module>\n    sys.exit(main())\n  File \"/usr/local/lib/python3.10/dist-packages/pangeo_forge_runner/cli.py\", line 28, in main\n    app.start()\n  File \"/usr/local/lib/python3.10/dist-packages/pangeo_forge_runner/cli.py\", line 23, in start\n    super().start()\n  File \"/usr/local/lib/python3.10/dist-packages/traitlets/config/application.py\", line 475, in start\n    return self.subapp.start()\n  File \"/usr/local/lib/python3.10/dist-packages/pangeo_forge_runner/commands/bake.py\", line 229, in start\n    recipes = feedstock.parse_recipes()\n  File \"/usr/local/lib/python3.10/dist-packages/pangeo_forge_runner/feedstock.py\", line 80, in parse_recipes\n    assert any(\nAssertionError", "status": "failed"***

It seemed related to the new schema validation but I had to ask @cisaacstern to get to the bottom of the problem.

Could we raise a more informative error message like:

Your recipe_id:`my_recipe` contains fields ['not_this', 'worse'] which are not allowed, only allowed values are ['def_this', 'yay']
@cisaacstern
Copy link
Member

Agreed that the error message should be more descriptive. This error is being raised in feedstock.py, and taking a closer look at this, I now feel I disagree with this comment that I left there:

# MetaYaml schema validation of self.meta ensures that one of these two
# conditions is true, but just assert anyway, to make sure there are no
# edge cases that slip through the cracks.

This double-validation feels like a code smell to me. The validation should just happen in the MetaYaml schema, and if that validation is leaky, we should fix it there, not double-check it somewhere else (as we are now doing, which is my fault).

So the place this should be fixed is here:

if isinstance(proposal["value"], list):
for recipe_spec in proposal["value"]:
try:
jsonschema.validate(recipe_spec, recipes_field_per_element_schema)
except jsonschema.ValidationError as e:
raise TraitError(e)

Interestingly, this block did not raise an error (i.e. the validation here is currently leaky), which suggests to me that the issue is that we are allowing extra fields in that validation block, which I think we should not be doing.

@yuvipanda
Copy link
Collaborator

This sounds like the assert caught an issue with the schema validation actually being leaky, so it can be fixed there. Sounds like a win + positive result of the comment you left + assert there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants