cypari2 icon indicating copy to clipboard operation
cypari2 copied to clipboard

merge cypari and cypari2

Open videlec opened this issue 7 years ago • 40 comments

The original cypari written by N. Dunfield and M. Culler will be merged with this project to ease maintenance. Though we are facing several incompatibilities

1 - Windows

cypari can be installed on Windows (however not directly from source, they provide wheels on PyPI). This is at the cost of a fork of cysignals and many special casing.

  • [x] cysignals is not working on windows. See issue 63.
  • [ ] issues with long (cypari defines a new pari_ulongword type)

Note that these issues refer to plain Windows. Under cygwin, there are no such issues.

2 - Static vs dynamic linking / source vs distribution

cypari currently ships gmp, pari and (a fork of) cysignals. cypari2 contains only sources for a Python module. The original cypari has the advantage of being installable on many platforms using wheels (among other it solves the problem of having cysignals built after libpari).

We propose in the merged version to have three possible ways of installation

  1. pip install cypari: installs a wheel with static linking and embedded cysignals (i.e. what is currently available with the cypari wheels)

  2. pip install --no-binary cypari: installs from an sdist with dynamic linking from gmp/pari on the system and dependency from cysignals (i.e. the cypari2 version)

  3. (optional) pip install --no-binary --install-option=static-link cypari: installs from an sdist but also builds libpari.a and libgmp.a and statically links against them (intermediate solution)

4 - Cython macro

Cython macros (either defined using DEF or via compile_time_env option of cythonize) modifies the C code generated by Cython from the pyx files. Having many of them imposes to provide a different versions of the generated C files for each possible values of the constants (in order to allow to build from source without Cython).

The Python2/Python3 compatibility can be achieved without any of them (as done in cypari2, see #13). However it seems unavoidable for the Windows port.

videlec avatar Jul 20 '17 09:07 videlec

Building gmp and pari would better not be part of the setup.py script.

In my view, this is mostly an problem of user interface. It's trivial to support different kinds of builds. But the hard question is: how does the interface work? What should happen when somebody does pip install cypari? When installing from source using setup.py, it would be possible to require an argument like setup.py --build-pari or whatever. But with wheels, I don't think this is possible.

jdemeyer avatar Jul 20 '17 10:07 jdemeyer

Building gmp and pari would better not be part of the setup.py script.

In my view, this is mostly an problem of user interface. It's trivial to support different kinds of builds. But the hard question is: how does the interface work? What should happen when somebody does pip install cypari? When installing from source using setup.py, it would be possible to require an argument like setup.py --build-pari or whatever. But with wheels, I don't think this is possible.

This is why I proposed in the exchange of e-mails to have two packages cypari-mini (only source?) and cypari-full (source and wheels). I don't think there is a need for more options. Don't you like this way?

videlec avatar Jul 20 '17 10:07 videlec

@videlec With regards to cypari-mini versus cypari-full is the idea that they would both install a package named cypari but they would be separate packages on PyPI? Or would they install differently named packages? Either way, we probably need to think about what should happen when pip tries to install another Python package that depends on cypari-*.

NathanDunfield avatar Jul 20 '17 16:07 NathanDunfield

With regards to cypari-mini versus cypari-full is the idea that they would both install a package named cypari but they would be separate packages on PyPI? Or would they install differently named packages? Either way, we probably need to think about what should happen when pip tries to install another Python package that depends on cypari-*.

@NathanDunfield my idea is not completely clear yet :-) I definitely want both versions to install a cypari package. The best I think will be to have cypari-full as a "virtual package" that do the install gmp, pari, cysignals, cypari-mini in that order. If this is the way to go, we would perhaps better use the name cypari for cypari-mini because it would match the Python module names. However, this change would break backward compatibility of pip install cypari.

Concerning dependencies on cypari, is it possible to have fine dependencies in pip? I imagine something like: "if cypari-mini is there then fine, otherwise install cypari-full".

videlec avatar Jul 20 '17 16:07 videlec

@videlec @NathanDunfield

A simple implementation of this idea might be to just have a setup.py option which specifies whether to use system libraries or to build the libraries from scratch. The default would be to use system libraries. That way: pip install --no-binary cypari would be the equivalent of installing cypari-mini (provided the build succeeds). On the other hand pip install cypari would download the wheel, which would necessarily include its own copies of the libraries, so that would be the equivalent of installing cypari-full. To build the wheels one would supply the option to setup.py which indicates to build the libraries. Someone who wanted to build their own standalone module could just clone the repo.

culler avatar Jul 20 '17 18:07 culler

@videlec wrote:

Concerning dependencies on cypari, is it possible to have fine dependencies in pip? I imagine something like: "if cypari-mini is there then fine, otherwise install cypari-full".

