ubarsc / python-fmask

A set of command line utilities and Python modules that implement the ‘fmask’ algorithm
https://www.pythonfmask.org
GNU General Public License v3.0
75 stars 21 forks source link

Make python-fmask an official python package #30

Closed cpaulik closed 3 months ago

cpaulik commented 4 years ago

Would you consider making python-fmask and official python package?

This would be great since it

Happy to help if you have any questions.

neilflood commented 4 years ago

I would certainly consider it. I have wondered about this for a while, but it had not been a priority for me. I know nothing about how to do this, though, so if you would like to point me at the appropriate documentation, and the right approach, that would help things along.

I am a bit tied up right now, but in a couple of weeks I am likely to have time to learn new things, so I will look into it then.

gillins commented 4 years ago

I haven't looked into this previously as GDAL appeared to be broken on pip and it didn't really look it could be changed to support GDAL properly (didn't support pure C++ packages and their dependencies). Hence why we pushed conda-forge since it makes it easy to install pure C and C++ packages esp. on Windows.

That being said I'd be interested to get your thoughts on how this would work on pypi now. We would also have to do the same for RIOS (https://github.com/ubarsc/rios/) since python-fmask requires it.

I understand we would have to create binary packages ourselves to upload to PyPI? Is there an automated way to do this for all platforms? This is another reason why we went with conda-forge, but if there is an automated way to do this using CI's or some other cloud provider what would be great. We certainly can't be bothered running manual builds for multiple Python versions on different platforms.

Interested to hear your thoughts on the above. I know very little about pypi etc.

cpaulik commented 4 years ago

If you want to pre-compile the package then you can automatically upload python wheels to pypi.

There are projects which aim to make this easier (https://github.com/joerick/cibuildwheel). But I have never used them. Looking at the CI configuration of any major package could also give good ideas.

For Windows wheels of one of my packages I use appveyor to build the windows wheels. See configuration at https://github.com/TUW-GEO/pytesmo/blob/master/appveyor.yml

There is a guide for uploading to PyPI from travis.ci

For my personal benefit a simple source distribution would be enough since we work on Linux where compilation of the python fmask c extensions should not be a problem.

I don't know if the wheels work for a complicated package like gdal. But they do seem to work for packages like numpy and pandas which also have quite involved build processes. It might make sense to ask about that on the gdal github page.

cpaulik commented 4 years ago

I just asked this on the gdal dev mailing list and got two tips:

Rasterio builds wheels which include gdal as far as I can tell. https://github.com/rasterio/rasterio-wheels which is based on https://github.com/matthew-brett/multibuild

I might work on the gdal wheels if the devs agree.

neilflood commented 4 years ago

I am glad @gillins raised this point, as it is something which has always bothered me about pip with these kinds of packages. I don't use it myself, but I am aware that both rasterio and fiona will bring with them their own private binary of gdal, and I have always worried what happens when these are for different versions. If we do similarly, that would be 3 copies of gdal, potentially different. Should I be worried about this sort of thing?

gillins commented 4 years ago

I should point out that compilation of GDAL is difficult (esp. on Windows). What optional components should be included? GEOS? JPEG2000? HDF5? etc. How will we know we are providing everything the user might need?

Hence why I suspect conda-forge might be a better solution as there is one recipe that pretty much has everything enabled and already built successfully using CI infrastructure. Saves the duplication of effort (and libraries to download).

Is there any reason that we should recommend pip instead of conda-forge? I'll still thinking that pip is fine for small Python-only packages but fundamentally broken for more complex things like GDAL (hence why it got invented I guess). I'm happy to be talked around though :smile:

I'm certainly not against making it 'official' (whatever is involved with that) and making the source tarball available on PyPI (but not sure this helps non-expert users get GDAL, RIOS etc working on their PC)....

gillins commented 4 years ago

disclosure: I am one of the maintainers of the GDAL build on conda-forge so a bit biased. Feel free to ignore my comments!

cpaulik commented 4 years ago

I would not package gdal with python-fmask and I think we are talking about gdal too much.

The main problem that I see at this moment with python-fmask and also rios is that they are not python packages at all. They do not have their requirements specified in a way that pip would understand and they are not on PyPI. So If I have a package that depends on python-fmask and I try to pip install this package I get nothing useful. This might be a edge case but I still think there should at least be an error message if compilers or python-fmask dependencies are missing.

In my opinion this is separate from the fact that gdal is not on PyPi with all C dependencies.

eamanu commented 4 years ago

Hi, this is an interest issue. I was looking for a cloud mask for an application and I can see that python-fmask is very popular. I can help on package for Debian.

neilflood commented 4 years ago

@cpaulik So, I am a bit unsure what you are proposing. From what little I understand, one should use the install_requires= keyword in setup.py to specify pip dependencies. However, the only dependencies for rios are gdal and numpy. Since GDAL's python bindings require numpy anyway, and GDAL can't be installed via pip, it is not clear to me what we should specify.

I have also done some playing on Ubuntu 18.04, and using a venv with --system-site-packages still does not get to see the system-installed GDAL bindings. I don't really understand why, to be honest. So, I don't really know how this should be managed.

On Windows it would seem to be even worse.

Could you offer some guidance?

LiNinghui-AI commented 3 years ago

running build running config_cc unifing config_cc, config, build_clib, build_ext, build commands --compiler options running config_fc unifing config_fc, config, build_clib, build_ext, build commands --fcompiler options running build_src build_src building extension "_fillminima" sources building extension "_valueindexes" sources building data_files sources build_src: building npy-pkg config files running build_py running build_ext No module named 'numpy.distutils._msvccompiler' in numpy.distutils; trying from distutils customize MSVCCompiler customize MSVCCompiler using build_ext building '_fillminima' extension compiling C sources error: Unable to find vcvarsall.bat

I met this error when I used setup.py build to install the fmask. What should I do ?

gillins commented 3 years ago

fmask has some C sources so requires MSVC to be available when installing from source. See this article: https://wiki.python.org/moin/WindowsCompilers

Installing from source is particularly tricky under Windows, we would always recommend using conda-forge to get a working install as described here: http://www.pythonfmask.org/en/latest/#downloads

gillins commented 3 years ago

Works fine for me. Are you sure you are installing into a brand new environment (conda create -n myenv python-fmask) ? There could be conflicts with packages in an existing environment.

If your problems persist log an issue here: https://github.com/conda-forge/python-fmask-feedstock

LiNinghui-AI commented 3 years ago

Thanks a lot, the fmask has been installed successfully !

neilflood commented 3 years ago

@LiNinghui-AI Could you remove this question from the current issue, and open a separate issue with this question. It does not seem relevant to the original question being asked for Issue 30.

LiNinghui-AI commented 3 years ago

@LiNinghui-AI Could you remove this question from the current issue, and open a separate issue with this question. It does not seem relevant to the original question being asked for Issue 30.

Okay