-
Notifications
You must be signed in to change notification settings - Fork 361
Config Validation with Pydantic #1912
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
base: master
Are you sure you want to change the base?
Conversation
I went with option 1 (defining defaults in Pydantic), which means that |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, this is very rigorous! It will be super helpful to have some checking of configuration files for consistency! The only disadvantage I see is that developing new configuration settings now becomes a bit more complicated and less accessible. But I tend to agree that it is best to keep everything in one place.
-
I am glad all the comments disappear! We want all documentation in one place.
-
config.default.yamlandschema.jsonare autogenerated and should not be touched. This should be noted when writing the release note. -
Did you consider another location for the config validation code? Maybe just
./validation/config, thenscriptsis for the rules? Not sure, but the current structure looks quite deeply nested in subfolders. -
Thinking ahead, will the display also work in
mkdocswith a similar plugin? -
Sorry, I may have created more configuration settings by merging #1935.
-
Could you perhaps write up a few general principles of working with
pydanticwith links to resources in the documentation? -
Could you maybe summarise in one sentence (just for reference), why this is better than the standard validation in
snakemake? https://snakemake.readthedocs.io/en/stable/snakefiles/configuration.html#validation
| .. dropdown:: Details | ||
|
|
||
| .. jsonschema:: ../config/schema.json#/properties/version | ||
| :lift_description: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core information of the configuration page. I would not hide it in a dropdown, rather the other way around: the default config yaml could be in a dropdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've cleaned it up now, but still kept the detailed schema in a drop down menu. Otherwise, some configurations look just too bloated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @fneum that we should put the generated jsonschema directive first and without the dropdown. I think i would even re-include the defaults there. I think i would only keep it in the dropdown for the data section where it does not add any content.
For development, not really. You can still do whatever you want locally in your
I thought about it, but ended up with the current path:
Haven't tried it yet, but a way will be found. This will not be a blocker to deprecate
Using Snakemake for validation doesn't change much. Those are basically two questions: How you write the schema and how you validate it:
|
|
Thanks for the responses! That all sounds reasonable. |
proposal for using ruamel.yaml to bring back comments (not all top-level sections do have a doc heading, i think), in lkstrp#34 |
I think i would prefer to add language-server comments to the yaml files linking relatively to the schema. ie. add to This would be more self-contained and apply the right version automatically. This comment is interpreted as is by zed, in vscode one needs the redhat's vscode yaml extension. For using schemastore we would probably have to come up with a versioning scheme? |
|
Currently links in the description are markdown formatted and therefore don't parse correctly in the docs. In principle this is temporary, since we are switching to mkdocs eventually. I added a few commits to lkstrp#34 that convert those links to sphinx and then add a |
coroa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like it. i think mainly some guidance for soft-fork authors on how this should be extended is missing.
| .. dropdown:: Details | ||
|
|
||
| .. jsonschema:: ../config/schema.json#/properties/version | ||
| :lift_description: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @fneum that we should put the generated jsonschema directive first and without the dropdown. I think i would even re-include the defaults there. I think i would only keep it in the dropdown for the data section where it does not add any content.
| osm: _DataSourceConfig = Field( | ||
| default_factory=lambda: _DataSourceConfig(source="archive", version="latest"), | ||
| description="OSM data source configuration.", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the _DataSourceConfig defaults implicitly?
| osm: _DataSourceConfig = Field( | |
| default_factory=lambda: _DataSourceConfig(source="archive", version="latest"), | |
| description="OSM data source configuration.", | |
| ) | |
| osm: _DataSourceConfig = Field( | |
| default_factory=_DataSourceConfig, | |
| description="OSM data source configuration.", | |
| ) |
Supersedes #1547
Closes #1547
Closes #1734
Closes #1721
Changes proposed in this Pull Request
This PR revives and supersedes #1547. This time using pydantic which can export to JSON Schema instead of writing the JSON Schema directly.
With the new approach we can do a couple of new things and keep anything config related at one place:
bool,list[str]), Literals (e.g. for logging levelsLiteral["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"], as well as any custom validation you can come up with. This PR tries to not break anything, which means only types, andLiteralsfrom the previous doctabledescription/valuescolumn have been taken over. In future we wanna get as strict as possible.config.default.yamlandschema.json. The config can be used locally as is, but must be kept in sync upstream with the validation config. Since the file is auto generated now, we also lose all comments.snakemake.config["run"]andsnakemake.config.run.dash-casein the config and move tosnake_case. In the export, currently both is supported via aliases, but python attributes do not allow fordash-case. We could also duck type this, but it's cleaner to just remove them.schemastore.org(what we can do after this PR is merged), most IDEs will automatically give you inline tooltips on config files. It is also possible to set it up manually.Other changes
requirements.txtfor docs, pins docs dependencies in pixiChecklist
Post Merge
**/pypsa-eur/config/*.yamland**/pypsa-eur*/config/*.yamldash-casetocamel_casefor all config keys and add typed object