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

[TEP006] Create a JSON schema in YAML to replace the old tardis_config_definition.yml #549

Merged
merged 9 commits into from
Jun 2, 2016

Conversation

ftsamis
Copy link
Member

@ftsamis ftsamis commented Apr 26, 2016

This is a conversion of the configuration definition for the convergence_strategy property to JSON Schema to demonstrate the ability of JSON Schema to cover our needs without changing our configuration file syntax.

The convergence_strategy property was chosen intentionally as a starting point because:

  1. it is of type container-property which I consider to be the most complicated of TARDIS' types to express in JSON Schema, and
  2. it contains no Quantity sub-properties and therefore it's easier to demonstrate its validation, even with an online tool such as http://jsonschemalint.com/draft4/ (accepts YAML).

For example, you could try the input below taken from http://tardis.readthedocs.org/en/latest/examples/profilemodel.html in the aforementioned site against the schema in this commit.

convergence_strategy:
  type: specific
  damping_constant: 1.0
  threshold: 0.05
  fraction: 0.8
  hold_iterations: 3
  t_inner:
    damping_constant: 1.0

Of course, try to change any value to something invalid, or remove a required sub-property.

@ftsamis
Copy link
Member Author

ftsamis commented May 7, 2016

Added a conversion to JSON Schema of black_body_sampling, a property of type quantity_range_sampled, for demonstration purposes also.

@wkerzendorf wkerzendorf changed the title Create a JSON schema in YAML to replace the old tardis_config_definition.yml [WIP] Create a JSON schema in YAML to replace the old tardis_config_definition.yml May 9, 2016

# A container property expressed in JSON Schema
# Property: convergence_strategy, transcoded from tardis_config_definition.yml
type: object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does type: object mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

object is basically how javascript/json names dictionaries. For reference, the JSON → Python type analogs are as follows:

JSON type Python type
integer int
number float
string unicode
array list
object dict
boolean bool

@ftsamis
Copy link
Member Author

ftsamis commented May 16, 2016

Added the complete JSON schema for the configuration, as transcoded from tardis_config_definition.yml by a custom script I wrote. The new schema is ~100 lines longer than the old one (~400 → ~500) and validating it with jsonschema.Draft4Validator shows some different behaviour which needs some discussion:

  1. Integers are lexically identified by python jsonschema instead of by value, therefore 5.0 or 10e2 are not considered integers by jsonschema.

    Possible solutions

    1. Require that integers are provided in the configuration without decimal points or exponential notation. This is probably the worst option since it would break compatibility.
    2. Specify integers as numbers (float) in the JSON schema with the extra constraint of multipleOf: 1. This would allow integers with any representation.
    3. Change jsonschema.Draft4Validator's behaviour to validate integers by value. I haven't explored the feasibility of this option yet.
  2. By default, the jsonschema validator can accept additional properties, not defined in the schema. This includes anything from arbitrary "sections" to arbitrary properties in existing "sections".

    Possible solutions

    1. Specify the additionalProperties: False keyword in the schema of each and every object (dict) containing properties.
    2. Subclass jsonschema.Draft4Validator and default to not accepting additional properties. This could possibly restrict us in a future case where we would want a specific section to accept additional properties, not defined in the schema, but I think this is unlikely to appear as a need.
  3. If a property is marked as required (the equivalent of our current mandatory), it is indeed required by the validator, but the parent property containing it is not (i.e. the required constraint doesn't propagate upwards).

    Example: plasma.ionization is a required property but plasma itself would not be considered as required by the default jsonschema validator behaviour and therefore could be omitted.

    Possible approaches

    1. Leave it as is. (it's an approach...)
    2. Explicitly specify sections of each required property as required too.
    3. Change jsonschema.Draft4Validator's behaviour to consider sections with required properties as required too. I haven't explored the feasibility of this option yet.

@ftsamis
Copy link
Member Author

ftsamis commented May 20, 2016

I've now split the schema to multiple files, one for each top-level section, with base.yml being the main schema. This is really easier to look at, and should be easier to modify/extend as well.

With some customizations to jsonschema, I've also managed to validate against this new multi-file structure, so it should be OK. More specifically, jsonschema by default will try to treat external (to the base schema) files as json files, instead of yaml, and therefore a custom RefResolver with a file handler function needs to be specified in the Draft4Validator. This RefResolver has to also take care of the relative paths.

I'll present a full-fledged validator for this structure in another PR.

Also, I moved the schemas to tardis/io/schemas/. This may be not wanted, and if it is it would probably require specifying it in the setup.py script.

@wkerzendorf Thoughts?

@yeganer
Copy link
Contributor

yeganer commented Jun 1, 2016

Why is this PR abandoned in favor of #576 ?

@wkerzendorf
Copy link
Member

@yeganer - because I asked.

@wkerzendorf wkerzendorf merged commit 1a1520b into tardis-sn:master Jun 2, 2016
@ftsamis ftsamis changed the title [WIP] Create a JSON schema in YAML to replace the old tardis_config_definition.yml [TEP006] Create a JSON schema in YAML to replace the old tardis_config_definition.yml Jul 19, 2016
@ftsamis ftsamis deleted the new-schema branch December 20, 2016 15:30
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

Successfully merging this pull request may close these issues.

3 participants