Martinsos / edlib

Lightweight, super fast C/C++ (& Python) library for sequence alignment using edit (Levenshtein) distance.
http://martinsos.github.io/edlib
MIT License
514 stars 167 forks source link

Installation error with pip #64

Closed ksahlin closed 7 years ago

ksahlin commented 7 years ago

Hi Martin,

I get the following error (tried both python 2.7.9 and 3.4.3)

$ pip install edlib
Downloading/unpacking edlib
  Downloading edlib-1.1.2.tar.gz
  Running setup.py (path:/private/var/folders/6n/tw49pymn3jq4khf0jsv97vmr0000gp/T/pip_build_kxs624/edlib/setup.py) egg_info for package edlib
    Traceback (most recent call last):
      File "<string>", line 17, in <module>
      File "/private/var/folders/6n/tw49pymn3jq4khf0jsv97vmr0000gp/T/pip_build_kxs624/edlib/setup.py", line 28, in <module>
        cmdclass = {'build_ext': Cython.Build.build_ext}
    AttributeError: 'module' object has no attribute 'build_ext'
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):

  File "<string>", line 17, in <module>

  File "/private/var/folders/6n/tw49pymn3jq4khf0jsv97vmr0000gp/T/pip_build_kxs624/edlib/setup.py", line 28, in <module>

    cmdclass = {'build_ext': Cython.Build.build_ext}

AttributeError: 'module' object has no attribute 'build_ext'

Do you have any idea on how I can fix this?

Thanks, Kristoffer

ksahlin commented 7 years ago

I just noted the line

setup_requires = ['cython (>=0.25)'],

in setup.py and indeed I had an older version. I'm unsure why pip didn't detect this, but it may potentially be fixed if changed to

install_requires = ['cython (>=0.25)'],

In order to get the installation to work with pip.

I managed to install edlib with python setup install by modifying some lines in the setup (see pull request #65)

Martinsos commented 7 years ago

@ksahlin thanks for reporting the issue and helping out with it! This is my first time that I wrote a Python extension so I most likely didn't get everything right, I appreciate any help.

So this seems to be a pretty tricky problem - the thing is, we need Cython in order to execute the cmdclass = {'build_ext': Cython.Build.build_ext} line in setup.py, however setup_requires will not install Cython on time. setup_requires is they way to go however, as it specifies the dependencies needed to install the package. install_requires specifies the dependencies needed to run the package, which is not what we need here (and even if we used it, again Cython would not be available at needed moment).

I see that others are also struggling with this: https://github.com/pypa/pip/issues/1820, https://github.com/jwilk/python-afl/issues/3, http://stackoverflow.com/questions/18023546/marking-cython-as-a-build-dependency. However, most of the solutions are not so nice hacks, and I would like to avoid that if possible.

Finally, I found this post, which says that there is special support for this in setuptools since version 18.0!

Also, I found this other answer (much older, from 2010) which says that Cython should not be a dependency, but that instead .c files generated by Cython should be distributed. Then, if no Cython is installed on user's machine, .c files should be used, otherwise (if Cython is installed) .pyx files should be used, so that user can easily play with the source code.

If we go with the second one, we will need to make sure that .c file(s) are generated when building source distribution (I can probably do that easily in my Makefile, or using suggested approach in the same SO answer). However, I wonder following: if second approach is better, why did they add special support in setuptools for cython as a build dependency (first approach)?

I am still not sure which way to go - I like the first approach better since it is more straight forward and adds less complexity, however second approach does not force user to install cython, which may be important in some cases - but should we really worry about that?

I will take some time to test both and see how they work. In the meantime, I would love to hear your opinion on this!

Martinsos commented 7 years ago

I implemented the first approach (using special setuptools support) and it seems to work fine: 324893949211247db0ba312f9ae5bc1791905217. I published a new version on PyPI and tested it on different machine and it worked. Could you also try it and confirm that it works fine? The only thing now is that compilation process takes a long time (~half a minute or more) when installing from pip (not sure why since it takes about a second when building package locally). I am still not sure if this is the best approach or if I should use the second one instead - waiting to hear what you think about it.

ksahlin commented 7 years ago

I can confirm that pip install edlib works fine. I tested with python 2.7 and 3.4 on OSX. As for best practice regarding the Cython installation dependency I don't have enough knowledge on this particular topic to defend any opinion I might have :)

Regarding the installation time, do you think it's because setuptools downloads and installs a temporary version of Cython to be able to install the package?

Martinsos commented 7 years ago

Awesome! That should be it then - I will also add tests to Travis later, to be sure that it does not break again. Regarding speed - might be, I am not sure. When running pip install locally from the source distribution, it takes some time to install Cython dep, but it also takes some time to compile all together, so I guess it is both. I will try the second approach and see if it makes any difference speed-wise.

ksahlin commented 7 years ago

great, thanks!

Martinsos commented 7 years ago

I tried second approach (pre-compiling cython files) and it is super fast when installing, as it should be. I decided to go with it, since it seems more simple and robust, and process is faster and simpler on the user's side with small complexity added on our side. Commit b9f1ad50bd691b436bb0de1a1956ceb99e992775