jreback
commented
5 years ago +1 on this; sure could do this for 0.25.2
Closed TomAugspurger closed 5 years ago
jreback
commented
5 years ago +1 on this; sure could do this for 0.25.2
WillAyd
commented
5 years ago Makes sense to me. I suppose has the advantage of making these distributions smaller too, right?
jbrockmendel
commented
5 years ago Requiring Cython on top of that is not a big deal.
I don't think this is a problem, but it's worth mentioning that we tend to bump cython min-version relatively frequently in part because its not a requirement.
TomAugspurger
commented
5 years ago I don't think this is a problem
I also don't think that's a problem. With pip's build isolation, a pip install --no-binary=pandas pandas should pull down a completely independent environment. So you should still be able to have an older Cython in your application / library's dependencies even when you're building pandas from source.
jorisvandenbossche
commented
5 years ago I am fine with no longer shipping the cythonized files, but wondering if it is a good idea to do this in a bug fix release ..
I suppose the reason to actually do it now already for 0.25.2 is because this might help for python 3.8, and 0.25.2 will probably be the first release to support this?
TomAugspurger
commented
5 years ago I suppose the reason to actually do it now already for 0.25.2 is because this might help for python 3.8,
I share that concern, and yes.
So if we don't want to backport this, we would just need to ensure that the environment used to build the sdist has a new enough Cython (which I've messed up in the past). Overall, I think it's best to remove, even in the bugfix release.
jorisvandenbossche
commented
5 years ago Could we also remove the files and add cython to setup_requires, and backport this, and keep the pyproject.toml for 1.0 ? (as I think it is mainly this that can give troubles) Or is that only making a bigger mess?
TomAugspurger
commented
5 years ago Do you know what setuptools / pip does if the user has a requirements.txt requiring a version of Cython that conflicts with what we would put in our setup_requires? https://setuptools.readthedocs.io/en/latest/setuptools.html says
Note: projects listed in setup_requires will NOT be automatically installed on the system where the setup script is being run. They are simply downloaded to the ./.eggs directory if they’re not locally available already.
I'm trying it locally, and thing seem ok using setup_requires. This is all confusing though.
TomAugspurger
commented
5 years ago I'm trying it locally, and thing seem ok using setup_requires. This is all confusing though.
I confused myself. setup_requires does not work.
With these changes
diff --git a/setup.py b/setup.py
index d2c6b18b8..42737c4d1 100755
--- a/setup.py
+++ b/setup.py
@@ -32,18 +32,19 @@ def is_platform_mac():
min_numpy_ver = "1.13.3"
+min_cython_ver = "0.28.2"
setuptools_kwargs = {
"install_requires": [
"python-dateutil >= 2.6.1",
"pytz >= 2017.2",
"numpy >= {numpy_ver}".format(numpy_ver=min_numpy_ver),
],
- "setup_requires": ["numpy >= {numpy_ver}".format(numpy_ver=min_numpy_ver)],
+ "setup_requires": ["numpy >= {numpy_ver}".format(numpy_ver=min_numpy_ver),
+ "Cython >= {}".format(min_cython_ver)],
"zip_safe": False,
}
-min_cython_ver = "0.28.2"
try:
import Cython
@@ -526,6 +527,8 @@ def maybe_cythonize(extensions, *args, **kwargs):
# Avoid running cythonize on `python setup.py clean`
# See https://github.com/cython/cython/issues/1495
return extensions
+ elif "sdist" in sys.argv:
+ return extensions
if not cython:
# Avoid trying to look up numpy when installing from sdist
# https://github.com/pandas-dev/pandas/issues/25193I build an sdist with python setup.py sdist. I verify that the compiled files are not present.
And install
The relevant parts
UPDATING build/lib.macosx-10.14-x86_64-3.7/pandas/_version.py
set build/lib.macosx-10.14-x86_64-3.7/pandas/_version.py to '0.25.0+229.g9979b757f'
running build_ext
pandas._libs.algos: -> [['pandas/_libs/algos.c']]
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-req-build-hrpsio9k/setup.py", line 841, in <module>
**setuptools_kwargs
File "/Users/taugspurger/Envs/tmp2/lib/python3.7/site-packages/setuptools/__init__.py", line 145, in setup
return distutils.core.setup(**attrs)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/core.py", line 148, in setup
dist.run_commands()
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/dist.py", line 966, in run_commands
self.run_command(cmd)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/dist.py", line 985, in run_command
cmd_obj.run()
File "/Users/taugspurger/Envs/tmp2/lib/python3.7/site-packages/setuptools/command/install.py", line 61, in run
return orig.install.run(self)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/command/install.py", line 545, in run
self.run_command('build')
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/cmd.py", line 313, in run_command
self.distribution.run_command(command)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/dist.py", line 985, in run_command
cmd_obj.run()
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/command/build.py", line 135, in run
self.run_command(cmd_name)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/cmd.py", line 313, in run_command
self.distribution.run_command(command)
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/dist.py", line 985, in run_command
cmd_obj.run()
File "/usr/local/Cellar/python/3.7.3/Frameworks/Python.framework/Versions/3.7/lib/python3.7/distutils/command/build_ext.py", line 340, in run
self.build_extensions()
File "/private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-req-build-hrpsio9k/setup.py", line 408, in build_extensions
self.check_cython_extensions(self.extensions)
File "/private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-req-build-hrpsio9k/setup.py", line 403, in check_cython_extensions
src=src
Exception: Cython-generated file 'pandas/_libs/algos.c' not found.
Cython is required to compile pandas from a development branch.
Please install Cython or download a release package of pandas.
----------------------------------------
Can't roll back pandas; was not uninstalled
Command "/Users/taugspurger/Envs/tmp2/bin/python3 -u -c "import setuptools, tokenize;__file__='/private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-req-build-hrpsio9k/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record /private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-record-8nv7lqgz/install-record.txt --single-version-externally-managed --compile --install-headers /Users/taugspurger/Envs/tmp2/include/site/python3.7/pandas" failed with error code 1 in /private/var/folders/hz/f43khqfn7b1b1g8z_z6y3bsw0000gp/T/pip-req-build-hrpsio9k/So Cython apparently isn't installed (or if it is, it's not available for import).
TomAugspurger
commented
5 years ago And if we can't import it, we can't cythonize it. I think we would need to add Cython to install_requires as well, but I really don't think we want to do that (especially when pyproject.toml is supposed to fix this).
So tldr: I don't think setup_requires is a solution. I think our options are
min_cython_versionpip install pandas still works.
rgommers
commented
5 years ago That sounds about right to me. Without pyproject.toml you'll need to use setup_requires (no need for install_requires I think), but that invokes easy_install because pip doesn't support setup_requires natively. Which would not be good in 2019....
pyproject.toml does have the potential to create problems, a little risky in a bugfix release. you could also go with just being careful for 0.25.2, and if another cython fix >0.29.13 is needed then do a 0.25.3
jorisvandenbossche
commented
5 years ago you could also go with just being careful for 0.25.2, and if another cython fix >0.29.13 is needed then do a 0.25.3
Yes, that's what we decided in the dev call we just had, I think. For 0.25.2 do what we have done in the past (ship cythonized files), and then on master (for 1.0) add pyproject.toml again, which also enables to properly tackle this issue of needing cython in the build step.
TomAugspurger
commented
4 years ago For reference, this was done on the 1.0.0rc with https://github.com/pandas-dev/pandas-release/commit/62a2a728240464b5edd4a8f34d1a72e3bca88945
NumPy is considering this in https://github.com/numpy/numpy/pull/14453. I suspect other projects will follow suite.
The tldr is that as long as we ship the cythonized code, our older sdists have no hope of working with newer Pythons. The have to be compiled with a modern enough Cython.
I think this makes even more sense for us. We already require that NumPy is present as a build dependency. Requiring Cython on top of that is not a big deal.
Do we want to do this for 0.25.2?