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

some fixing of the tests #129

Merged
merged 18 commits into from
Sep 23, 2024
Merged

some fixing of the tests #129

merged 18 commits into from
Sep 23, 2024

Conversation

hboisgon
Copy link
Contributor

@hboisgon hboisgon commented Mar 22, 2024

Issue addressed

Tests were failing for several reasons and one of them was because of HydroMT:

After that I still get the error for Piave that the branches (rivers and pipes) and manholes geometry differ. I zoomed in in QGIS and could really see no differences at all...
But maybe good to double-check if I missed something. For now I skipped checking the geometry files.

Some notes:

  • Able to update hydromt, xugrid and meshkernel to latest versions thanks to the bugfix in get_mesh after xugrid update to 0.9.0
  • Able to update to latest pandas version
  • Able to update to hydrolib-core 0.7.0 but not yet 0.8.0 so I put a max version for now. 0.8.0 fails for something linked to this update from the hydrolib-core changelog: Raise error for unknown keywords in PR632
  • Xarray had quite some updates which causes issues also in hydromt core so for now I have a max version for xarray
  • In the meantime geopandas released their version 1 and hydromt core still need to adapt for it, so for now, one more pin on geopandas (and related pyogrio dependency to avoid installation error)
  • Also stopped testing for python 3.9. Not testing yet for python 3.12 as hydromt does not support it.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed -> not done, not sure if we should mention dependencies upgrades in the changelog

@hboisgon
Copy link
Contributor Author

@xldeltares : for your info, it's not really ready to merge yet but some things are fixed here

Copy link

sonarcloud bot commented Mar 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hboisgon hboisgon self-assigned this Sep 12, 2024
@hboisgon
Copy link
Contributor Author

I finally got the tests green again! Can you check and review @shartgring ?
Would be good if you can check that the built model with the new package version still runs in Delft3D-FM :)
I will leave updating to hydrolib-core 0.8.0 to another PR also because I may need some help there looking at the error I'm getting..

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Sep 19, 2024

I might be able to help with updating to newer hydrolib-core versions, since I was involved in the changes done there, and those in meshkernelpy.

@hboisgon
Copy link
Contributor Author

That would be great @veenstrajelmer ! I could update all but hydrolib-core. Feel free to do it in this PR or in a next one if you think it may be a lot of work.

@veenstrajelmer
Copy link
Collaborator

I hope it won't be a lot of work, but I guess it is wise to do it in a separate PR to keep the repos history clear.

Copy link
Collaborator

@shartgring shartgring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Whether to also restrict pandas to >= 2.2.0 I leave that to you, as I can assume that some other dependencies already account for this

hydromt_delft3dfm/workflows/boundaries.py Outdated Show resolved Hide resolved
@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Sep 20, 2024

The test are also green after removing some version restrictions:

  • add support for py3.12 >> local tests are green but cannot yet be added to ci because of Add support for python 3.12 #136
  • release xarray version restriction? >> all green (the xarray==2024.3.0 restriction comes already from hydromt 0.10.0, does not have to be duplicated here I guess)
  • release pyogrio version restriction? >> all green
  • remove upper geopandas version restriction? >> fails, reported in Support for geopandas>=1.0 #135
  • lower minimal pandas version? >> all green for pandas>=2.0.0 after expanding _TIMESTR dict. Lower versions raise different errors, but I guess 2.0.0 is old enough to support a wide range of users/platforms so I did not create a follow-up issue for this
  • update minimal meshkernel version

@hboisgon
Copy link
Contributor Author

The test are also green after removing some version restrictions:

  • add support for py3.12 >> local tests are green but cannot yet be added to ci because of Add support for python 3.12 #136
  • release xarray version restriction? >> all green (the xarray==2024.3.0 restriction comes already from hydromt 0.10.0, does not have to be duplicated here I guess)
  • release pyogrio version restriction? >> all green
  • remove upper geopandas version restriction? >> fails, reported in Support for geopandas>=1.0 #135
  • lower minimal pandas version? >> all green for pandas>=2.0.0 after expanding _TIMESTR dict. Lower versions raise different errors, but I guess 2.0.0 is old enough to support a wide range of users/platforms so I did not create a follow-up issue for this
  • update minimal meshkernel version

Thanks @veenstrajelmer for the extra changes!
For python 3.12, I think we need to wait on hydromt-core anyway as they do not support it yet. Maybe that's why the conda install faails?
Hydromt 0.10.0 does have a pin on xarray but I found with hydromt-wflow that the pin somehow was too strict for conda-forge to allow installing hydromt 0.10 and conda forge would instead use 0.9.4 which does not have a pin on xarray, thus leading to faulty install of hydromt-wflow from conda-forge... So I had to release hydromt-wflow with the pin as well (in that case I choose <= instead of ==). So I'm a little hesitant here. In a way there is not so maybe bugs related to that version of xarray so maybe the hydromt-delft3d-fm plugin would be alright anyhow without the pin? We can decide when we release but I dont know if hydromt core will still fix these kind of issues for version less than 1.0

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Sep 23, 2024

Hi @hboisgon, some additional remarks:

  • according to pyproject.toml of hydromt 0.10.0 py312 is supported (at least not excluded). The error was not dependency related: https://github.com/Deltares/hydromt_delft3dfm/actions/runs/10958049192/job/30427469470
  • I would advise against unnecessarily pinning dependencies, since it just works with newer versions without conda. Would you know with whom to request an additional hydromt release? Their main branch already dropped the xarray pin, so it does not require additional work. Also, 0.10.0 was also done after 1.0-alpha. maybe wise to also set <1 instead of <0.15, but not sure if 1-alpha is considered also <1. I will check this.

@veenstrajelmer
Copy link
Collaborator

veenstrajelmer commented Sep 23, 2024

Some minor improvements:

  • updated ci workflow
  • upinned numpy (already enforced by hydromt and hydrolib-core)
  • moved from hydromt<0.15 to hydromt<1 (this works correctly, locally with pip and also on github with conda)

Other insights:

  • indeed hydromt does not support python 3.12, but I do not understand where it comes from. I expect there is no reason to also enforce it in hydromt_delft3dfm since the testbank is green anyway. We might have to test the resulting models to be 100% sure.
  • hydromt might be able to release another pre v1 version, if no developments are required. I have created an issue to move to hydromt>=1 here: Support for hydromt>=1 #137

@veenstrajelmer veenstrajelmer linked an issue Sep 23, 2024 that may be closed by this pull request
2 tasks
Include timedelta bugfix in changelog
Copy link

sonarcloud bot commented Sep 23, 2024

Copy link
Collaborator

@shartgring shartgring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Only one thing: I've added the bugfix of the TimeDelta changes to the changelog. Should the other fixes also be added to changelog or not really as it does not relate directly to features of hydromt_delft3dfm?

@shartgring
Copy link
Collaborator

Looks good to me.

Only one thing: I've added the bugfix of the TimeDelta changes to the changelog. Should the other fixes also be added to changelog or not really as it does not relate directly to features of hydromt_delft3dfm?

I see now you mentioned this at the top @hboisgon and I agree so I will just merge it. So after my addition I think the changelog is complete

@shartgring shartgring merged commit 73b79bf into main Sep 23, 2024
6 checks passed
@shartgring shartgring deleted the fix-tests branch September 23, 2024 12:59
@veenstrajelmer veenstrajelmer mentioned this pull request Sep 23, 2024
5 tasks
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 this pull request may close these issues.

failed test due to dependency updates
3 participants