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

Add hydrolib-core dependency #122

Closed
visr opened this issue Aug 1, 2024 · 6 comments · Fixed by #130
Closed

Add hydrolib-core dependency #122

visr opened this issue Aug 1, 2024 · 6 comments · Fixed by #130

Comments

@visr
Copy link
Member

visr commented Aug 1, 2024

I tried

pixi add --feature common --pypi hydrolib-core

But this reported conflicts with numpy and peilbeheerst_model. So we have to look into that first. And then add it as a ribasim_lumping dependency as well.

@Huite
Copy link

Huite commented Aug 2, 2024

See Deltares/HYDROLIB-core#657

visr added a commit that referenced this issue Aug 6, 2024
Moves ribasim_lumping from scripts to src, adds pyproject.toml and
readme. For the other packages we now all switch to using hatchling
building, and adding them all as editable dependencies to pixi.

Only ribasim_lumping doesn't load yet due to #122. Though this is good
to merge.
@visr
Copy link
Member Author

visr commented Aug 14, 2024

As an alternative we can remove the hydrolib code from ribasim_lumping, since we won't really be using it anytime soon anyway. We could always add it back later if needed. There is already plenty of code in this repository, and we shouldn't really maintain code that we don't use.

I did look into this a little bit and there are some parts where the hydrolib and hydamo code are interweaved, so it's a little tricky to untangle this.

@DanielTollenaar
Copy link
Collaborator

As an alternative we can remove the hydrolib code from ribasim_lumping, since we won't really be using it anytime soon anyway. We could always add it back later if needed. There is already plenty of code in this repository, and we shouldn't really maintain code that we don't use.

I did look into this a little bit and there are some parts where the hydrolib and hydamo code are interweaved, so it's a little tricky to untangle this.

Yes, think it is only used for NZV with a custom notebook in the cloud-storage. NZV Ribasim-model is derived from a DHYDRO model.

@visr
Copy link
Member Author

visr commented Aug 15, 2024

Oh right, does that mean we should retain that code for now? Or can we move NZV off of D-Hydro anytime soon?

@DanielTollenaar
Copy link
Collaborator

Oh right, does that mean we should retain that code for now? Or can we move NZV off of D-Hydro anytime soon?

I think we can't, but I suggest we postpone NZV a bit and first get the workflow for the other water-authorities running.

@visr
Copy link
Member Author

visr commented Aug 15, 2024

We should be able to install the current hydrolib just fine since we don't require NumPy 2 anywhere. There is no conflict if we first explicitly ask for NumPy 1, and then install the PyPI dependencies.

pixi add -f common numpy=1
pixi add -f common --pypi dfm_tools
pixi add -f common --pypi hydrolib

And then add the ribasim_lumping dependencies to its pyproject.toml:

    "dfm_tools",
    "hydrolib-core",

I didn't do this yet since I keep running into an error "Unsupported archive type: setuptools", which hopefully will be fixed soon: prefix-dev/pixi#1686 (comment)

visr added a commit that referenced this issue Aug 19, 2024
Fixes #122.

At this point we'll work with the already uploaded results of ribasim_lumping and patch those up for use with the current Ribasim version, like for instance done in #129.

That means for now we don't need to update this code to run. I don't delete the code because we probably want to use it later on and may need to look at the implementation. This does mean we cannot `import ribasim_lumping` within the pixi environment.
@visr visr closed this as completed in #130 Aug 22, 2024
visr added a commit that referenced this issue Aug 22, 2024
Fixes #122.

At this point we'll work with the already uploaded results of
ribasim_lumping and patch those up for use with the current Ribasim
version, like for instance done in #129.

That means for now we don't need to update this code to run. I don't
delete the code because we probably want to use it later on and may need
to look at the implementation. This does mean we cannot `import
ribasim_lumping` within the pixi environment.

Also updates dependencies with `pixi update`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants