jgrss / geowombat

GeoWombat: Utilities for geospatial data
https://geowombat.readthedocs.io
MIT License
184 stars 10 forks source link

Newer versions (> 1.2.0) do not work with Intel OpenMP installed #230

Closed DManowitz closed 1 year ago

DManowitz commented 1 year ago

On conda-forge, I see that all versions of geowombat > 1.2.0 require llvm-openmp. This causes a problem if intel-openmp is installed in an environment. Would it be possible to make versions of geowombat (since at least 2.0.3, since I'm stuck at gdal 3.5.0) that either don't require llvm-openmp or use intel-openmp instead?

jgrss commented 1 year ago

Thanks for raising this @DManowitz. Is this issue related to what you are seeing?

Any thoughts @mmann1123?

DManowitz commented 1 year ago

That other issue is a similar issue on MacOS. I'm on Windows, and there, llvm-openmp installs Library/bin/libiomp5md.dll, which completely clobbers the version of the file installed by intel-openmp (see this issue)

Also, I installed another package which required me to downgrade gdal to 3.4.3, so if you make this change as far back as 1.8.6, I'd appreciate it.

mmann1123 commented 1 year ago

This is getting over my head. But I did find this discussion. There might be some argument for switching to the intel build for integration with pytorch

https://github.com/conda-forge/conda-forge.github.io/issues/1597

DManowitz commented 1 year ago

How is OpenMP used in geowombat? I performed a test where I first installed geowombat over pytorch. I then got an error when I tried to import pytorch. However, I then force-removed both llvm-openmp and intel-openmp, and then reinstalled llvm-openmp first, followed by intel-openmp (so the conflicting file from intel-openmp would clobber the file from llvm-openmp), and I was able to get both pytorch and geowombat (and all of geowombat's top-level files) to import without errors.

mmann1123 commented 1 year ago

Outside of pytorch it is only used to run resize in the radiometry module. If pytorch is moving to intel-openmp, I can't see a strong argument against using it instead of llvm. I defer to @jgrss, who may be in the throws of an intercontinental move.

jgrss commented 1 year ago

We should be able to remove OpenCV usage (can replace with numpy/scipy) from geowombat and get rid of the OpenMP dependency.

jgrss commented 1 year ago

@mmann1123 Can you remove OpenMP from the Conda install if you use #232?

mmann1123 commented 1 year ago

@jgrss yes I will. Flying solo with sick kids. I'll put it in my list.

mmann1123 commented 1 year ago

Conda is balking. Looks like you have a reference to opencv in the setup.cfg. I think it building some requirements from there?

setup.cfg opencv-python>=4.6.*

jgrss commented 1 year ago

I am not seeing OpenCV in the setup.cfg. However, I forgot to remove a reference to an OpenCV import in the surface reflectance module. Tests are re-running over at #232 on branch jgrss/openmp

mmann1123 commented 1 year ago

Its under install_requires = as opencv-python>=4.6.*

jgrss commented 1 year ago

It's on the main branch but not on jgrss/openmp.

jgrss commented 1 year ago

@mmann1123 release v2.0.18 is available to test.

mmann1123 commented 1 year ago

Ok this should be resolved. Just merged an update to the conda-forge feedstock. All tests passing. @jgrss @DManowitz

jgrss commented 1 year ago

I am going to close this, but feel free to reopen, @DManowitz, if the issue persists.