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

Use setup_requires to ensure cython and numpy are present #260

Closed
wants to merge 1 commit into from

Conversation

j-santander
Copy link

@j-santander j-santander commented Oct 31, 2016

Addressing issue #256
Summary of changes:

I'm using setup_requires to indicate the dependencies on "setup" time, with install_requires to indicate the dependencies on "run" time.

I'm also ensuring the setup_requires is evaluated before making any reference to them.

As numpy version I've used the declared dependency for Pandas 0.17.0

@codecov-io
Copy link

Current coverage is 87.25% (diff: 100%)

Merging #260 into master will increase coverage by 0.06%

@@             master       #260   diff @@
==========================================
  Files             5          5          
  Lines           414        416     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            361        363     +2   
  Misses           53         53          
  Partials          0          0          

Powered by Codecov. Last update 6d93b2f...9c661cc

@wesm
Copy link
Owner

wesm commented Nov 9, 2016

Need to make sure this doesn't conflict with conda environments -- setuptools has a way of wanting to build NumPy or Cython from source when it's already installed in your site-packages

@tdsmith
Copy link
Contributor

tdsmith commented Dec 1, 2016

A problem with using setup_requires this way is that, if they're missing, setuptools will install a temporary copy of cython and numpy for each setup.py invocation. Unfortunately, tools like pip will call setup.py several times at least twice to do things like extract metadata.

One solution is to cargo-cult this function around: https://github.com/pyca/cryptography/blob/8e66ca6813016d9fc6f57d5f1e50530fc39f78ae/setup.py#L114 -- the cryptography project runs into a similar chicken-egg problem with cffi.

Mercifully, this will be resolved in pip 10 by PEP 518 (pyproject.toml).

pip PEP 518 tracking bug: pypa/pip#3691

@tdsmith
Copy link
Contributor

tdsmith commented Dec 1, 2016

-- although the cryptography setup.py doesn't model a complete solution for feather since cryptography manages to avoid actually importing cffi directly, relying on cffi's setuptools extension hooks instead; packaging is hard.

@j-santander
Copy link
Author

Yes, I know :) Thanks for looking into this.

@danpalmer
Copy link

Ping on this? I just tried to install feather-format and it failed. Not a great first-run experience for someone new to the format.

It also means we can't use a requirements.txt, as pip doesn't know the dependency and therefore can't install numpy and Cython first.

# Force `setup_requires` stuff like Cython to be installed before proceeding
#
from setuptools.dist import Distribution
Distribution(dict(setup_requires=['cython>=0.21', 'numpy>=1.7.1']))

Choose a reason for hiding this comment

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

The correct name of the package is Cython – using the lowercase version will result in issues with some tooling as it is not the canonical name.

@wesm
Copy link
Owner

wesm commented Jul 6, 2017

Closing as Cython is no longer required to create the feather Python package (which is just a convenience interface to pyarrow.feather)

@wesm wesm closed this Jul 6, 2017
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.

5 participants