Resolving constants milestone issues and constants bundling.

[davidorme]

This is a cross between a meta-issue and a discussion to tidy up a backlog of constants questions that we've grouped in the "Constants system" milestone.

This issue is also a summary of the inaugural monthly "GitHub issues" developer meetings to bundle existing issues and clean them up 🎉 🥂

Where we are now

• We have now adopted the constants system presented in discussion #162
• There's been some discussion of passing multiple constants classes to a model: CoreConstants and ModelSpecificConstants and whether it might be easier to merge core constants into each model constants to provide a simpler namespace (see #342).

Outstanding issues

We have five outstanding constants issues:

• 129 is about creating core constants but contains still relevant discussions about units. We have a separate milestone bundling units issues so I think we can simply close #129.

• 277 is about how we ensure that configured constants are passed down correctly rather than falling back to defaults. I think this is largely outdated now that we are using the constants system, so I think we can close #277.

• 348 is still active and just needs a PR to redefine constants values through the models.

The remaining two issues are linked and this PR suggests a resolution for both:

• 342 is the discussion about simplifying model signatures to only need one bundled constants class. Implementations for this were also discussed in #341.

• 358 is an issue that @vgro had with sharing constants and functions between parallel model development: if a function using constants is defined in one model then currently importing and re-using that function in another model requires also importing the constants and juggling multiple parallel constants classes.

Sharing constants and functionality

The proposed solution to #358 that we can define model "meta" constants classes and use class inheritance to allow the clean use of different constants classes with shared functions. So:

from virtual_rainforest.core.constants import ConstantsDataclass

class ModelMeta(ConstantsDataclass):
    """A meta constants class created in e.g. SimpleModel."""
    b = 2
    c = 3

class ActualModelSimple(ModelMeta):
    """The actual simple model constants from SimpleModel.

    This is derived from the meta class and only adds a constant specific to this model
    """
    d = 5

class ActualModelComplex(ModelMeta):
    """A more complex model constants from a parallel model.

    This adds two new constants and overwrites an existing constant with a model specific version
    """
    b = 4
    d = 5
    e = 6

def shared_function(constants: ModelMeta):
    """A function created in simple model that we also want to use in the complex model."""    
    return constants.b 

These can then be used seamlessly in both models - all of the statements below are mypy friendly because the typing of ModelMeta supports the meta class itself and any derived model specific subclasses.

In [7]: meta = ModelMeta()
In [8]: simple = ActualModelSimple()
In [10]: cplex = ActualModelComplex()
In [11]: shared_function(meta) # Although you'd never really do this in practice!
Out[11]: 2
In [12]: shared_function(simple)
Out[12]: 2
In [13]: shared_function(cplex)
Out[13]: 4

Merging core constants into models - we could but no

That solution also opens up this resolution for #342 - which seems really clean and avoids the whole issue with properties that was mentioned on this topic in #341.

from virtual_rainforest.core.constants import ConstantsDataclass

class CoreConstants(ConstantsDataclass):
    a = 1

class ModelMeta(CoreConstants):
    b = 2
    c = 3

class ActualModelSimple(ModelMeta):
    d = 5

And hence:

In [15]: const = ActualModelSimple()
In [16]: const.a
Out[16]: 1

However, the dev meeting concluded:

• That seems super clean and simple but we then have the core constants multiply defined independently in each model constants class - they shouldn't differ but why would we even open up the possibility?
• This does mean that models will typically always have two constants classes, but this does make a clean separation of a singly defined set of core constants and then model specific things.
• As an aside, at the moment, models using the core constants are independently creating fresh instances from Config, but we should simply create one instance at startup and share it through the model signature somehow. We could add core_constants to the ABC signature.

So - we should simply close #342 and live with having two constants classes.

[dalonsoa]

I was following the argument well until I got here:

That seems super clean and simple but we then have the core constants multiply defined independently in each model constants class - they shouldn't differ but why would we even open up the possibility?

How is that possibility open? Core constants will be defined using ClassVar, like in https://github.com/ImperialCollegeLondon/virtual_rainforest/blob/dd41f744e6177151e9e7c1b51ffb9ebe78904622/virtual_rainforest/core/constants.py#L21 So even if the constants are multiply defined, the core ones will be the same. By construction, they cannot be changed.

Did I miss anything?

[davidorme]

Not all core constants are necessarily defined using ClassVar - there may be core constants that users can sensibly alter. My concern here is that in theory, users could get models with different core constants, but looking back, that is extremely paranoid:

• The instances cannot be manipulated within vr_run
• Even from within a Python terminal, instances are frozen - technically they can be changed between model instantation, but a user could equally well just use different CoreConstants instances with different models. Both of those are clearly malicious incompetence 😄.

I think the more compelling argument is that it is just needless duplication.

[dalonsoa]

As we all know, in Python, if a user want to change something, there's absolutely nothing that can prevent that. But beyond certain reasonable cases, if a user changes something they should not and that results in useless results, to be honest, they deserve it.

On your last point, needless duplication does not sound too compelling to me if you sacrifice simplicity. For me needless duplication is having to keep two separate sets of constants. That makes the interface more complicated - not super complicated, just a bit more complicated. If the risk of having inconsistent set of constants is as small as you suggest, I honestly would go for the super clean and super simple approach you have come up with.

[davidorme]

I agree about users deserving what they get in those cases 😄

I think there is a cost to simplicity in what we would need to do to load_constants to merge the currently separated config namespaces for core and model constants. In the meeting, I think @vgro , @jacobcook1995 and I generally agreed that having two constants namespaces was a mild complication we were happy to live with?

Beyond the handling of core constants, though, the approach for the meta constants seems sound?

[dalonsoa]

Yes, that looked sensible, considering the problem to solve. I guess one potential difficulty might be finding the common constants that should go in the Meta model, but maybe I'm seeing a problem where there is none.