-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix sync issues in model math / config / defaults attributes #610
Conversation
@sjpfenninger @brynpickering I tried to update relevant tests, let me know if I missed something. |
Changed this PR to merge directly into main instead of the schema override feature. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
|
My initial setup for testing the new linked properties was ugly and hard to maintain. I improved it following better testing practices. |
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.
Although I added a slight clean-up of repetition, I'm not sure this is actually the way we should go with these properties.
config
items should be completely frozen - you can view them using e.g. config.build[...]
but you cannot set any values in them. Any runtime overrides should be kept to the model calls (model.build(**config_override_kwargs)
).
defaults
is one that a user could feasibly edit, but I'm aware that it is quite brittle at the moment. If you update a value in model.defaults
for a parameter available in model._model_data
, that change won't propagate (because those parameters have a default
attribute assigned to it earlier on which takes precedence). So it should also probably be frozen for the time being.
model.math
is the one I know you want to be able to replace as part of #609 and is the only one that may benefit from the ability to set it as an entirely new dictionary. However, inconsistencies can arise. If you build and solve a model and then update model.math
and save your model, the model math is then out of sync with the model results and you'll be sharing potentially misleading datasets.
For all these properties, the question then remains that if we let users provide on-the-fly updates (e.g. the kwargs in model.build(**kwargs)
), should they propagate back to the properties and be visible in those frozen dicts or should those stay frozen based on what was loaded from file and then we have some additional property that is build/solve-dependent, giving the config overrides that were applied.
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 ( |
@brynpickering 👍
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:
With this we no longer have unclear attributes in |
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 |
Options for making these properties read-only:
|
94ef7db
to
4079fbd
Compare
Python handles this automatically by creating a getter, but no setter. We already do this for |
@brynpickering Retry! In theory, we could define "protected" 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. |
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. |
I've just realized that we are putting the model configuration in ANOTHER attribute: This PR is going back to the drawing board, unfortunately. |
This PR is replaced by #625. |
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
model.math
,model.config
andmodel.defaults
into properties that refer tomodel._model_data.attrs
directly to avoid double instancing and potential desyncsmodel.math
andmodel.config
with values inmodel._model_data
.Reviewer checklist