Importing `virtual_rainforest` package always causes models to be loaded

[alexdewar]

Currently if you import the virtual_rainforest package, all models are loaded (which is slow) and lots of log output is produced, even though the models are not necessarily needed. For example, if you run vr_run --help, you get this output.

This isn't currently a problem (though it would be nice if the output of vr_run --help weren't so ugly), but we may wish to add additional commands to vr_run or make virtual_rainforest usable as a library in some way, in which case this will become annoying.

[davidorme]

The intention behind this was to auto discover all models and make sure registries are populated and schema are discovered. There is very little that can be done with the model until that has happened, so sticking it in __init__.py made a lot of sense, but as the suite of tools and entry points we support increases, that is going to be happening prematurely.

I think we basically need to wrap this step up into a discover_models function - but having that as a mandatory step before you can run a model also seems a bit clunky.

The immediate logical place to put that step might be within the Config set up:

• You have to have a Config object to run a model.
• You have to have loaded all the models to validate the configuration.
• It delays model loading until a step that logically precedes running a model.

So if this happens automatically as a method of Config? The thing I can't remember is whether the model tests necessarily go through the config process? The sequence is awkward.

[alexdewar]

That makes sense.

Would it work to have the model discovery step be done explicitly in vr_run()? In the short term, it may be sufficient to just move various imports into the body of vr_run().

[davidorme]

We could do that but I think it would be better if the model loading happened as a part of standard operation - like creating the Config - rather than as part of a longer recipe. That is primarily so that programmatic use of the library is more logical/cleaner. Short term, we could use a vr_run recipe, but I don't think that's a natural place for it.

[alexdewar]

I've been taking a look at this and I can't help wondering whether it would simply be best to just try load the models as needed. Aside from this specific issue we're talking about, another problem with the current code is that every model is loaded even if the user only requires a subset.

One way to do this would be to just import the modules lazily, as Python is clever enough not to reimport modules anyway, so before trying to access SCHEMA_REGISTRY["my_model"] you'd run importlib.import_module("virtual_rainforest.models.my_model"). It would probably make sense to have this functionality for accessing the SCHEMA_REGISTRY hidden behind a helper function.

[davidorme]

That makes sense but I was wondering if we can make SCHEMA_REGISTRY itself a bit smarter and import modules? The advantage to that is that we can just use SCHEMA_REGISTRY["my_model"] to trigger the import. I think the argument for this is that then we don't even have to create a Config to ensure that a model is loaded, which I think might be useful in some of the tests?

Something like this? I haven't tested it at all 😄

class SchemaRegistry(dict):

    def __getitem__(self, __key: Any) -> Any:

        if __key in self:
            return super().__getitem__(__key)

        try:
            import_module(f"virtual_rainforest.models.{__key}")
        except Exception as excep:
            LOGGER.error(f"Error while loading '{__key}' model: {str(excep)}")
            raise

        return super().__getitem__(__key)

SCHEMA_REGISTRY = SchemaRegistry()

One thing that might be a problem there is that self is modified by the module import while __getitem__ is executing - would the last line in the method actually access that updated value?

[davidorme]

I think it works. If I modify that so that import_module is just a simple function to modify SCHEMA_REGISTRY (and switch the logic about to make it cleaner).

class SchemaRegistry(dict):

    def __getitem__(self, __key: Any) -> Any:

        if not __key in self:
            try:
                extend_sr(__key)
            except Exception as excep:
                LOGGER.error(f"Error while loading '{__key}' model: {str(excep)}")
                raise

        return super().__getitem__(__key)

SCHEMA_REGISTRY = SchemaRegistry()

def extend_sr(ky):

    SCHEMA_REGISTRY[ky] = ky * 3

Then:

In [12]: SCHEMA_REGISTRY
Out[12]: {}

In [13]: SCHEMA_REGISTRY['gone']
Out[13]: 'gonegonegone'

In [14]: 

[jacobcook1995]

Yeah definitely think moving away from importing every model is sensible. The current approach was just a stop gap I added because I couldn't come up with a more elegant way to make sure that MODEL_REGISTRY and SCHEMA_REGISTRY contained the models when vr_run was called

[alexdewar]

@davidorme Yes, that's the kind of thing I had in mind.

I would say though that we don't want to be overriding magic methods where we don't have to. How about just adding a regular function to load/get the schemas? I don't think anyone should be using SCHEMA_REGISTRY directly outside of config.py anyhow.

[davidorme]

@alexdewar Avoiding overriding methods because it is less clear that something else happens beyond the getting? I buy that - it is an extra line of code, but it is clearer. I'd be inclined to make it a method of a SchemaRegistry object, but that's just packaging.

SCHEMA_REGISTRY.load('my_model')
schema = SCHEMA_REGISTRY['my_model']

The only thing in the back of my mind is that the tests needed the SCHEMA_REGISTRY to be populated before the tests could run, but I can't remember the details and that sounds sub-optimal anyway.