ASFHyP3 / hyp3-gamma

HyP3 plugin for generating SAR products with GAMMA
BSD 3-Clause "New" or "Revised" License
30 stars 6 forks source link

upgrade to GAMMA 20231208 #558

Closed asjohnston-asf closed 3 months ago

asjohnston-asf commented 3 months ago

Build of the python environment succeeds locally and in the docker build, but fails in the reusable pytest action because of the --no-deps option during the pip install. We could consider removing --no-deps from the action, I'm not sure there was a specific use case that we enabled it for. See https://github.com/ASFHyP3/actions/pull/10

jhkennedy commented 3 months ago

@asjohnston-asf --no-deps prevents a mismatch in expectations -- it makes it so conda is solely responsible for the dependencies instead of the final pip install changing dependencies out from under you.

It looks like what you're seeing are pandas and scipy dependency issues as we're constraining numpy to an old version. I'm not sure why conda would be allowing that, however.

jhkennedy commented 3 months ago

Ugg, looks like conda-forge isn't pinning the numpy version for pandas: https://github.com/conda-forge/pandas-feedstock/blob/main/recipe/meta.yaml

Similarly, the scipy version pinning seems to be broader than the error message indicates: https://github.com/conda-forge/scipy-feedstock/blob/main/recipe/meta.yaml

jhkennedy commented 3 months ago

Creating a fresh environment locally via:

mamba env create -f environment.yml
conda activate hyp3-gamma
python -m pip install -e .

I see pip downgrading a number of the conda installed packages: image

And looking at the environment, these packages are installed via pip instead of conda-forge:

$ conda list | grep pypi
geopandas                 0.14.3                   pypi_0    pypi
hyp3-gamma                8.1.3.dev8+g02d86a0          pypi_0    pypi
pandas                    2.0.3                    pypi_0    pypi
scipy                     1.11.4                   pypi_0    pypi
statsmodels               0.14.1                   pypi_0    pypi

So, we should probably put version constraints on these packages in the conda environment.

jhkennedy commented 3 months ago

So, since hyp3-gamma is installed into the system python, and all the python dependencies are apt installed, I think it might make the most sense to pip freeze > requirements.txt inside the container and use that to create the conda environment so that they are coherent. That'd mean changing the environment file to:

name: hyp3-gamma
channels:
  - conda-forge
  - nodefaults
dependencies:
  - python=X.X.X
  - pip
  - libdal=X.X.X
  - pip:
    - -r requirements.txt

with the X.X.X version matching what's in the container. This way, pip will always manage all the dependencies (except libgal, which is a pain) and we'll better ensure we have the same environment inside and outside the container.

The other option is just to run the tests inside the container, but that's a little harder on us where we want to easily and quickly tests things locally without having to build the container.