pypsa-meets-earth / pypsa-earth

PyPSA-Earth: A flexible Python-based open optimisation model to study energy system futures around the world.
https://pypsa-earth.readthedocs.io/en/latest/
225 stars 177 forks source link

Test routine: add Makefile; CI: consolidate ci.yaml files #1053

Closed FabianHofmann closed 1 week ago

FabianHofmann commented 3 months ago

Closes #1052

Changes proposed in this Pull Request

This PR proposes four changes:

In general, the workflow would profit from using --configfile=<additional.config.yaml> instead of the diff_config approach. Was there a specific reason to stick to the manual insertion of config diffs? Perhaps is that historically grown? Note that --configfile overrides existing and adds additonal keys to the config given in the Snakefile. So it does essentially the same.

The CI will likely fail now, I need probably some more iterations.

Checklist

pz-max commented 3 months ago

I can comment here a bit:

Regarding this bit:

In general, the workflow would profit from using --configfile= instead of the diff_config approach. Was there a specific reason to stick to the manual insertion of config diffs? Perhaps is that historically grown? Note that --configfile overrides existing and adds additonal keys to the config given in the Snakefile. So it does essentially the same

I worked on the config diff stuff. Snakemake uses the exact same config merging code as we do (realised that later). I think @davide-f possibly added a few other parts. We implemented it like this to be independent of Snakemake and more flexible for this functionality (e.g. we are using ruaml (as Yaml container), or smt like that, which also keeps comments after the config is merged). Not sure what other benefits/ disadvantages exist. @davide-f might have more thoughts.

Feel free to decide what is the better solution for now :+1:

FabianHofmann commented 2 months ago

@pz-max @davide-f this is ready for being merged, objective values are reproduced. A merge today would be cool, so I can look into #796 soon

finozzifa commented 2 months ago

Indeed a great PR @FabianHofmann! thanks a lot

FabianHofmann commented 2 months ago

@davide-f @ekatef the PR is now ready. As discussed the config.default.yaml is ignored by now, and if no config.yaml is existent, the config.tutorial.yaml is copy to config.yaml

ekatef commented 2 months ago

@davide-f @ekatef the PR is now ready. As discussed the config.default.yaml is ignored by now, and if no config.yaml is existent, the config.tutorial.yaml is copy to config.yaml

Hey @FabianHofmann, thank you so much for the discussion and the adjustments! I love the solution with commenting-out the automated update using config.default.yaml. Absolutely agree that improvements are needed on management of the configuration files, and having this like in Snakemake should be an effective reminder! Could we probably add an issue to track the ideas and the progress on that, also accounting for the modularity idea @davide-f has suggested?

FabianHofmann commented 2 months ago

@ekatef that is a good idea! thank you for your comment. I will raise an issue

davide-f commented 2 months ago

Thanks @FabianHofmann ! Agree, let's keep it like this for now and edit after the merge :)

FabianHofmann commented 2 months ago

mmh, annoying, as soon as passing the --configfile via snakemake, the condition

if "config" not in globals() or not config:  # skip when used as sub-workflow

is false, so it will not load the config.yaml. It reverted the latest changes and have a look on monday, whether the if clause is really necessary for pypsa-earth-sec, or how we can adjust it.

davide-f commented 2 months ago

mmh, annoying, as soon as passing the --configfile via snakemake, the condition

if "config" not in globals() or not config:  # skip when used as sub-workflow

is false, so it will not load the config.yaml. It reverted the latest changes and have a look on monday, whether the if clause is really necessary for pypsa-earth-sec, or how we can adjust it.

Unfortunately it is necessary, I had to add that if clause exactly because of that unfortunately. Worth pointing @hazemakhalek to agree on a solution. If we merge this, we cannot update -sec to latest main here The merge will solve this issue...

davide-f commented 1 week ago

Hello @FabianHofmann , just to confirm, we can close this PR as it is merged already into the merge right?

FabianHofmann commented 1 week ago

Correct!