Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Mar 23, 2024

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
Copy link

codecov-commenter commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skirpichev
Copy link
Contributor Author

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

@casevh
Copy link
Collaborator

casevh commented Mar 24, 2024

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 skirpichev force-pushed the no-gmpy2-dir branch 2 times, most recently from 2d67094 to 0322c26 Compare March 24, 2024 08:32
@skirpichev
Copy link
Contributor Author

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 skirpichev marked this pull request as ready for review March 24, 2024 09:20
@skirpichev
Copy link
Contributor Author

This is ready for review.

This uses more modern directory layout (like the ``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
@skirpichev
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

casevh commented Mar 31, 2024 via email

@skirpichev
Copy link
Contributor Author

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
Copy link
Collaborator

casevh commented Apr 7, 2024

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. flintlib/python-flint#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
Copy link
Collaborator

casevh commented Apr 21, 2024

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
Copy link
Contributor Author

@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
Copy link
Collaborator

casevh commented Apr 21, 2024

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
Copy link
Contributor Author

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.

@skirpichev skirpichev closed this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants