Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require numpy and specify this in setup.py #1307

Merged
merged 7 commits into from Jul 5, 2017

Conversation

peterjc
Copy link
Member

@peterjc peterjc commented Jul 3, 2017

This pull request makes NumPy a hard requirement (except on Jython and IronPython). This in itself massively simplifies the setup.py file - and once we drop Jython (and IronPython) we can go further.

This does mean it is no longer going to be possible to install Biopython on PyPy or C Python without NumPy - currently allowed via a user interaction.

Having made that change, and given we've dropped distutils in favour of assuming setuptools will be present, as per pypa/packaging-problems#78 and https://caremad.io/posts/2013/07/setup-vs-requirement/ we should specify our (now strict) dependency on NumPy via setup(..., install_requires=...) rather than requirements.txt (see #987).

I believe this means we after this release is on PyPI be able to simplify the current README installation instructions from:

pip install numpy
pip install biopython

to just:

pip install biopython

This should address #1302 and should more fully resolve #978.

Copy link
Contributor

@etal etal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. Is the sys.exit necessary? What are the consequences of leaving it out?

@peterjc
Copy link
Member Author

peterjc commented Jul 3, 2017

Well it used to be sys.stderr.write(...) and sys.exit(-1) so by doing sys.exit(msg) its going to give a non-zero exit value of one. That could have side effects so reverting to the explicit sys.exit(-1) is OK with me.

@peterjc
Copy link
Member Author

peterjc commented Jul 3, 2017

Note to self: We should as part of this remove requirements.txt from MANIFEST.ini, I'd do it now but my connection is poor right now.

@etal
Copy link
Contributor

etal commented Jul 3, 2017

sys.exit(msg) sounds fine, the exit code is nonzero either way. Just wondering how much we can rely on setuptools/pip to complain about a missing dependency, versus doing the check ourselves.

@codecov
Copy link

codecov bot commented Jul 4, 2017

Codecov Report

Merging #1307 into master will increase coverage by 0.87%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
+ Coverage   83.35%   84.23%   +0.87%     
==========================================
  Files         317      318       +1     
  Lines       48655    48712      +57     
==========================================
+ Hits        40558    41031     +473     
+ Misses       8097     7681     -416
Impacted Files Coverage Δ
Bio/Wise/__init__.py 78.94% <0%> (-1.76%) ⬇️
Bio/phenotype/pm_fitting.py 82.45% <0%> (ø)
Bio/Phylo/BaseTree.py 89.39% <0%> (+0.19%) ⬆️
Bio/SeqIO/UniprotIO.py 92.21% <0%> (+0.29%) ⬆️
Bio/NeuralNetwork/Gene/Schema.py 71.9% <0%> (+0.82%) ⬆️
Bio/phenotype/phen_micro.py 85.22% <0%> (+2.22%) ⬆️
Bio/Phylo/_utils.py 66.25% <0%> (+36.25%) ⬆️
Bio/codonalign/codonseq.py 87.75% <0%> (+43.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c759829...34ae75a. Read the comment docs.

Copy link
Member

@bow bow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok @peterjc , I did a local install test on a Linux box and installation feels smoother now.

@peterjc
Copy link
Member Author

peterjc commented Jul 4, 2017

Thanks @bow - that's good to hear.

In my local testing using Python 3.6, pip3 install . will now install NumPy if missing - while python3 setup.py install will abort with the reworded message:

$ python3 setup.py install ; echo "Return code $?"
running install
Missing required dependency NumPy (Numerical Python).

Unless running under Jython or IronPython, we require NumPy be installed
when compiling Biopython. See http://www.numpy.org for details.

Return code 1

vs:

$ pip3 install .
Processing /Users/pc40583/repositories/biopython
Collecting numpy (from biopython==1.70.dev0)
  Using cached numpy-1.13.0-cp36-cp36m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl
Installing collected packages: numpy, biopython
  Running setup.py install for biopython ... done
Successfully installed biopython-1.70.dev0 numpy-1.13.0

Note thanks to the NumPy wheels, installing NumPy is very fast.

Cope with it being missing on Jython or IronPython.
Now that we've dropped distutils in favour of using
setuptools we can provide the install_requires list.

For a library this is prefered over requirements.txt
(which is intended more for an application).
Since we now require setuptools, single-version-externally-managed
should be there already.
@peterjc
Copy link
Member Author

peterjc commented Jul 4, 2017

@etal It seems that pip will enforce the install_requires= metadata early on, and takes care of installing NumPy smoothly. However, ...

If I remove our dependency check functions (which means install_biopython and build_ext_biopython are no longer needed - the base classes are enough), it makes no difference to the pip install . route. However python3 setup.py install will attempt to install NumPy. That is not wheel aware, so attempts to compile NumPy from source which is slow, and for some reason currently fails on my machine.

i.e. Right now our sublcassed methods like build_ext_biopython are still useful safety net for people using the python setup.py install route, so I think we should leave them in place for the next release.

(i.e. I'm being cautious - we're already changing a lot in the setup.py file)

After Biopython 1.70 is out, and assuming pip works perfectly in the field, we can remove all references to using python setup.py install and focus the documentation on using pip only. And then we can remove the dependency checks in our setup.py file :)

