aleaxit / gmpy

General Multi-Precision arithmetic for Python 2.6+/3+ (GMP, MPIR, MPFR, MPC)
https://gmpy2.readthedocs.io/en/latest/
GNU Lesser General Public License v3.0
506 stars 84 forks source link

Drop gmpy2 directory & move source files to src/gmpy2/ #474

Closed skirpichev closed 3 months ago

skirpichev commented 4 months ago

This uses more modern directory layout (like python-flint does, for example) and avoids extra pure-python code while doing import. The drawback is that you have to use extra --follow arguments for some git commands (e.g. git log) to access early history.

With this patch:

$ time python -c 'from gmpy2 import *'

real    0m0.065s
user    0m0.050s
sys     0m0.015s

On the master:

$ time python -c 'from gmpy2 import *'

real    0m0.120s
user    0m0.095s
sys     0m0.024s

Uses patched delocate from my repo: git+https://github.com/skirpichev/delocate.git@fix-lib-sdir

codecov-commenter commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.52%. Comparing base (224745b) to head (0f499bd).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #474 +/- ## ======================================= Coverage 85.52% 85.52% ======================================= Files 50 50 Lines 11656 11656 Branches 2202 2202 ======================================= Hits 9969 9969 Misses 1687 1687 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

skirpichev commented 4 months ago

@casevh, this is a draft pr, but let me know if you like this idea.

casevh commented 4 months ago

I installed a Linux wheel. It imports and runs just fine. I then looked at the file layout. The shared library is installed in site-packages/, the include files are installed in site-packages/gmpy2/, and the supporting libraries are installed in site-packages.gmpy2.libs.

The following comments are based on memories from a discussion that occurred several years. gmpy and gmpy2 always used to install its shared library in site-packages. When the gmpy2 direcctory was added as part of support for setuptools, the shared library was moved to the gmpy2 directory and loaded via gmpy2/__init__.py. I questioned the rationale. I was left with the impression that using gmpy2/__init__.py was the correct approach and that Python extensions shouldn't just be dumped into site-packages.

I personally liked having gmpy2...so in site-packages. (For many years, the Windows binaries where distributed as a single statically-linked DLL that was placed in site-packages. This predated pip.) But I don't want to rely on an unsupported file location that might depracted in the future.

I do like the faster import time (so +1), but I don't know if it is worth the possible long-term stability risks (-0.5) and the code churn (also -0.5).

I'll continue to think about this...

skirpichev commented 4 months ago

But I don't want to rely on an unsupported file location that might depracted in the future.

I don't sure I realize which scenario do you mean. The module so-file in the site-packages directory is a long-standing default for the setuptools.Extension().

skirpichev commented 4 months ago

This is ready for review.

skirpichev commented 4 months ago

Hmm, it seems delvewheel in this settings puts all libraries to the site-packages/ dir. Will think if it's possible to workaround this.

Meanwhile, mark this as a draft. Please don't merge.

skirpichev commented 3 months ago

Well, the problem is that PE format has nothing like RPATH in ELF... So, options are either

  1. put dll's alongside with the module dll.
  2. use __init__.py file and put here suitable os.add_dll_directory() calls, that will point to the gmpy2.libs dir.

The delvewheel uses both options, respectively, (1) - if extension module is top-level, and (2) - if it has __init__.py file.

I think (1) is fine, if we will also use name-mangling on M$ Windows (now - turned off). Not sure if it's file without name-mangling. Maybe the better alternative is postpone this pr and, instead, package GMP, MPFR and MPC libraries as separate packages like scipy-openblas64?

casevh commented 3 months ago

I propose option (1) but renaming the DLLs to well-defined names. The renaming is done before gmpy2 is compiled.

Look at https://github.com/emilbayes/rename-dll for a utility to do the renaming.

For a naming convention, how about libgmp-10.dll becomes lib_gmpy2v2_2-10.dll and similarly for mpfr and mpc. The implies that the DLLs were distributed as part of gmpy2 version 2.2.x. The name would change if the DLLs change in a backwards incompatible way. And that should trigger a new version of gmpy2.

