Remove compiled Cython files from sdist #28341
Comments
|
+1 on this; sure could do this for 0.25.2 |
|
Makes sense to me. I suppose has the advantage of making these distributions smaller too, right? |
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. |
I also don't think that's a problem. With pip's build isolation, a |
TomAugspurger
added a commit
to TomAugspurger/pandas
that referenced
this issue
Sep 10, 2019
* Adds pyproject.toml to support this. * Bumps minimum Cython version to the version supporting Python 3.8. Closes pandas-dev#28341
|
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? |
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. |
|
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? |
|
Do you know what setuptools / pip does if the user has a
I'm trying it locally, and thing seem ok using |
I confused myself. 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 And install The relevant parts So Cython apparently isn't installed (or if it is, it's not available for import). |
|
And if we can't import it, we can't cythonize it. I think we would need to add Cython to So tldr: I don't think setup_requires is a solution. I think our options are
|
|
That sounds about right to me. Without
|
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. |
proost
pushed a commit
to proost/pandas
that referenced
this issue
Dec 19, 2019
proost
pushed a commit
to proost/pandas
that referenced
this issue
Dec 19, 2019
|
For reference, this was done on the 1.0.0rc with pandas-dev/pandas-release@62a2a72 |
jason-ytsao
pushed a commit
to jason-ytsao/exp_vtb
that referenced
this issue
Aug 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
NumPy is considering this in numpy/numpy#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?
The text was updated successfully, but these errors were encountered: