Skip to content

Comments

Package and testing updates and fixes#435

Open
jmcvey3 wants to merge 9 commits intoMHKiT-Software:developfrom
jmcvey3:package-cleanup
Open

Package and testing updates and fixes#435
jmcvey3 wants to merge 9 commits intoMHKiT-Software:developfrom
jmcvey3:package-cleanup

Conversation

@jmcvey3
Copy link
Contributor

@jmcvey3 jmcvey3 commented Feb 19, 2026

A general PR to cover tests that have been breaking recently to python and dependency updates. Also contains cleanup of the package in general

  1. Makes the caching code in utils/cache.py less opaque. To properly test cache code, one needs to delete the .cache/mhkit folder, wherever that is stored on your local
  2. Cleans up a new cache bug in wave/io/cdip.py
  3. Packaging updates:
    • Part of our CI troubles may be that dependencies are double-installed via the conda environment.yaml file and the pyproject.toml file. Best practice is to use the environment file for conda-specific packages and keep dependencies in the .toml file. Dependencies will come from conda if the mhkit package is installed via conda instead of pip.
    • One possible issue may come from the hdf5-related packages. I'm aware there can an issue with hdf5 C library filepaths if they are installed partly via conda and partly via pip.
    • Updates github actions CI versions

@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Feb 19, 2026

And the wind-hindcast-cache tests have a dtype error. Something was changed and now the data returns as float64 instead of float32

@jmcvey3 jmcvey3 requested review from akeeste and simmsa February 19, 2026 17:33
@simmsa
Copy link
Contributor

simmsa commented Feb 21, 2026

@jmcvey3 thank you for breaking this out into a separate PR.

There is a non obvious "test" here where we are verifying that the conda-forge install works. The original environment.yml is the same as this: https://github.com/conda-forge/mhkit-feedstock/blob/main/recipe/meta.yaml which is what conda-forge uses to build the conda version of mhkit.

Also important is that conda-forge uses this line:

{{ PYTHON }} -m pip install .[all] -vv --no-deps --no-build-isolation

to build the conda forge package.

TBH this is a bit of python packing magic that I don't fully understand. In lieu of understanding, the tests build mhkit using a similar workflow as conda forge and basically "verify" that conda-forge will work, or not work. I think we should maintain this pattern and restore the environment.yml to be the same as the conda forge version.

Having to define the same versions in two different files, pyproject.toml and environment.yml is really annoying, and maintaining a 3rd file in another repo even more annoying. I haven't seen any solutions for this in python, but maybe something better exists.

I think it is possible to autogen environment.yml from pyproject.toml to keep versions the same, so maybe that helps here. Or some other tool altogether.

Or maybe we need some other packaging solution that handles H5/HDF5 binaries better.

I'd be happy to try to pick this up, and try to get the tests working. Let us know what makes sense here.

@jmcvey3
Copy link
Contributor Author

jmcvey3 commented Feb 21, 2026

@simmsa Thank you for the background here, this is starting to ring a bell

So to be clear, the only reason the extra dependencies are in environment.yml is to test and make sure the conda-forge meta.yaml will function properly?

If so, should we move the original environment.yml file into something like "mhkit/tests/conda-forge/enviroment.yml" and leave a barebones environment.yml file in the main repo directory? That way we can refer to the former in the "conda-ubunut-latest" tests and the latter in the "pip-ubuntu-latest" tests and separate them properly?

@@ -54,8 +56,6 @@ river = [
]

dolfyn = [
Copy link
Contributor Author

@jmcvey3 jmcvey3 Feb 21, 2026

Choose a reason for hiding this comment

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

I also saw that the h5py libraries are set here, but dolfyn doesn't actually require any of the h5-related libraries. These might belong under the wave module dependencies

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.

3 participants