Closed kalekundert closed 10 months ago
Thanks for all the work you put into this! Everything looks good to me but I am not an expert on python packaging, so I'd like @mariogeiger to have a look at this before merging. I think it was also Mario who handled up the PyPi distribution, which will need to be updated.
Any update on this? Even if the PyPI distribution can't be updated immediately, it'd be nice to be able to install from GitHub.
I totally support this PR. This problem impedes usage of escnn
with more recent versions of python. Would be great to get this resolved (either with this PR or in any other way).
Merged! Sorry for the delay
No worries! Let me know if there are any issues that come up with the packaging, and I'll be happy to fix them.
TLDR: This PR will make it so that
pip install lie_learn
works again. In the meantime, here are the commands needed to install this package:It's currently very difficult to install
lie_learn
in an up-to-date python environment. It turns out that there are multiple reasons for this:The first relates to pip, and is briefly described here. Before version 23.1, pip would attempt each of the following steps (in order) when trying to install a package:
pyproject.toml
.python -m build
, if thewheel
package is installed.python setup.py install
.lie_learn
does not specify a build backend, and most people don't havewheel
installed, solie_learn
is usually installed viapython setup.py install
. Starting in version 23.1, though, pip no longer attempts the last step and instead produces the following (very cryptic) error message. The work-around is to manually installwheel
, or downgrade pip, but of course it would be better if this weren't necessary.The second reason relates to python itself. The most recent release of
lie_learn
on PyPI includes a source distribution and binary distributions for python 3.6–3.8 (Linux and Mac). Users of python 3.6–3.8 are fine, because they can install the binary distributions. Users of python 3.9 or later need to install the source distribution. Importantly, the source distribution includes the Cython-generated C files, but not the Cython source files themselves. It turns out that users of python 3.9–3.10 are fine (assuming that they've either installedwheel
or have an old version of pip), because the C files included in the source distribution happen to be compatible with these versions of python. Users of python 3.11, however, get the following error:A very similar error is discussed here, and I believe this is exactly what's happening in #24. The reason is that the generated C files depend on a header that was apparently moved in python 3.11. The solution is to regenerate these files. In fact, this is something that
setuptools
would do automatically if the Cython source files were included in the source distribution.If you try to regenerate the C files yourself, there's another very subtle trap that you can fall into. When making the list of extension modules to build, the
setup.py
script checks to see if Cython is installed. If it is, the list comprises all files matching*.pyx
. If not, it comprises all files matching*.c
instead. The problem happens if you try to install the source distribution while Cython is installed, e.g.:In this case, because Cython is installed,
setup.py
will only try to build extension modules for files matching*.pyx
. But because the source distribution doesn't have any*.pyx
files, no extension modules end up getting built. The end result is that the installation seems to work, but none of the extension modules can be imported. I'm pretty sure that this is what's happening in #16. The solution, which a number of people seem to have stumbled onto, is to install from GitHub (pip install git+https://github.com/AMLab-Amsterdam/lie_learn
) instead of PyPI.The last problem is that Cython>=3.0 assumes python3 syntax by default, while the scripts in this repository still use python2 syntax. There's no way to tell Cython which syntax to expect without changing the code itself (see: cython/cython#3930), so unfortunately the only option right now is to install an old version of Cython.
This PR fixes all these problems by modernizing the build system, following the recommendations of the
setuptools
team. Specifically, I added apyproject.toml
file specifying that:Note that the above recommendations aren't opposed to leaving the
*.pyx
files out of the source distribution (i.e. the status quo). Doing so eliminates the possibility that different users might get different behavior due to using different versions of Cython, which is certainly an advantage. But it also requires the maintainers to re-upload the source distribution every time a new version of python is released, which seems too onerous to me.Once the
*.pyx
files are included in the source distribution, there's no reason for the generated C files to be included as well. (They'll always be ignored.) In fact, I don't think there's any reason for them to even be committed to the repository. But in the interest of not changing any more than I had to, I left that change for another time.Thanks for taking the time to read all of this; I know it's a lot!