Overhaul of Python architecture #731

evetion · 2023-11-02T15:54:38Z

Fixes #512
Groundwork for #630 & #252

The code has become much simpler in some places (e.g. write_toml).

from ribasim import Model

m = Model(filepath="generated_testmodels/basic/ribasim.toml")
m.database.node.df # Node table
m.basin.static.df # BasinStatc table
m.write("test")

Some notes:

The config.py file cannot be autogenerated anymore. The schemas still can, but I disabled it for now to be sure (some imports error).

Changes:

I created new (parent) classes:

BaseModel, from Pydantic, with our own config
FileModel, like hydrolib-core (but now Pydantic v2), which can take a single filepath for initilization. Models who inherit require defining _load and _save, dealing with that filepath.
NodeModel, a class for nodes (where add will be).
TableModel, a class to read/write individual tables from/to gpkg/arrow.
SpatialTableModel, inherits TableModel, but reads/writes spatial stuff.

I changed:

the Model class to be a carbon copy of Config (which has been deleted), so it mirrors the toml.
in turn this created a database NodeModel class (reflecting the field in the toml), with only Node and Edge underneath.
the NodeModel classes Basin from their node_type version to the one in Config, and set the type of the underlying table with a TypeVar like so:

class Terminal(NodeModel):
    static: TableModel[TerminalStaticSchema]

Yet to do:

Update tests
Fix sort! rules
Delete node_types folder
Link schemas to their Pydantic class (TableModel[TerminalStaticSchema] => TerminalStatic)

Does not yet work with ribasim-python.

The testmodels need to be present for the build process to work I've also formatted, so the diff is slightly bigger than expected

Mypy doesn't like these dict defaults in config.py, since they are not the same type as the annotated type. Can't we somehow avoid copying the same defaults twice here?

visr · 2023-11-02T15:57:51Z

The config cannot be autogenerated without overwriting this code.

What do you mean by this?

evetion · 2023-11-02T16:04:37Z

The config cannot be autogenerated without overwriting this code.

What do you mean by this?

We currently autogenerate all code in config.py, but I changed that code manually and use it extensively. Unless I write a new template for the codegenerator, I don't see autogeneration happening in the near future. (we can/do still autogenerate the schemas though).

visr · 2023-11-02T16:39:07Z

Ok thanks. That's fine, I don't see that as a blocker.

Hofer-Julian

Looks good @evetion.

To later review the usage, tests or even better the example jupyter notebook would be helpful.

pixi.toml

python/ribasim/pyproject.toml

python/ribasim/ribasim/config.py

python/ribasim/ribasim/model.py

python/ribasim/ribasim/config.py

Co-authored-by: Hofer-Julian <[email protected]>

Hofer-Julian · 2023-11-03T13:57:04Z

I've added 507d925 to your branch so similar type hint lints are caught by ruff directly.

evetion · 2023-11-03T21:15:10Z

I've added 507d925 to your branch so similar type hint lints are caught by ruff directly.

I had to revert it for now, as the autogenerated schemas still use the old notation. :/ Will see if I can fix that at a later stage.

visr · 2023-11-07T09:58:44Z

the autogenerated schemas still use the old notation

Is that fixed by target-python-version? datamodel-codegen --target-python-version=3.10
Mentioned in #653 (comment).

evetion · 2023-11-13T10:03:45Z

the autogenerated schemas still use the old notation

Is that fixed by target-python-version? datamodel-codegen --target-python-version=3.10 Mentioned in #653 (comment).

Nope, but it was --use-union-operator 🎉

pixi.toml

Hofer-Julian · 2023-11-14T08:34:14Z

I've added be766f9 as an example how to simplify the code

Hofer-Julian

The changeset is naturally massive so I didn't check every detail.
But overall it looks good

python/ribasim/ribasim/input_base.py

visr

Looks good to me! Left some comments, but they could also be moved out to separate issues or follow up PRs if you prefer.

visr · 2023-11-14T14:06:48Z

python/ribasim/tests/test_model.py

        {"flow_rate": 1, "node_id": -1, "remarks": "", "active": True},
        ignore_index=True,
    )