You can achieve this sort of effect by putting some logic in the dependent package's setup.py script and modifying the list of packages passed to setup(install_requires=...). We do this with SnapPy, e.g. not including cypari as a requirement when installing into Sage. But so far I haven't seen any built in support for this in pip, e.g. requirements of the form foo>2.0|bar>1.5 which is satisfied if either is present and otherwise it tries to install the first one.

@culler wrote:

On the other hand pip install cypari would download the wheel, which would necessarily include its own copies of the libraries, so that would be the equivalent of installing cypari-full.

One possible issue: support for Linux wheels in pip is pretty new, and so on some distros pip install cypari would actually try to build the source tarball (unless one also posts eggs).

NathanDunfield avatar Jul 20 '17 19:07 NathanDunfield

@videlec @culler Another approach would be to have three packages: cypari, cypari-full and cypari-mini. The first contains only a few lines of code that try to import cypari-mini and cypari-full in turn; when it succeeds it sets cypari.pari to cypari_found.pari etc and otherwise raises an ImportError. If you do pip install cypari, then we make this succeed only if one of cypari-full and cypari-mini is present, otherwise it throws polite error message indicating that the user should first install their choice of cypari-full and cypari-mini. We could put in certain shortcuts, e.g. on Windows pip install cypari would automatically require cypari-full. Possibly the same thing should be true on macOS for a Frameworks build of Python (such as the ones downloaded from Python.org) as opposed the "plain unix style" Pythons that come with SageMath and (I think) brew.

For example, one concrete possibility would be to have the setup.py script of cypari do the following:

  1. Try to import cypari-mini and cypari-full. If one is available and has the same version number as cypari then everything is good and it just installs its own few lines of code.

  2. If no suitable version of cypari-* is available and the OS is either Windows or macOS with a Frameworks build of Python, then add cypari-full to install_requirements and pip will handle the rest.

  3. On Linux or with a "plain unix" macOS python, then it could do some tests to see if there appears to be suitable versions of gmp and pari available. If so, add cypari-mini (which forces cysignals) to the install_requirements and otherwise add cypari-full.

NathanDunfield avatar Jul 20 '17 19:07 NathanDunfield

@NathanDunfield

Another approach would be to have three packages: cypari, cypari-full and cypari-mini. [ ... snip ...]

With your proposal, if I want the minimal version should I do

$ pip install cypari-mini
$ pip install cypari
  1. On Linux or with a "plain unix" macOS python, then it could do some tests to see if there appears to be suitable versions of gmp and pari available. If so, add cypari-mini (which forces cysignals) to the install_requirements and otherwise add cypari-full.

It is hard to provide reliable tests to see whether one can build cypari-mini. Namely, the user might not have gcc. I would be more convinced by a solution that by default always install cypari-full unless explicitely specified (as Marc proposed with --no-binary).

videlec avatar Jul 21 '17 10:07 videlec

@videlec wrote:

With your proposal, if I want the minimal version should I do

$ pip install cypari-mini
$ pip install cypari

Yes, that's right.

It is hard to provide reliable tests to see whether one can build cypari-mini. Namely, the user might not have gcc. I would be more convinced by a solution that by default always install cypari-full unless explicitly specified (as Marc proposed with --no-binary).

I agree that figuring out whether cypari-mini can be installed is tricky and should be avoided if possible. I would be happy with a solution that installs cypari-full unless explicitly specified (via --no-binary or some other flag), but wasn't sure if that would be acceptable to you guys.

NathanDunfield avatar Jul 21 '17 14:07 NathanDunfield

Also, I was thinking that perhaps we should use slightly more descriptive names, perhaps cypari-selfcontained and cypari-system-pari or cypari-static and cypari-libpari.

NathanDunfield avatar Jul 21 '17 14:07 NathanDunfield

Then, we might want to stick to a unique cypari package when in source mode (i.e. --no-binary pip option) will use whatever pari is available on the system (that is the current cypari2 version) and for which all the wheels are self-contained (ie the current cypari wheels)? When used in source mode it is assumed that libpari is available and that can be tested for providing nice error message.

I am not sure there is a need for intermediate installations. This is the job of distributions.

I see one backward compatibility with this proposal for people who used to install the original cypari in source mode.

videlec avatar Jul 21 '17 15:07 videlec

So pip has an --install-option flag which can be used to pass options down to setup.py. It seems like this would allow us to cover the backward compatibility case you mention via something like:

pip install --no-binary --install-option=build-own-pari  cypari

