pymatting icon indicating copy to clipboard operation
pymatting copied to clipboard

`setup.py` should define actual dependencies

Open jangop opened this issue 2 years ago • 5 comments

Currently, the packages specified in requirements.txt are copied into setup.py:

    install_requires=load_text("requirements.txt").strip().split("\n"),

This is bad practice and can cause problems downstream.

requirements.txt should be used to define a repeatable installation, such as a development environment or a production environment. As such, versions of dependencies contained therein should be as specific as possible.

install_requires should be used to indicate dependencies necessary to run the package. As such, versions of dependencies contained therein should be as broad as possible.

Please see “install_requires vs requirements files” on python.org or “requirements.txt vs setup.py” on stackoverflow for more information.

I'd be happy to contribute a PR with loose dependency specifications in setup.py and concrete specifications in requirements.txt.

jangop avatar Dec 19 '22 13:12 jangop

The requirements in requirements.txt are already quite broad. I hope they are also broad enough for install_requires.

Did you find any incompatibility issues? If not, I'd rather keep things as they are as to not break any downstream projects. Otherwise, someone would have to check whether downstream projects will survive the change. The largest projects I could find which use PyMatting are:

  • https://github.com/PaddlePaddle/PaddleSeg/blob/19351bab9a824a8f96e1c1b527ec2d7db21309c9/Matting/requirements.txt
  • https://github.com/danielgatis/rembg/blob/main/setup.py
  • https://github.com/nadermx/backgroundremover/blob/2a7421970591a12f57f275079e54febe9dd3c287/requirements.txt
  • https://github.com/open-mmlab/mmediting/blob/7245818cafa30cebdb1763e4a5751acf66c2db4c/setup.cfg#L22
  • https://github.com/itainf/aiphoto/blob/master/to_background/to_background.py
  • https://github.com/kfeng123/LSA-Matting/blob/master/tools/reestimate_foreground_final.py

99991 avatar Dec 19 '22 14:12 99991

You are right; in fact, requirements.txt is too broad with regards to numba. Any version earlier than 0.49.0 is allowed, not just any version after 0.49.0.

I came here going down the rabbit hole from https://github.com/danielgatis/rembg/issues/364. I believe pymatting is the problem, because of numba!=0.49.0.

Instead of simply changing requirements.txt I'd like to do a proper fix; see the links to python.org and StackOverflow above.

jangop avatar Dec 20 '22 07:12 jangop

Numba version 0.49.0 had been disabled because that specific version contained a bug which was triggered by our KDTree code. Version 0.48.0 did not have that bug. More details in issue https://github.com/pymatting/pymatting/issues/15

https://github.com/danielgatis/rembg/issues/364 looks to me like an issue with Numba. I had problems with incompatible Numba and llvmlite versions in the past, which looked similar. Perhaps this is related?

99991 avatar Dec 20 '22 10:12 99991

Numba version 0.49.0 had been disabled because that specific version contained a bug which was triggered by our KDTree code. Version 0.48.0 did not have that bug. More details in issue #15

Yes, that I did notice.

The problem is that numba!=0.49.0 allows very old versions, e.g., 0.0.1-alpha1, which is probably not what you want.

PEP 440 allows logical operators. We could specify numba>=0.48.0,!=0.49.0, where the comma , is interpreted as “and”.

danielgatis/rembg#364 looks to me like an issue with Numba. I had problems with incompatible Numba and llvmlite versions in the past, which looked similar. Perhaps this is related?

Yes, exactly. And because rembg does not use numba directly, the installed version is determined via pymatting, which leads to the reported issue.

Because pymatting -- according to setup.py -- supports Python 3.10, it would make sense to at least require numba>=0.55.0, which is the earliest version of numba to support Python 3.10.

jangop avatar Dec 20 '22 11:12 jangop

We could specify numba>=0.48.0,!=0.49.0

That is probably more conservative than necessary. It is likely that PyMatting will also work with earlier versions, although I do not have a list.

Because pymatting -- according to setup.py -- supports Python 3.10, it would make sense to at least require numba>=0.55.0, which is the earliest version of numba to support Python 3.10.

If numba<0.55.0 does not work with Python 3.10, then the Numba developers should probably declare that in their setup.py. For numba==0.54.0, they explicitly exclude Python 3.10.

I am not sure how numba==0.34.0 got installed here. It should default to the newest version. Perhaps that user had a different error message earlier and then tried to install libraries with random versions to fix it? The numba version 0.34.0 is suspiciously close to some recent llvmlite version, so perhaps there was a mixup.

But Linux Mint 18 is EOL anyway, so who knows what is going on there.

On Ubuntu 22.04, numba==0.53.0 llvmlite==0.36.0 numpy==1.23.5 works as tested with the shell commands below, unlike here, so I think the issue lies somewhere else.

docker run --rm -ti ubuntu:22.04
# Install things
apt update
export DEBIAN_FRONTEND=noninteractive
apt install -y software-properties-common
apt-add-repository -y ppa:deadsnakes/ppa
apt update
apt install -y python3.9 python3.9-distutils python3-pip
python3.9 -m pip install numba==0.53.0 llvmlite==0.36.0 numpy==1.23.5
# Check whether numba works
python3.9 -c 'from numba import prange, njit'
# Check whether rembg works with those library versions
python3.9 -m pip install rembg[gpu]
rembg --help && python3.9 -m pip freeze | grep 'llvmlite\|numba\|numpy'

Either way, that user's issue will likely also occur when just running python3.9 -c 'from numba import prange, njit', so this is unrelated to PyMatting.

I am still on the fence whether we should exclude extremely old Numba versions. The minimum Numba versions should be different for different Python installations, but we would duplicate work that the Numba developers already did.

99991 avatar Dec 21 '22 08:12 99991