Feature attrdict and model definition declutter #719

irm-codebase · 2024-11-28T19:19:02Z

Prepares our IO structure for pydantic by moving as much YAML read features as possible out of AttrDict.

Summary of changes in this pull request

YAML reading functionality has been moved out of AttrDict
templates: improvements
- now resolved before model definition reaches calliope.Model
- template: can be used anywhere within a YAML file, not just nodes, techs or data_tables
- cyclic template inheritance errors (template1 -> template2 -> template1 -> template2...) are now detected by using a stack
removed inheritance helper function. It was made redundant by the template solving.
Grouped scenario, override_dict and templates into preprocess/model_definition.py to ensure a single point of entry for defining their order of priority.

Reviewer checklist

Test(s) added to cover contribution
Documentation updated
Changelog updated
Coverage maintained or improved

… YAML / inheritance functions

irm-codebase · 2024-11-29T19:26:50Z

@brynpickering tests run well on my side, with no visible impact on performance.

irm-codebase · 2024-12-02T10:09:33Z

Some additional context @brynpickering:
One of the main ideas behind this PR is to make clear where the configuration object is delivered to calliope, so that the pydantic model that #704 (and #717) use has a single place of delivery. This would be the output of the new preprocess/model_definition.py file.

irm-codebase · 2024-12-02T14:36:19Z