Putting this together, a concrete plan would be:

  1. Have a single cypari package on PyPI with the the usual 16 or so cypari-full binary wheels and a source tarball containing the Cythonized C source files but not the gmp and pari sources.

  2. Have the source tarball default to cypari-mini unless a suitable flag is set, in which case it fetches the gmp and pari sources and builds its own copy of pari from scratch.

What do people think of such a plan? It seems good to me.

NathanDunfield avatar Jul 21 '17 15:07 NathanDunfield

I see one backward compatibility with this proposal for people who used to install the original cypari in source mode.

I think Nathan, Matthias and I will be able to handle it. :^) Oops, I forgot Bill Allombert! He will be OK too.

culler avatar Jul 21 '17 15:07 culler

That sounds great to me!

  • Marc

On Fri, Jul 21, 2017 at 10:46 AM, NathanDunfield [email protected] wrote:

So pip has an --install-option flag which can be used to pass options down to setup.py. It seems like this would allow us to cover the backward compatibility case you mention via something like:

pip install --no-binary --install-option=build-own-pari cypari

Putting this together, a concrete plan would be:

Have a single cypari package on PyPI with the the usual 16 or so cypari-full binary wheels and a source tarball containing the Cythonized C source files but not the gmp and pari sources. 2.

Have the source tarball default to cypari-mini unless a suitable flag is set, in which case it fetches the gmp and pari sources and builds its own copy of pari from scratch.

What do people think of such a plan? It seems good to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defeo/cypari2/issues/19#issuecomment-317036773, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPhP0YkawD8XN10vYkqjgQxCj61gZDAks5sQMfkgaJpZM4Od2xy .

culler avatar Jul 21 '17 15:07 culler

What do people think of such a plan? It seems good to me.

I thought a bit more this. One important question is: how to link against libpari? Are we doing static linking (like the current cypari) or dynamic linking?

The problem is that both have significant disadvantages:

  • a static library cannot be shared across modules. So, if there are other packages which link against PARI, they will use a different independent copy of PARI. In particular, cysignals should not be built with PARI support in this case (I am assuming that cysignals will still be a separate package, contrary to what cypari does)
  • a dynamic library seems fundamentally incompatible with wheel. You could ship the dynamic library (the .so file) inside the wheel, but the directory where the library gets installed might not be in the library search path. For system installs, this might be OK but for a --user install, this will almost certainly break.

jdemeyer avatar Jul 28 '17 14:07 jdemeyer

I guess what I would prefer is to give the user many options, but make nothing the default. In other words, allow pip install cypari-static-libpari but let pip install cypari fail with an error message explaining the various options.

jdemeyer avatar Jul 28 '17 14:07 jdemeyer

You can have dynamic libraries inside a wheel. That's what numpy, scipy etc does with openblas.

isuruf avatar Jul 28 '17 14:07 isuruf

I thought we were proposing three options, with linking as follows:

pip install cypari (installs a wheel with static linking)

pip install --no-binary cypari (installs from an sdist with dynamic linking)

pip install --no-binary --install-option=static-link cypari (installs from an sdist but also builds libpari.a and libgmp.a and statically links against them)

  • Marc

On Fri, Jul 28, 2017 at 9:29 AM, jdemeyer [email protected] wrote:

What do people think of such a plan? It seems good to me.

I thought a bit more this. One important question is: how to link against libpari? Are we doing static linking (like the current cypari) or dynamic linking?

The problem is that both have significant disadvantages:

  • a static library cannot be shared across modules. So, if there are other packages which link against PARI, they will use a different independent copy of PARI. In particular, cysignals should not be built with PARI support in this case (I am assuming that cysignals will still be a separate package, contrary to what cypari does)
  • a dynamic library seems fundamentally incompatible with wheel. You could ship the dynamic library (the .so file) inside the wheel, but the directory where the library gets installed might not be in the library search path. For system installs, this might be OK but for a --user install, this will almost certainly break.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defeo/cypari2/issues/19#issuecomment-318667045, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPhP8_zlpd3PrxHYwg8-5oqbUXnIjFJks5sSfBKgaJpZM4Od2xy .

culler avatar Jul 28 '17 14:07 culler

Fernando,

Does that work on Windows as well?

  • Marc

On Fri, Jul 28, 2017 at 9:39 AM, Isuru Fernando [email protected] wrote:

You can have dynamic libraries inside a wheel. That's what numpy, scipy etc does with openblas.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defeo/cypari2/issues/19#issuecomment-318669632, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPhP9kkPPaFWCnczNMUDAt__9vckPInks5sSfKagaJpZM4Od2xy .

culler avatar Jul 28 '17 14:07 culler

Does that work on Windows as well?

Yes.

isuruf avatar Jul 28 '17 14:07 isuruf

You can have dynamic libraries inside a wheel. That's what numpy, scipy etc does with openblas.

