Fix sync issues in model math / config / defaults attributes

[irm-codebase]

Fixes #608 (and is a prerequisite for #606).

This PR is meant to fix potential issues due to duplication of model data at the model.attribute level and at the model._model_data.attrs level.

Instead of copying stuff, class @properties are used to call and modify specific things (math, config, defaults). model.attributes should be used for data we want to lose between runs (instance-specific timestamps, temp flags, etc).

By drawing this distinction, the code should become easier to maintain down the line.

Summary of changes in this pull request

• Turned model.math, model.config and model.defaults into properties that refer to model._model_data.attrs directly to avoid double instancing and potential desyncs
• Removed depreciated methods to sync model.math and model.config with values in model._model_data.
• Updated some tests.

Reviewer checklist

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

[irm-codebase]

@sjpfenninger @brynpickering Here is a proposed improvement to avoid sync issues in the model object and its underlying xarray data.

I tried to update relevant tests, let me know if I missed something.

[irm-codebase]

Changed this PR to merge directly into main instead of the schema override feature. I figured that getting this in will benefit other things beyond that.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.93%. Comparing base (872978d) to head (8c92ba5). Report is 13 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #610 +/- ## ========================================== + Coverage 95.85% 95.93% +0.07% ========================================== Files 24 24 Lines 3619 3638 +19 Branches 788 736 -52 ========================================== + Hits 3469 3490 +21 + Misses 86 84 -2 Partials 64 64 ``` | [Files](https://app.codecov.io/gh/calliope-project/calliope/pull/610?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=calliope-project) | Coverage Δ | | |---|---|---| | [src/calliope/model.py](https://app.codecov.io/gh/calliope-project/calliope/pull/610?src=pr&el=tree&filepath=src%2Fcalliope%2Fmodel.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=calliope-project#diff-c3JjL2NhbGxpb3BlL21vZGVsLnB5) | `93.41% <100.00%> (-0.24%)` | :arrow_down: | | [src/calliope/postprocess/postprocess.py](https://app.codecov.io/gh/calliope-project/calliope/pull/610?src=pr&el=tree&filepath=src%2Fcalliope%2Fpostprocess%2Fpostprocess.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=calliope-project#diff-c3JjL2NhbGxpb3BlL3Bvc3Rwcm9jZXNzL3Bvc3Rwcm9jZXNzLnB5) | `98.21% <100.00%> (+4.66%)` | :arrow_up: | ... and [12 files with indirect coverage changes](https://app.codecov.io/gh/calliope-project/calliope/pull/610/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=calliope-project)

[irm-codebase]

My initial setup for testing the new linked properties was ugly and hard to maintain. I improved it following better testing practices.

[brynpickering]

I'd prefer just two ways to define the config/defaults/math for a model: 1. in the YAML files, 2. at the point of method calls (Model(**kwargs), model.build(**kwargs), model.solve(**kwargs)). Model.config, Model.defaults, Model.math are then just handy references for the current state of the configuration.

[irm-codebase]

@brynpickering :+1: Alright, I'll try to summarize the points. For what I understand, there are two issues

• Issue 1 is whether or not we want model._model_data to be frozen. Particularly in these situations
  • Should kwargs be saved to it, or only affect the object temporarily?
  • Should we offer interfaces to modify this beyond YAML files and kwargs?
• Issue 2 (this PR) is that model.attrs have inconsistent access to model._model_data.attrs. Python works with pointers, so in most cases modifying model.config will also modify model._model_data.attrs["config"]... but in our current approach this is not guaranteed!

My fix was not meant to be user-facing. It was meant to streamline the way we access data within our code. But I see why it could be seen that way...

Here are my proposed fixes to this PR in order to address Issue 2:

• remove model.defaults: I included it because it was declared in model.__init__, but unless we ensure updates propagate it should definitely not be there.
• alter model.math to model._math to specify users should not mess with it. We keep the setter and getter using @property to ensure consistency.
• ditto for model.config
• modify our code accordingly

With this we no longer have unclear attributes in model, and we have methods that streamline our code for the future. What do you think?

[brynpickering]

I don't think they should be private properties (hence why they are public right now), but they should be immutable. Providing a setter goes against that idea as you essentially say "you can replace this entire dictionary if you like". It probably just makes sense to make the change you've implemented except for the setters. I.e., you don't provide a way for a user to completely replace those properties, although we still have no way of stopping them replacing the equivalent dictionary in model._model_data.attrs, which would have the same effect.

[brynpickering]

Options for making these properties read-only:

1. Use something like frozendict to make the dictonaries linked to the properties un-settable.
2. pretty-print the dictionary instead of providing as a dictionary object (e.g. print(yaml.dump(m.config.init.as_dict()))).
3. Add some switch in AttrDict to allow freezing (i.e. just suppressing the dictionary setter in there somehow).
4. Turning the AttrDict into a set of nested ModelConfig objects that you can get (model.config.init.name), but you can't set

[irm-codebase]

Python handles this automatically by creating a getter, but no setter. We already do this for model.is_solved. I plan to follow a similar approach in the re-try.

[irm-codebase]

@brynpickering Retry! This time I kept things simple: I've added getter @property that will let users see model math, defaults and configuration, but not modify them.

In theory, we could define "protected" @property version of these to make our code leaner (i.e., not always having to use self._model_data.attrs["yaddayadda"] for everything. For example: model._math could have a setter and a getter, while model.math only has a getter.

I've decided against this to keep the PR simple, and because it's a nice-to-have. Let me know if you want this included.

[irm-codebase]

This PR is related to #617 too. Unfortunately, fixing that is currently breaking operate mode. It's likely that fixing that issue involves too many changes, so this PR will be kept as-is to make the review easier.

[irm-codebase]

I've just realized that we are putting the model configuration in ANOTHER attribute: model._model_def_dict

This PR is going back to the drawing board, unfortunately.

[irm-codebase]

This PR is replaced by #625.