We won't conflict with standard names. And C extensions are able to use the renamed DLLs.

On Fri, Mar 29, 2024 at 6:57 AM Sergey B Kirpichev @.***> wrote:

Well, the problem is that PE format has nothing like RPATH in ELF... So, options are either

  1. put dll's alongside with the module dll.
  2. use init.py file and put here suitable os.add_dll_directory() calls, that will point to the gmpy2.libs dir.

The delvewheel uses both options, respectively, (1) - if extension module is top-level, and (2) - if it has init.py file.

I think (1) is fine, if we will also use name-mangling on M$ Windows (now

  • turned off). Not sure if it's file without name-mangling. Maybe the better alternative is postpone this pr and, instead, package GMP, MPFR and MPC libraries as separate packages like scipy-openblas64?

— Reply to this email directly, view it on GitHub https://github.com/aleaxit/gmpy/pull/474#issuecomment-2027284096, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMR235AJ6DPLVES3FDMYK3Y2VXNRAVCNFSM6AAAAABFEL2QICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXGI4DIMBZGY . You are receiving this because you were mentioned.Message ID: @.***>

skirpichev commented 3 months ago

For a naming convention, how about libgmp-10.dll becomes lib_gmpy2v2_2-10.dll and similarly for mpfr and mpc.

I guess you meant something like "libgmp_gmpy2v2_2-10.dll". Your variant doesn't include library name.

rename-dll for a utility to do the renaming.

Do you suggest to restrict this workaround only for M$ Windows? I would prefer a common setup for all platforms. That's why I'm thinking about separate packages for libraries. Probably, this should be discussed in #352: now we have binary wheels for all platforms, what are your plans for the referenced issue?

casevh commented 3 months ago

I guess you meant something like "libgmp_gmpy2v2_2-10.dll". Your variant doesn't include library name.

Correct.

Do you suggest to restrict this workaround only for M$ Windows? I would prefer a common setup for all platforms. That's why I'm thinking about separate packages for libraries. Probably, this should be discussed in #352: now we have binary wheels for all platforms, what are your plans for the referenced issue?

It looks like flint has updated their build system to meson. https://github.com/flintlib/python-flint/issues/52

I would like to focus on resolving the challenges with creating extensions that want to use gmpy2's C-API. I think it is fine to require installing a binary wheel of gmpy2 to get the GMP, MPFR, and MPC libraries. The fundamental issues are (1) predictable names and (2) finding the required files.

For Windows, I slightly prefer to place the supporting files in the same directoy as the gmpy2 pyd file. That layout has been used in the past. This does require importing gmpy2 before importing the C extension but I don't think that is unreasonable.

casevh commented 3 months ago

There are many unresolved questions in this PR. Would you be okay if we release gmpy2 2.2.0 quickly to address support for Python 3.12 and 3.13 and delay this PR until gmpy2 2.3 (hopefully released when Python 3.14 is released)?

If you agree, I'll bump the version to b1 and release it ASAP for testing. Then release rc1 shortly after 3.13.0b1 is released.

skirpichev commented 3 months ago

@casevh, sure! It's a draft pr, it shouldn't block anything.

But maybe you would like also to do a minor release for 2.1.x with support for CPython 3.12/13?

casevh commented 3 months ago

But maybe you would like also to do a minor release for 2.1.x with support for CPython 3.12/13? @skirpichev A source-only release for Linux distributions would be easy. I need to see if I can replicate the 2.1.x process for compiling the binary wheels for Windows.

Maybe just release 2.1.6 as source-only and release 2.2.0 as rc1. The "pip install gmpy2" should grab the rc1 version and a standard 2.1.6 version is available for Linux distributions.

I'll probably increment the version to b1 this evening. Are there any last minute changes you want to make?

skirpichev commented 3 months ago

Maybe just release 2.1.6 as source-only

Does make sense for me.

Are there any last minute changes you want to make?

No:) I would like to solve #467, but it seems that new C API will not land in the CPython 3.13.