I just checked what numpy does. It has an openblas library hidden inside site-packages/numpy/.libs. So it's really meant to be used only by numpy. As far as external packages are concerned, it still has the disadvantages of a static library.

jdemeyer avatar Jul 28 '17 15:07 jdemeyer

@isuruf: Do other python packages use the version of the openblas library that ships with numpy, or does only the numpy module itself use it? If just the latter, then effectively this is no different than an entirely static wheel.

Just to clarify, the second option @culler refers to means:

pip install --no-binary cypari (installs from an sdist with dynamic linking against a previous installed system libpari)

NathanDunfield avatar Jul 28 '17 15:07 NathanDunfield

Embedding shared libraries in the wheel would have several advantages. The most important one is that it allows more flexibility with the configuration of modules. Currently we are forced to have only one extension module which is statically linked against libpari because of pari's global variables (especially the stack). Separate modules which are both statically linked cannot share Pari objects since they have different stacks. Dynamic linking solves this problem.

  • Marc

On Fri, Jul 28, 2017 at 9:54 AM, Isuru Fernando [email protected] wrote:

Does that work on Windows as well?

Yes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defeo/cypari2/issues/19#issuecomment-318673828, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPhP849Gp_ZHvawzTdGlQFJh6dB1cuUks5sSfYYgaJpZM4Od2xy .

culler avatar Jul 28 '17 15:07 culler

The most important one is that it allows more flexibility with the configuration of modules.

You are right that it fixes this problem, but that's also the only problem that the numpy approach fixes.

jdemeyer avatar Jul 28 '17 15:07 jdemeyer

That is possible, but I am not sure. I don't know what mechanism numpy is using, but I am guessing that they somehow specify a path within their package to dlload. If one module can do that, then shouldn't other modules be able to extract the path from the module which contains the library, and load it in the same way?

On Fri, Jul 28, 2017 at 10:05 AM, jdemeyer [email protected] wrote:

The most important one is that it allows more flexibility with the configuration of modules.

You are right that it fixes this problem, but that's also the only problem that the numpy approach fixes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defeo/cypari2/issues/19#issuecomment-318676944, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPhP9IMH25ipQ6crb9MlEh8MV_Dhmv7ks5sSfiigaJpZM4Od2xy .

culler avatar Jul 28 '17 15:07 culler

@isuruf: Do other python packages use the version of the openblas library that ships with numpy, or does only the numpy module itself use it? If just the latter, then effectively this is no different than an entirely static wheel.

Both scipy and numpy have openblas dependency. And both ship the exact same version of openblas shared library. I'm guessing that whichever package is imported first in python brings in openblas and only that copy is loaded.

isuruf avatar Jul 28 '17 15:07 isuruf

And both ship the exact same version of openblas shared library. I'm guessing that whichever package is imported first in python brings in openblas and only that copy is loaded.

That seems to be the case on Linux at least. I have no idea what happens on other systems (given that dynamic linking is very system-dependent, you cannot really extrapolate from Linux).

Suppose that this really works on all systems where we want to build wheels for. Then we could make a Python package libpari which just contains the PARI dynamic library and then use it from cypari and cysignals using the same approach as numpy and scipy.

jdemeyer avatar Jul 28 '17 15:07 jdemeyer

One way to check: install numpy and scipy from the binary wheel and then delete the directory .../site-packages/scipy/.libs/ If you can still import scipy.stats.statlib, then the trick works.

jdemeyer avatar Jul 28 '17 15:07 jdemeyer

One way to check: install numpy and scipy from the binary wheel and then delete the directory .../site-packages/scipy/.libs/ If you can still import scipy.stats.statlib, then the trick work

Yes, it works. Removing numpy/.libs and keeping scipy/.libs doesn't work because scipy imports numpy first.

isuruf avatar Jul 28 '17 15:07 isuruf

One way to check: install numpy and scipy from the binary wheel and then delete the directory .../site-packages/scipy/.libs/ If you can still import scipy.stats.statlib, then the trick works.

I took a stab at this. On macOS, the analogous directory appears to be .../site-packages/scipy/.dylibs/ which contains:

libgcc_s.1.dylib     libgfortran.3.dylib* libquadmath.0.dylib* 

However, numpy has no .dylibs directory, and numpy/core/multiarray.cpython-36m-darwin.so is only linked against:

	/System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate (compatibility version 1.0.0, current version 4.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1213.0.0)

My guess is the former includes Apple's version of blas. For funsies, I did try deleting scipy/.dylibs/ but import scipy.stats.statlib failed as you might expect.

On Windows, there is no wheel for scipy at all, so I wasn't able to get data there at all.

NathanDunfield avatar Jul 28 '17 15:07 NathanDunfield