Documentation has been updated. The YAML section now also explains templating (since it's now a generic feature).
I've also separated the section into 'standard YAML' and our 'rich YAML', for clarity.

…entation

codecov · 2024-12-02T16:15:23Z

Codecov Report

Attention: Patch coverage is 99.37888% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.09%. Comparing base (c300f2e) to head (8d58894).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/calliope/util/generate_runs.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
+ Coverage   95.98%   96.09%   +0.10%     
==========================================
  Files          29       29              
  Lines        4060     4067       +7     
  Branches      580      584       +4     
==========================================
+ Hits         3897     3908      +11     
+ Misses         72       68       -4     
  Partials       91       91

Files with missing lines	Coverage Δ
src/calliope/attrdict.py	`97.63% <100.00%> (+1.15%)`	⬆️
src/calliope/backend/backend_model.py	`97.98% <100.00%> (ø)`
src/calliope/backend/helper_functions.py	`96.72% <ø> (-0.26%)`	⬇️
src/calliope/cli.py	`81.57% <100.00%> (ø)`
src/calliope/io.py	`98.08% <100.00%> (+1.28%)`	⬆️
src/calliope/model.py	`95.94% <100.00%> (-0.06%)`	⬇️
src/calliope/preprocess/__init__.py	`100.00% <100.00%> (ø)`
src/calliope/preprocess/data_tables.py	`100.00% <100.00%> (ø)`
src/calliope/preprocess/model_data.py	`100.00% <100.00%> (ø)`
src/calliope/preprocess/model_definition.py	`95.23% <100.00%> (ø)`
... and 4 more

brynpickering

Nice. I'm on board with this. We're slowly but surely extracting content from AttrDict and will eventually be able to remove it entirely! We'll need to work out why this change has impacted the national-scale example model, though. Something about the order of dict unions is likely messing up the national-scale model.

docs/creating/yaml.md

brynpickering · 2024-12-04T08:38:37Z

docs/creating/yaml.md

@@ -125,9 +206,160 @@ scenarios:
    * The imported files may include further files, so arbitrary degrees of nested configurations are possible.
    * The `import` statement can either give an absolute path or a path relative to the importing file.

-## Overriding one file with another
+### Reusing definitions through templates


should we make it clear that templates can only be used in nodes, techs, and data_tables (at present)?

not needed, they can be used anywhere now
The code in nodes, techs and data_tables that handled this was removed in this PR.

ok, interesting. so you could technically have:

templates: my_template: tech1: ... tech2: ... techs: template: my_template

?

I suppose it's fine that it can cover that edge case, even if it wouldn't be realistically used. I see now why you were concerned about the order of overrides/templates. I.e., this can't work:

templates: my_template: techs: tech1: ... tech2: ... overrides: override1: template: my_template

Because the overrides are resolved before templates are?

Correct. Your first example would work well.

I saw templates as purely 'extended' yaml. As in, they would work with any YAML file, and the templates code would live inside io.py.

You are also correct in that using templates within the override would introduce issues. My code currently does not cover for it. This is the kind of edge-case that is avoided by just solving it before overrides.

So, two choices:

Do we keep the current behaviour, and add code for the edge-case you described?

Do we move templates to the io.py function (meaning that read_rich_yaml always solves them)?

I prefer the second even if we lose some templating power because it is more robust overall, code-wise.

In terms of power of the functionality for the user, the first is the best option (since you can update just a template in an override and have it propagate throughout the rest of the model). Let's leave it like that and if we get lots of complaints of weird behaviour, we can revisit. It does require us to protect "template" as a key and not allow it as a subkey of override or scenario.

Correct. I will add a function that tests for that then.
It is important because after this goes in, schemas (pydantic) and so on must check the data structure of the dictionary returned by model_definition, so this edge case will be missed.

After going through this edge-case, it seems like a non-issue. The templates algorithm will just see the 'new' templates or template requested if you change it on the data, and solve accordingly.

This is more a question of what would feel 'natural' to users: updating what templates 'do' within the overrides, or just seeing them as 'copy-paste' within the overrides. Either case should be basically the same in terms of the amount of bugs.

Updating the templates within the overrides seems too convoluted for me, so I would not use it, but I think it's fine to keep the code as-is.

brynpickering · 2024-12-04T08:40:38Z

docs/migrating.md

@@ -181,10 +181,9 @@ This split means you can change configuration options on-the-fly if you are work
 `locations` (abbreviated to `locs` in the Calliope data dimensions) has been renamed to `nodes` (no abbreviation).
 This allows us to not require an abbreviation and is a disambiguation from the [pandas.DataFrame.loc][] and [xarray.DataArray.loc][] methods.

-### `parent` → `base_tech` + `template`
+### `parent` → `base_tech`


The original is correct. in v0.6 you could have an inheritance chain with parent. You could define a parent from tech_groups, which in turn could define a base_tech as its parent.

The problem here is that this PR removes all the 'inheritance' stuff. The original text would be confusing...
Would this make sense?

### `parent` → `base_tech` Technology inheritance has been unlinked from its abstract "base" technology. Technologies must now inherit from `base_tech` which fixed to be one of [`demand`, `supply`, `conversion`, `transmission`, `storage`].

OK, but for migrating from having an inheritance chain (previously defined by parent alone) is having base_tech + template. Maybe there needs to be a separate section for tech_groups -> templates. Basically, if a user has tech_groups, this page should provide them with a clear pointer for how to migrate. Currently there isn't one. That templates can do even more than tech_groups is besides the point.

Sounds reasonable. I will include that once other aspects of this PR are clarified.

src/calliope/preprocess/model_definition.py

tests/test_core_attrdict.py

tests/test_io.py

tests/test_preprocess_model_definition.py

irm-codebase · 2024-12-05T15:48:55Z

Ready for review again @brynpickering!

I've included all your suggestions, except a couple that I believe are not necessary (see the unresolved comments).

The random error in the national example model has been fixed. I must be honest in saying that I still see it as a bug, but it is not within the scope of this PR since it predates it.

brynpickering · 2024-12-05T16:56:09Z

Thanks @irm-codebase - I've responded. Where has the loss in coverage come from?

irm-codebase · 2024-12-05T17:06:23Z

Thanks @irm-codebase - I've responded. Where has the loss in coverage come from?

No prob. Looking at the codeconv report, it seems like it comes from the thinning of tools.py and other files.
This PR removed a lot of code from src/, so the percentages lie a bit here.

I can add tests, but they would not be for the functionality impacted by this PR.

brynpickering · 2024-12-05T17:09:55Z

I can add tests, but they would not be for the functionality impacted by this PR.

Makes sense. Nah, I won't force that on you!

irm-codebase · 2024-12-05T17:13:16Z

An exception seems to be in _load_yaml... for something that might not even be used anymore.
I'll give it a look in the next round of the review.

brynpickering

Sorry, forgot to comment on this!

src/calliope/io.py

irm-codebase · 2024-12-06T11:44:01Z

@brynpickering final update is in!
The important change is only the 'migrating' section. To keep this section short, I did not a new subsection. Instead, I modified the title a bit and added a short sentence.

I did this because the example serves both cases, and adding more text just increases our maintenance burden.

irm-codebase added 6 commits November 27, 2024 20:54

Move YAML functionality to calliope.io (pre AttrDict YAML removal).

e2c9b98

Add YAML templating w/tests (pre AttrDict YAML removal).

361f4a8

Simplify model initialisation to two main cases (pre-removal AttrDict…

14839de

… YAML / inheritance functions

Replace AttrDict and remove redundant 'inheritance' helper functionality

003f122

Rename scenarios to model_definition and rearrange test location.

2a1330d

Remove redundant AttrDict call in AttrDict.union(). Fill CHANGELOG.

5526a65

irm-codebase requested a review from brynpickering November 29, 2024 19:23

irm-codebase marked this pull request as ready for review November 29, 2024 19:23

irm-codebase changed the title ~~Feature attrdict declutter~~ Feature attrdict and model definition declutter Dec 2, 2024

Update documentation: templates now in YAML section

3b005bc

irm-codebase added 3 commits December 2, 2024 15:43

YAML title shortening

3af68b7

Remove references to the 'inheritance' helper function from the docum…

9f978d9

…entation

Merge branch 'main' into feature-attrdict-declutter

5eafc04

Remove unused load function in attrdict

3ba48a2

brynpickering requested changes Dec 4, 2024

View reviewed changes

irm-codebase mentioned this pull request Dec 4, 2024

Data priority not respected by ModelDataFactory #723

Closed

3 tasks

irm-codebase added 3 commits December 5, 2024 13:45

Move to_yaml from AttrDict to io

f9a377e

PR review improvements. Fix random data table information loss bug.

0253194

Merge branch 'main' into feature-attrdict-declutter

7b27b87

irm-codebase requested a review from brynpickering December 5, 2024 15:46

brynpickering reviewed Dec 5, 2024

View reviewed changes

src/calliope/io.py Outdated Show resolved Hide resolved

PR updates: migration section improvements, io remove unnecessary code

8d58894

irm-codebase requested a review from brynpickering December 6, 2024 11:44

brynpickering approved these changes Dec 6, 2024

View reviewed changes

brynpickering merged commit 9f33ecb into main Dec 6, 2024
13 checks passed

brynpickering deleted the feature-attrdict-declutter branch December 6, 2024 14:03

brynpickering mentioned this pull request Dec 10, 2024

Allow to use active in data tables #725

Open

This was referenced Dec 10, 2024

Use pydantic for the whole model definition #726

Open

Add config object to keep config in sync at all times #704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature attrdict and model definition declutter #719

Feature attrdict and model definition declutter #719

irm-codebase commented Nov 28, 2024 •

edited

Loading

irm-codebase commented Nov 29, 2024 •

edited

Loading

irm-codebase commented Dec 2, 2024

irm-codebase commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

brynpickering left a comment

brynpickering Dec 4, 2024

irm-codebase Dec 5, 2024

brynpickering Dec 5, 2024

irm-codebase Dec 5, 2024

brynpickering Dec 5, 2024

irm-codebase Dec 5, 2024

irm-codebase Dec 6, 2024

brynpickering Dec 4, 2024

irm-codebase Dec 5, 2024 •

edited

Loading

brynpickering Dec 5, 2024

irm-codebase Dec 5, 2024

irm-codebase commented Dec 5, 2024 •

edited

Loading

brynpickering commented Dec 5, 2024

irm-codebase commented Dec 5, 2024

brynpickering commented Dec 5, 2024

irm-codebase commented Dec 5, 2024

brynpickering left a comment

irm-codebase commented Dec 6, 2024

Feature attrdict and model definition declutter #719

Feature attrdict and model definition declutter #719

Conversation

irm-codebase commented Nov 28, 2024 • edited Loading

Summary of changes in this pull request

Reviewer checklist

irm-codebase commented Nov 29, 2024 • edited Loading

irm-codebase commented Dec 2, 2024

irm-codebase commented Dec 2, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

brynpickering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irm-codebase Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irm-codebase commented Dec 5, 2024 • edited Loading

brynpickering commented Dec 5, 2024

irm-codebase commented Dec 5, 2024

brynpickering commented Dec 5, 2024

irm-codebase commented Dec 5, 2024

brynpickering left a comment

Choose a reason for hiding this comment

irm-codebase commented Dec 6, 2024

irm-codebase commented Nov 28, 2024 •

edited

Loading

irm-codebase commented Nov 29, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

irm-codebase Dec 5, 2024 •

edited

Loading

irm-codebase commented Dec 5, 2024 •

edited

Loading