Now that we insist on NumPy (expect on Jython/IronPython),
setup.py will not prompt for continuing if NumPy is missing.
@peterjc peterjc merged commit 20a899b into biopython:master Jul 5, 2017
@peterjc
Copy link
Member Author

peterjc commented Jul 5, 2017

Thanks everyone - this is another step towards a modern installation setup 👍

@peterjc peterjc deleted the req_numpy branch July 5, 2017 13:38
@Cediddi
Copy link

Cediddi commented Oct 30, 2017

I have a feedback. I'm very glad that installing biopython is now works flawlessly no matter what. But there's a very small roughness left. I think you should specify this on your readme.

BioLinux is a fork of ubuntu 14.04LTS which comes with python3.4, setuptools 3.3 and pip 1.5.4 (ancient!). When you create a virtual environment and try installing biopython, pip doesn't check the install_requires statement, thus throwing an error. But if you upgrade pip there's no problem at all.

I think a simple suggestion of "Consider upgrading pip before installing biopython" would be great.

pip install pip --upgrade

This was no issue for me as I always keep pip and setuptools up to date, faced with this issue many times before with so many packages.

👍 Great work.

@peterjc
Copy link
Member Author

peterjc commented Oct 30, 2017

@Cediddi do you know when pip started checking install_requires? https://pip.pypa.io/en/stable/news/ does not mention it directly.

i.e. How old does pip need to be before it is worth issuing a warning, or even aborting?

@Cediddi
Copy link

Cediddi commented Oct 30, 2017 via email

@peterjc
Copy link
Member Author

peterjc commented Nov 9, 2017

I skimmed over https://pip.pypa.io/en/stable/news/ and there are so many wheel or other important changes that it is hard to know at what point pip is going to be too old.

Would picking an arbitrary threshold like pip version 8.0.0 seem like a good compromise (released 2016-01-19)? i.e. Abort if pip is older than this, abort with a message saying pip is too old, try pip install pip --upgrade first.

(We can of course easily make that check stricter if and when we realise we really do need an even newer version of pip)

@Cediddi
Copy link

Cediddi commented Nov 9, 2017

Just created two virtual environments one with pip==8.0.0 other with pip==9.0.1 (that hotfix is important)

Pip 8 downloaded biopython-1.70.tar.gz and numpy-1.13.3.zip and started compiling numpy and then installed biopython without an error. (so not supporting wheels is not the issue after all)

Pip 9 downloaded biopython-1.70-cp34-cp34m-manylinux1_x86_64.whl and numpy-1.13.3-cp34-cp34m-manylinux1_x86_64.whl and installed without a problem and was faster because not compiling the packages.

I think Pip <6 should abort installation, Pip >6,<9 should warn and say "You should consider upgrading via the 'pip install --upgrade pip' command for using precompiled wheels". If I had no gcc laying around, Pip 9 would still work I presume.

PS: I also tried Pip 7 and it worked just like Pip 8. Pip 6 also worked but gave a warning UserWarning: Unknown distribution option: 'python_requires'

PPS: Pip jumped from 1.5.6 to 6.0 I didn't bothered with 1.5.6.

PPPS: You can actually do something very interesting with pip 6 and onwards:

try:
    from pip.utils.outdated import pip_version_check
    pip_version_check(None)  # you can actually send your pip session here
except ImportError:
    raise Exception("Your pip is prehistoric, please upgrade your pip")

@peterjc
Copy link
Member Author

peterjc commented Nov 10, 2017

Your plan sounds good @Cediddi, could you open a new issue for that proposal please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip throws error when installing.
4 participants