+    # Currently can't handle mixed NaN and None in a DataFrame


Is this a pandas limitation? We could benefit from a clear guide on how we deal with / represent missing data for different data types on both Julia, Python, Arrow and GeoPackage sides.

See also this comment: @evetion what do you recommend for missing / default string columns; missing or ""?

Pandas is fine, it's just that with adding new columns, mixing None (object) and NaN (float) doesn't play nice with pandera. I might've fixed that in the last commit. I will write a guide in the subsequent PR on how to handle missing values in Python. (I think it's pretty clear in Julia & geopackage).

visr · 2023-11-14T14:10:04Z

python/ribasim_testmodels/ribasim_testmodels/trivial.py

+        database=ribasim.Database(
+            node=node,
+            edge=edge,
+        ),


I like that these are now together in a single class, and understand you named it Database since these tables always have to be in the database. But wouldn't a name like Network better represent what it really is? If all tables are stored in the database, users may expect to find all tables in ribasim.Database.

The field is database because it is named so in the toml. I thought about naming the class Topology, but Network is a good alternative. This can be renamed in the follow up PR, when we actually start using said class more extensively.

Fixes #512 Groundwork for #630 & #252 The code has become much simpler in some places (e.g. write_toml). ```python from ribasim import Model m = Model(filepath="generated_testmodels/basic/ribasim.toml") m.database.node.df # Node table m.basin.static.df # BasinStatc table m.write("test") ``` ### Some notes: - The config.py file cannot be autogenerated anymore. The schemas still can, but I disabled it for now to be sure (some imports error). ### Changes: I created new (parent) classes: - BaseModel, from Pydantic, with our own config - FileModel, like hydrolib-core (but now Pydantic v2), which can take a single filepath for initilization. Models who inherit require defining _load and _save, dealing with that filepath. - NodeModel, a class for nodes (where `add` will be). - TableModel, a class to read/write individual tables from/to gpkg/arrow. - SpatialTableModel, inherits TableModel, but reads/writes spatial stuff. I changed: - the Model class to be a carbon copy of Config (which has been deleted), so it mirrors the toml. - in turn this created a `database` NodeModel class (reflecting the field in the toml), with only Node and Edge underneath. - the NodeModel classes Basin from their node_type version to the one in Config, and set the type of the underlying table with a TypeVar like so: ```python class Terminal(NodeModel): static: TableModel[TerminalStaticSchema] ``` ### Yet to do: - [x] Update tests - [x] Fix sort! rules - [x] Delete node_types folder - [x] Link schemas to their Pydantic class (TableModel[TerminalStaticSchema] => TerminalStatic) --------- Co-authored-by: Martijn Visser <[email protected]> Co-authored-by: Hofer-Julian <[email protected]> Co-authored-by: Hofer-Julian <[email protected]>

I noticed with #731 we went back from Python 3.12 to 3.11. This updates all dependencies, including Python back to 3.12. I did this using pixi 0.7.0 which just came out: https://github.com/prefix-dev/pixi/releases/tag/v0.7.0 The lockfile format seems the same, so I'm hoping this works on TeamCity with pixi 0.6.0, but I triggered tests to make sure. There is a new top level `version: 2` entry on top of the lockfile, so it looks like they started versioning it.

@evetion

Follow-up as mentioned in #731 (comment) Not passing the tests yet, as I don't understand how to have a different TOML key `geopackage` and field `network`. Writing to TOML is 'fixed' by bbb2982, but tests are still failing. @evetion could you help out? --------- Co-authored-by: Maarten Pronk <[email protected]>

visr and others added 9 commits October 16, 2023 22:17

get pydantic v2

d736371

Does not yet work with ribasim-python.

add default None to Optional node types for pydantic v2

ec23146

run bump-pydantic

e2a8e3e

update addnode example

135be97

fix deprecation warning

d07e9ad

