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

deprecate, rename and check config entries #1514

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

FabianHofmann
Copy link
Contributor

@FabianHofmann FabianHofmann commented Jan 26, 2025

Add scheme for deprecating config entries and warn for config validation

This PR introduces specific warning classes for deprecated and invalid configuration entries. A scheme is proposed to deprecate and optionally rename config entries through the config/deprecations.yaml.

Like this snakemake raises:

  • a DeprecationConfigWarning for deprecated config entries
  • a InvalidConfigWarning for unsupported/invalid config entries which are not included in config.default.yaml

Testing

Added assertions to verify warning classes in:

  • test_config_deprecations()
  • test_config_invalid_entries()

Open points

  • extend this to capture scenario config.

Checklist

  • I tested my contribution locally and it works as intended.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to envs/environment.yaml.
  • Changes in configuration options are added in config/config.default.yaml.
  • Changes in configuration options are documented in doc/configtables/*.csv.
  • Sources of newly added data are documented in doc/data_sources.rst.
  • A release note doc/release_notes.rst is added.

add check for validity of config entries
@FabianHofmann FabianHofmann requested review from fneum and lkstrp January 26, 2025 10:53
@FabianHofmann FabianHofmann changed the title add scheme to deprecate/rename config entries deprecate, rename and check config entries Jan 26, 2025
Copy link
Contributor

github-actions bot commented Jan 26, 2025

Validator Report

I am the Validator. Download all artifacts here.
I'll be back and edit this comment for each new commit.

❗ Run failed!

Download 'logs' artifact to see more details.

  • master failed in: no_logs_found
  • config-deprecations failed in: no_logs_found

Model Metrics

Benchmarks Image not available Image not available Image not available

Comparing config-deprecations (5bbbecb) with master (590b684).
Branch is 6 commits ahead and 0 commits behind.
Last updated on 2025-02-04 17:17:01 CET.

@fneum
Copy link
Member

fneum commented Jan 27, 2025

This is cool!

Copy link
Member

@lkstrp lkstrp left a comment

Choose a reason for hiding this comment

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

While I strongly support any config validation, I am not sure if we wanna implement custom functionality for that.

If this is only about Deprecations, it's fine. But when checking for invalid configs, specially if we add more advanced checks, this will be much easier and simpler with some data validation library.

There are json schema validation libraries out there, but if I point to similar validations in PyPSA and the data validation layers discussed on Friday, which would be really useful in the mid term within pypsa-eur, maybe we can go for Pydantic for config checks as well.

I can draft something how this could look for json data. But I think it is better than introducing another custom solution which will be much harder to extend

@fneum
Copy link
Member

fneum commented Jan 27, 2025

I think it’s for deprecation. For validation of cong filers there is already built in functionality in snakemake: https://snakemake.readthedocs.io/en/stable/snakefiles/configuration.html#validation

@lkstrp
Copy link
Member

lkstrp commented Jan 28, 2025

Great, then json schema is already there. But this is even better, we don’t need any extra imports and have everything:

  • InvalidConfigWarning: Value validation fails
  • DeprecationConfigWarning: Key is not in schema

Json schema allows for basic value checks which should be enough for config validation.
And also has a deprecation property, which raises those warnings. This needs to be checked with snakemake, but looks like its build on top and should handle that all.

@FabianHofmann
Copy link
Contributor Author

Gotcha, so perhaps the naming is a bit misleading, the implementation only checks whether a key provided by the user is deprecated or not supported by the default.yaml. That said I am totally open to generalize this and use a more solid json schema from snakemake which might even check values. I am not sure though, how well these can be incorporated with our scnenario configs, but there is definitely a way. I will take a look. Also the question arises whether it is worth to define the scheme now, before we are doing some potential restructuring changes that might also touch the config ( in the preparation of which I made this PR).

@fneum
Copy link
Member

fneum commented Feb 4, 2025

@FabianHofmann, yes I see this as two independent steps. Key deprecation can go ahead.

@FabianHofmann
Copy link
Contributor Author

okay, given the considerations of @lkstrp I would even consider that we leave this PR and make the whole thing via the jsonschema. there my questions would only be

  • can the jsonschema indicate deprecations and custom messages for those?
  • can we set the condition that all configurations used (also via the scenario management) must be in the jsonschema and a warning is returned if not

@lkstrp since know more about this, any clue if this is supported? from your answer above it seems so

@lkstrp
Copy link
Member

lkstrp commented Feb 4, 2025

@FabianHofmann
Yes, I would rather not merge this one.

I checked it quickly, but I think on both the answer is yes. We need to check if snakemake handles all json-schema features. For scenario management I am not sure though

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