Updated model API #354

Peter9192 · 2023-06-13T15:46:53Z

This PR rewrites the core eWaterCycle model class, such that

There is a base implementation for a LocalModel (mostly for development purposes)
There is a base implementation for a ContainerizedModel (intended for production)

Together with the new with the new plugin architecture (#336, #347), this should make it easier to add new models to eWaterCycle.

To do:

Fix typeerror
Use the new model interface for Wflow
Use the new model interface for Marrmot
Use the new model interface for Lisflood
Use the new model interface for Hype

moved to #359:

Update tests for abstractmodel to test the new API (once crystallized)
Update documentation
Remove new_model_api.py from repo.
Nice to have: Extend containerimage class to check version
Nice to have: Extend containerimage class to automatically pull docker image and convert to singularity

…and xarray; add get_latlon_grid method

…wb can now be instantiated.

Peter9192 · 2023-06-16T16:06:03Z

@BSchilperoort @sverhoeven I'm gonna call it a (holi)day. Tried not to make too big of a mess.. Hope you can pick it up from here.

BSchilperoort · 2023-07-24T11:24:13Z

I discussed the API with Peter, and we've decided to (for now) pin the container version in the model class.
Model plugin developers will have to release a new version of when a new docker version is released.

E.g.:

class Wflow(ContainerizedModel):
    bmi_image: ContainerImage("ewatercycle/wflow-grpc4bmi:2020.1.3")

Setting the version to "latest" will have reproducibility issues & managing lists of valid versions is a lot of bookkeeping and hassle.
Of course, as bmi_image is a kwarg, a user can overwrite this.

Additionally, I believe this will remove the need for ParameterSet.supported_model_versions, as the model's parameter set is packaged with the model class in the plugin.

BSchilperoort · 2023-07-25T06:26:44Z

@Peter9192 I have moved over Hype, and fixed the Hype tests (/hype/test_model.py). Could you have a look before I move all the rest over and fix their tests?
I also had to make some modifications to /base/model.py

Peter9192

Hi Bart, nice work so far. I have a few comments, we can discuss them if anything is unclear.

src/ewatercycle/base/model.py

src/ewatercycle/plugins/hype/model.py

Peter9192 · 2023-07-26T05:56:53Z

src/ewatercycle/plugins/hype/model.py

+                in CFG.output_dir
+        """
+        cfg_path = super()._make_cfg_dir(
+            cfg_dir=cfg_dir, folder_prefix="hype", **kwargs


Do we need to do this for every model now only to add the folder prefix? Would it be possible to get that from self.__class__ or self.__name__?

Using self.__class__.__name__.lower() would probably be a nice way of retrieving the folder prefix. It would usually remove the need to define _make_cfg_dir

Peter9192 · 2023-07-26T06:00:43Z

src/ewatercycle/plugins/marrmot/model.py

+    _model_name: str = PrivateAttr("m_01_collie1_1p_1s")
+    _parameters: List[float] = PrivateAttr([1000.0])
+    _store_ini: List[float] = PrivateAttr([900.0])
+    _solver: Solver = PrivateAttr(Solver())
+    _model_start_time: str | None = PrivateAttr()
+    _model_end_time: str | None = PrivateAttr()

-    def __init__(self, version: str, forcing: MarrmotForcing):  # noqa: D107
-        super().__init__(version, forcing=forcing)
-        self._parameters = [1000.0]
-        self.store_ini = [900.0]
-        self.solver = Solver()
-        self._check_forcing(forcing)
+    _forcing_filepath: str = PrivateAttr()
+    _forcing_start_time: datetime.datetime = PrivateAttr()
+    _forcing_end_time: datetime.datetime = PrivateAttr()


I wonder if it makes sense to store all these as attributes separately on the model, or whether we could store them all in a single config object (can be a simple dict). Ideally we could use the models' native configuration files for that. So on creation you'd parse the template config file, update it in memory as necessary, then in setup save a copy with the latest changes and return that one.

I am not sure if it matters too much if they're separate private attributes or items of a private attr dictionary.

Up to now I've mostly focused on getting everything moved over to the new API and working again. Seeing as these are all private attributes (i.e. not user accessible) we can change the internal workings (e.g., by rearranging them in a dict) without breaking things.

Peter9192 · 2023-07-26T07:08:20Z

Additionally, I believe this will remove the need for ParameterSet.supported_model_versions, as the model's parameter set is packaged with the model class in the plugin.

This only holds for the example parameter sets. However, we envision that researchers could share their parametersets as a standalone dataset, and that you could then load that by means of a configuration file. Ideally datasets would be shared via zenodo or other repos and retrieved through their doi. So then, you'd need to add sufficient metadata about compatibility.

BSchilperoort · 2023-07-26T14:27:45Z

I have now moved over all models, and attempted to standardize them a bit more. All tests except the lisflood ones (and AbstractModel one) are now passing. The lisflood ones are a bit difficult to work on because a large download takes place every time the test is run.

Working with pydantic is a bit frustrating as it doesn't seem to fit our use completely. In v1 for example, a post_init method is missing, and the @root_validator decorator is not a very good replacement (as it takes cls and not self). At least that's fixed in v2.

Also, pydantic seems to have an attribute "parameters" which can get a bit confusing for us/our users.

Peter9192 · 2023-07-27T07:13:01Z

Working with pydantic is a bit frustrating as it doesn't seem to fit our use completely.

We should definetely start using v2 asap. But if that's still frustrating, we could consider using "normal" dataclasses.

Peter9192 · 2023-07-27T09:19:23Z

src/ewatercycle/plugins/wflow/model.py

-            lat: Latitudinal value
-            lon: Longitudinal value
+    def get_parameters(self):
+        self._post_init()


this post_init is not defined, should it call _parse_config instead?

Oh yeah you're right. I guess the tests didn't catch any of that... I think renaming the _parse_config is best.

Doing these operations in a post_init in all models would make them look more similar.

Peter9192 · 2023-07-27T09:54:22Z

src/ewatercycle/plugins/hype/model.py

+    def get_latlon_grid(self, name: str) -> tuple[Any, Any, Any]:
+        raise NotImplementedError("Hype coordinates cannot be mapped to grid")
+
+    def get_parameters(self) -> Iterable[Tuple[str, Any]]:


@BSchilperoort why did you change the parameters from a property called parameters to a method called get_parameters? Is this to do with what you said about pydantic?

Note that we want to use parameter akin to pymt: https://pymt.readthedocs.io/en/latest/usage.html#model-setup

I now see that *Model.parameters is defined as a variable property in BaseModel. I think it conflicted with defining the parameters as @property in the specific model implementations.

I guess it's best to change how the BaseModel implements that.

Side note: I've found it a bit confusing that our BaseModel has the same class name as the pydantic BaseModel 😕

class BaseModel(pydantic.BaseModel, abc.ABC): ...

Renamed to eWaterCycleModel in 998b226

BSchilperoort · 2023-07-27T10:24:06Z

Working with pydantic is a bit frustrating as it doesn't seem to fit our use completely.

We should definetely start using v2 asap. But if that's still frustrating, we could consider using "normal" dataclasses.

If we do at some point decide to not use pydantic, a good alternative is probably typing's Protocol, and use @runtime_checkable. Although those checks are not as robust as Pydantic.

Peter9192 · 2023-07-27T10:25:39Z

Pff merge concluded.. 😅

sverhoeven

Better then before. Still lots to do.

Peter9192 · 2023-07-28T07:17:07Z

Better then before. Still lots to do.

See #359

Peter9192 added 11 commits June 13, 2023 17:39

Core proposal for new model interface

e63e7d2

Move concept to replace the original api

08da73d

Add class for dealing with container image names and verisons

0336861

wip - use new interface for pcrglobwb

20de9df

Use new api in pcrglob and provide default implementation for coords …

e58c3d5

…and xarray; add get_latlon_grid method

use optionaldestbmi wrapper

56f8c1a

fix flake8 and type issues in base/model.py

6541406

Make ContainerImage subclass of str and add validation to it

ff84bbc

Fix types and flakes in container and pcrglob model

40b80e4

add tests for containerimage

16b7eb7

Use validators to process received parametersets and forcing; pcrglob…

a384061

…wb can now be instantiated.

BSchilperoort added 5 commits July 24, 2023 14:05

Move wflow to new model interface

faf4b07

Fix typing issues, improve wflow implementation

ee6352f

Move hype to new model api, fix hype tests

b336979

Make cfg dir and file pydantic privateattrs

4cda895

Add type assertion, pydantic (v1) seems unreliable

54a7b80

BSchilperoort added 3 commits July 25, 2023 10:49

Fix wflow tests

156ad1b

Make _make_cfg_dir reusable

b9fae33

Move marrmot to new api, fix tests

054a011

Peter9192 commented Jul 26, 2023

View reviewed changes

Peter9192 mentioned this pull request Jul 26, 2023

Basemodel #342

Closed

BSchilperoort added 2 commits July 26, 2023 14:28

Improve _make_cfg_dir, fix tests

f9d490a

Add version nrs, start move to post_init

380979c

Add preliminary parameter set version check

2928b59

Move lisflood to new API, fix tests

6bc3cee

Peter9192 mentioned this pull request Apr 6, 2023

Simplify adding models #338

Closed

5 tasks

Peter9192 commented Jul 27, 2023

View reviewed changes

Peter9192 added 2 commits July 27, 2023 12:23

Merge remote-tracking branch 'origin/main' into defaultmodels

73e4fee

fix some tests

422550b

Peter9192 added 2 commits July 27, 2023 14:12

fix setup of pcrglobwb (failing test)

9baf4de

fix more tests

11617db

Peter9192 mentioned this pull request Jul 27, 2023

Model baseclass follow-up todo's #359

Closed

8 tasks

Peter9192 marked this pull request as ready for review July 27, 2023 13:05

sverhoeven approved these changes Jul 27, 2023

View reviewed changes

Peter9192 merged commit 92cd616 into main Jul 27, 2023
2 of 3 checks passed

Peter9192 deleted the defaultmodels branch July 27, 2023 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated model API #354

Updated model API #354

Peter9192 commented Jun 13, 2023 •

edited

Loading

Peter9192 commented Jun 16, 2023

BSchilperoort commented Jul 24, 2023

BSchilperoort commented Jul 25, 2023

Peter9192 left a comment

Peter9192 Jul 26, 2023

BSchilperoort Jul 26, 2023

Peter9192 Jul 26, 2023

BSchilperoort Jul 27, 2023

Peter9192 commented Jul 26, 2023

BSchilperoort commented Jul 26, 2023 •

edited

Loading

Peter9192 commented Jul 27, 2023

Peter9192 Jul 27, 2023

BSchilperoort Jul 27, 2023

Peter9192 Jul 27, 2023 •

edited

Loading

BSchilperoort Jul 27, 2023

Peter9192 Jul 28, 2023

BSchilperoort commented Jul 27, 2023

Peter9192 commented Jul 27, 2023

sverhoeven left a comment

Peter9192 commented Jul 28, 2023 •

edited

Loading

Updated model API #354

Updated model API #354

Conversation

Peter9192 commented Jun 13, 2023 • edited Loading

Peter9192 commented Jun 16, 2023

BSchilperoort commented Jul 24, 2023

BSchilperoort commented Jul 25, 2023

Peter9192 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

Peter9192 commented Jul 26, 2023

BSchilperoort commented Jul 26, 2023 • edited Loading

Peter9192 commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peter9192 Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BSchilperoort commented Jul 27, 2023

Peter9192 commented Jul 27, 2023

sverhoeven left a comment

Choose a reason for hiding this comment

Peter9192 commented Jul 28, 2023 • edited Loading

Peter9192 commented Jun 13, 2023 •

edited

Loading

BSchilperoort commented Jul 26, 2023 •

edited

Loading

Peter9192 Jul 27, 2023 •

edited

Loading

Peter9192 commented Jul 28, 2023 •

edited

Loading