Add dependency on testmodel for build binary tasks (#672)

ce4b332

The testmodels need to be present for the build process to work I've also formatted, so the diff is slightly bigger than expected

datamodel-codegen pydantic v2 output

50cf27f

Mypy doesn't like these dict defaults in config.py, since they are not the same type as the annotated type. Can't we somehow avoid copying the same defaults twice here?

Merge main.

2d2d439

Reading writing works.

6d70886

Merged main.

e2510be

Hofer-Julian reviewed Nov 3, 2023

View reviewed changes

evetion and others added 2 commits November 3, 2023 10:33

Apply suggestions from code review

6f088dd

Co-authored-by: Hofer-Julian <[email protected]>

Enforce new language syntax with pyupgrade rule

507d925

Fix sorting. Deleted node_types.

d4eadb5

evetion mentioned this pull request Nov 7, 2023

Support Arrow input tables #170

Closed

Fix all testmodels.

6b1d5fb

Fixed mypy and toml files.

bd34248

evetion marked this pull request as ready for review November 13, 2023 16:43

evetion added 2 commits November 13, 2023 18:02

Merged main.

ac3a45e

Add pydantic v2.

a50e453

evetion requested review from visr and Hofer-Julian November 13, 2023 17:17

Hofer-Julian reviewed Nov 13, 2023

View reviewed changes

pixi.toml Outdated Show resolved Hide resolved

evetion and others added 3 commits November 13, 2023 21:39

Fix examples notebook.

77270f6

Final mypy error.

b9b2120

Stop parameterizing Edge

be766f9

Make Node and Edge less generic.

124159b

Hofer-Julian approved these changes Nov 14, 2023

View reviewed changes

python/ribasim/ribasim/input_base.py Outdated Show resolved Hide resolved

Fix allocation_network_id.

24bd84e

visr approved these changes Nov 14, 2023

View reviewed changes

evetion merged commit 633d635 into main Nov 14, 2023

evetion deleted the feat/pyrefact branch November 14, 2023 15:13

visr mentioned this pull request Nov 14, 2023

rename ribasim.Database to ribasim.Network #786

Merged

visr mentioned this pull request Nov 14, 2023

update lockfile with pixi 0.7 #787

Merged

This was referenced Nov 16, 2023

support pydantic v2 #673

Closed

Start supporting pydantic v2 #382

Closed

This was referenced Nov 20, 2023

Migrate to Pydantic V2 Deltares/HYDROLIB-core#588

Closed

Allow pydantic v2 Deltares/GEOLib#152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul of Python architecture #731

Overhaul of Python architecture #731

evetion commented Nov 2, 2023 •

edited

Loading

visr commented Nov 2, 2023

evetion commented Nov 2, 2023 •

edited

Loading

visr commented Nov 2, 2023

Hofer-Julian left a comment

Hofer-Julian commented Nov 3, 2023

evetion commented Nov 3, 2023

visr commented Nov 7, 2023

evetion commented Nov 13, 2023

Hofer-Julian commented Nov 14, 2023

Hofer-Julian left a comment

visr left a comment

visr Nov 14, 2023

evetion Nov 14, 2023

visr Nov 14, 2023

evetion Nov 14, 2023

Overhaul of Python architecture #731

Overhaul of Python architecture #731

Conversation

evetion commented Nov 2, 2023 • edited Loading

Some notes:

Changes:

Yet to do:

visr commented Nov 2, 2023

evetion commented Nov 2, 2023 • edited Loading

visr commented Nov 2, 2023

Hofer-Julian left a comment

Choose a reason for hiding this comment

Hofer-Julian commented Nov 3, 2023

evetion commented Nov 3, 2023

visr commented Nov 7, 2023

evetion commented Nov 13, 2023

Hofer-Julian commented Nov 14, 2023

Hofer-Julian left a comment

Choose a reason for hiding this comment

visr left a comment

Choose a reason for hiding this comment

visr Nov 14, 2023

Choose a reason for hiding this comment

evetion Nov 14, 2023

Choose a reason for hiding this comment

visr Nov 14, 2023

Choose a reason for hiding this comment

evetion Nov 14, 2023

Choose a reason for hiding this comment

evetion commented Nov 2, 2023 •

edited

Loading

evetion commented Nov 2, 2023 •

edited

Loading