zadorlab / sella

A Python software package for saddle point optimization and minimization of atomic systems.
https://www.ecc-project.org/
Other
72 stars 20 forks source link

Tests require compiler due pyximport usage #13

Closed awvwgk closed 2 years ago

awvwgk commented 2 years ago

I'm trying to package Sella for conda-forge (see https://github.com/conda-forge/staged-recipes/pull/18786), however I'm encountering an issue with the Cython compilation when running the tests. Several errors are reported because of failed compilation:

Error compiling Cython file:
------------------------------------------------------------
...

def wrap_skew(x, Y, scale):
    return skew(x, Y, scale)

def wrap_mgs(X, Y, eps1=1e-15, eps2=1e-6, maxiter=1000):
    return mgs(X, Y=Y, eps1=eps1, eps2=eps2, maxiter=maxiter)
          ^
------------------------------------------------------------

tests/utilities/math_wrappers.pyx:19:11: 'mgs' is not a constant, variable or function identifier
ehermes commented 2 years ago

I don't really know anything about conda-forge, and I have a pretty negative opinion of Anaconda in general, so I'm not sure how much I can help with this.

The pyx file that it's attempting to compile is not meant to be compiled at build-time, it's meant to be compiled post-install when tests are run. If you look at tests/utilities/test_math.py, you see it compiles the math_wrappers.pyx file using the pyximport package. This requires that the sella/utilities/math.pyx already be compiled and installed in the appropriate directory for Python to be able to find it.

I will say that users ought to be able to install Sella even within Anacaonda environments by using pip.

awvwgk commented 2 years ago

conda-forge is not associated with Anaconda and an independent project lead by volunteers. I'm sorry for your negative experience with Anaconda, but please don't let it out onto the conda-forge community.

I never packaged a Cython project before, seems like I need to require a compiler in the run environment unless I am able to precompile all the pyx in the package phase. In this case the user should be able to install Sella and use it without a compiler, shouldn't they?

ehermes commented 2 years ago

The way that it works on PyPI is the .pyx source files are first "Cythonized" into .c source files. If you publish with python setup.py sdist, then those .c files are packaged with the .py files into a tarball and uploaded to PyPI servers. It is also possible to build wheels with the .c files precompiled into .so files, but that needs to be done for every platform (every possible combination of supported operating system, CPU architecture, and Python version). My understanding is that this can be accomplished somewhat easily with build bots, but I have not had the opportunity to set that up. If there is a desire for prebuilt versions of Sella, I would much rather go this route and enable support for all Python distributions than lock it down to only (ana)conda distributions of Python.

Edit: And anyway, running the tests will always require a compiler -- at least, testing the Cython code will require a compiler. Perhaps there's some way to disable those tests if the user doesn't have a compiler, and I'd be willing to look into that if there is demand.

awvwgk commented 2 years ago

I thought I had compiled the Cython code, but one of the tests seems to still require a compiler, maybe this is due to using pyximport here?

https://github.com/zadorlab/sella/blob/215e013c7fd091855a77c999444c6dbeb4a91e0b/tests/utilities/test_math.py#L8-L10

ehermes commented 2 years ago

Yes, all of the tests for the Cython parts of the codebase will require a compiler.

awvwgk commented 2 years ago

I'm actually creating binary distributions and compiling the Cython code. However, I was surprised to see that a compiler is required for the tests, but I found that the usage of pyximport ignores the already compiled Cython code and instead tries to compile it again.

My solution for now is to ignore the one path with --ignore=tests/utilities/test_math.py when validating the binary package.

ehermes commented 2 years ago

I've configured Github CI to automatically publish binary wheels for Linux (Python 3.6 through Python 3.10) and MacOS (Python 3.7 through Python 3.10) when I make a new release. Binaries for Sella 2.1.0 are already available. You should be able to install them with pip.

These are the only platforms for which jaxlib binaries are available on PyPI. On other platforms, you'll need to build jaxlib anyway, so it doesn't make sense to publish Sella wheels for those platforms.

Regarding the tests requiring a compiler (as this issue is now titled), this is not avoidable. You can skip the relevant tests if you so choose, but in that case I suggest you just use the binaries published on PyPI, rather than relying on conda-forge.

awvwgk commented 2 years ago

Thanks for the advice and sorry for having bothered you with this issues.

ehermes commented 2 years ago

No worries! Thank you for taking an interest in Sella. You prompted me to finally set up CI, which I should have done ages ago.