Skip to content
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

Pydantic model_dump(exclude_unset=True, ...) causes model.write to ignore mutation #811

Closed
Huite opened this issue Nov 21, 2023 · 2 comments · Fixed by #847
Closed

Pydantic model_dump(exclude_unset=True, ...) causes model.write to ignore mutation #811

Huite opened this issue Nov 21, 2023 · 2 comments · Fixed by #847
Labels
python Relates to one of the Ribasim python packages

Comments

@Huite
Copy link
Contributor

Huite commented Nov 21, 2023

Here's an MWE:

import ribasim_testmodels

model = ribasim_testmodels.trivial.trivial_model()
model.solver.saveat = 86400.0
model.write("here")

If you now check the TOML, I'd expect to see a non-default saveat entry, but instead:

starttime = 2020-01-01 00:00:00
endtime = 2021-01-01 00:00:00
database = "database.gpkg"

I believe this is due to the (expected) behavior of exlude_unset, see also:
pydantic/pydantic#5749

Creating a new object does work, probably because the solver entry is detected as being set by pydantic?

model = ribasim_testmodels.trivial.trivial_model()
model.solver = ribasim.Solver(saveat=86400.0)
model.write("here")

TOML:

starttime = 2020-01-01 00:00:00
endtime = 2021-01-01 00:00:00
database = "database.gpkg"

[solver]
saveat = 86400.0

Given that this seems expected behavior by pydantic, I'm not sure it's easy to fix (without dirty hacks).

A pragmatic solution might be something like this:

model.solver = ribasim.solver.update(saveat=86400.0)

The update method just returns a new object.

You can argue this is consistent with how you're supposed to work with pandas and xarray, where the inplace operations are mostly discouraged or removed; although you might as well make the object immutable in that case to avoid confusion?

@github-project-automation github-project-automation bot moved this to To do in Ribasim Nov 21, 2023
@visr visr added the python Relates to one of the Ribasim python packages label Nov 21, 2023
@Huite
Copy link
Contributor Author

Huite commented Nov 22, 2023

Another thought: this can be avoided entirely by not using exclude_unset. This then writes all the defaults to the TOML, but that's fine. I mostly prefer having the settings in that way, because it explicitly communicates what e.g. the solver settings are.

@evetion
Copy link
Member

evetion commented Nov 28, 2023

I'll try a fix by making a relationship between Model and its children.

@visr visr closed this as completed in #847 Nov 29, 2023
visr pushed a commit that referenced this issue Nov 29, 2023
Fixes #811 

More groundwork for #760, as doing `model.node.add()`, requires
accessing the `node._parent.network.`
@github-project-automation github-project-automation bot moved this from To do to ✅ Done in Ribasim Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Relates to one of the Ribasim python packages
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants