search

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

Build and test windows wheels #469

Closed skirpichev closed 4 months ago

skirpichev commented 4 months ago

Closes #468

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 (38bf13e) to head (bbc5586).

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #469 +/- ## ======================================= 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.

casevh commented 4 months ago

Why aren't the patches related to mp_bitcnt_t (sp?) carried over to the Windows compilation of GMP?

Question: Assuming we use delve-wheel, what GMP/MPFR/MPC libraries will a cython/C-API extension use when gmpy2 v2.2.0 is upgraded to v2.2.1? (The problem is (possible) shared internal state in the GMP library that is inherited by MPFR and MPC.)

skirpichev commented 4 months ago

Why aren't the patches related to mp_bitcnt_t (sp?) carried over to the Windows compilation of GMP?

That does make sense for me, but shouldn't you submit these patches to upstream? BTW, was this applied to previous stable release?

Assuming we use delve-wheel, what GMP/MPFR/MPC libraries will a cython/C-API extension use when gmpy2 v2.2.0 is upgraded to v2.2.1?

If we will bundle same versions of libraries across minor releases gmpy2 - I doubt there will be problems.

I think we could adopt scipy-like approach (for openblas), i.e. with separate wheels for libraries. Probably, python-flint could reuse these libraries. But this issue might be more complex to address just in the next release. So, in a meantime I'll adopt same approach as for previous gmpy2 releases: no mangling (unless it can't be disabled as with auditwheel), libraries are in the gmpy2.libs directory.

skirpichev commented 4 months ago

@casevh, I think it's ready for review. But I would appreciate if you take look on #470 first.

open issues:

PS: As I have not access to M$ Windows - I would appreciate if someone else could test generated wheels.

casevh commented 4 months ago

I have done a quick test of the Python 3.11 Windows wheel and it worked just fine.

I don't think the version of Windows will matter but I would probably move to 2022 just to avoid having to move at some time in the future.

skirpichev commented 4 months ago

I would probably move to 2022 just to avoid having to move at some time in the future.

Done.

eendebakpt commented 4 months ago

I have done a quick test of the Python 3.11 Windows wheel and it worked just fine.

I don't think the version of Windows will matter but I would probably move to 2022 just to avoid having to move at some time in the future.

@casevh Where I can I find the windows wheels from this PR?

casevh commented 4 months ago

Most recents wheels should be at https://github.com/aleaxit/gmpy/actions/runs/8229996172/artifacts/1314269722

This is a later build than I've tested so let us know if it doesn't work for you.

eendebakpt commented 4 months ago

Most recents wheels should be at https://github.com/aleaxit/gmpy/actions/runs/8229996172/artifacts/1314269722

This is a later build than I've tested so let us know if it doesn't work for you.

Thanks! Installation went fine (python 3.12) and my code runs fine so far as I have tested.

skirpichev commented 4 months ago

Build wheels should be available with every commit, that has green CI check. Go to any "Build wheels" job and then to the "Summary" page.

Edit: last commit add documentation for building release wheels on M$. Let me know if this complicates code review: we can factor out this change for later prs.

casevh commented 4 months ago

Is this PR ready to merge?

Has the mpbitcnt_t patch been included?

Are there any ordering dependancies between the three PRs?

skirpichev commented 4 months ago

Is this PR ready to merge?

It's ready for review:)

Has the mpbitcnt_t patch been included?

No, for reasons I mentioned above.

Now I did this (last commit). This seems to be a bad idea, however.

Are there any ordering dependancies between the three PRs?

There is only one other ready for review pr: #471. I think it shouldn't conflict with the current one.

casevh commented 4 months ago

That does make sense for me, but shouldn't you submit these patches to upstream? BTW, was this applied to previous stable release?

The initial motivation to patch mp_bitcnt_t was based on this comment in the GMP manual:

"Counts of bits of a multi-precision number are represented in the C type mp_bitcnt_t. Currently this is always an unsigned long, but on some systems it will be an unsigned long long in the future."

Patched versions of GMP 6.2 were used for gmpy2 2.1.x.

The second patch is coding error in a function introduced in GMP 6.3. It has been reported.

The patch fixes the following problem. mpz_sizeinbase(mpz, 2) returns size_t which is 64 bits on 64-bit versions of Windows. But all the bit manipulation functions use mp_bitnct_t. Without the patch, mp_bitcnt_t is only 32 bits. Functions such as mpz_popcount(mpz) will fail for large nunbers with many bits set.

Assuming we use delve-wheel, what GMP/MPFR/MPC libraries will a cython/C-API extension use when gmpy2 v2.2.0 is upgraded to v2.2.1?

If we will bundle same versions of libraries across minor releases gmpy2 - I doubt there will be problems.

I think we could adopt scipy-like approach (for openblas), i.e. with separate wheels for libraries. Probably, python-flint could reuse these libraries. But this issue might be more complex to address just in the next release. So, in a meantime I'll adopt same approach as for previous gmpy2 releases: no mangling (unless it can't be disabled as with auditwheel), libraries are in the gmpy2.libs directory.

skirpichev commented 4 months ago

The initial motivation to patch mp_bitcnt_t was based on this comment in the GMP manual:

Perhaps, we should prepare a real patch and convince gmp people to accept it.

Patched versions of GMP 6.2 were used for gmpy2 2.1.x.

Ok, I see.

Should I adapt instructions in https://gmpy2.readthedocs.io/en/latest/install.html#installation to mention that patch? Sadly, we can't just instruct people to use gmp package from the msys2.

The second patch is coding error in a function introduced in GMP 6.3.

Sorry, I miss that. Is it was committed before? mingw64/msys2_64_shared.sh includes only the mp_bitcnt_t patch.

skirpichev commented 4 months ago

Last commit include the mp_bitcnt_t patch as a plain diff.

casevh commented 4 months ago

I tested a couple of the wheels and they look fine. I will merge this PR and will merge #474 later. Then I'll do some extensive testing on Windows.

skirpichev commented 4 months ago

Then I'll do some extensive testing on Windows.

Let me know if you find any problems.

I would appreciate, if we could make new alpha release, with support for the CPython 3.13.

BTW, as now we built wheels for all supported platforms in CI - you may consider using https://github.com/pypa/gh-action-pypi-publish to automate PyPI uploads. See mpmath workflow: https://github.com/mpmath/mpmath/blob/3512b74baa53122269a7e47a35f61e9d96b50725/.github/workflows/test.yml#L83-L88

casevh commented 4 months ago

I agree on a new alpha release with support for Python 3.13. I'd like to target a beta release shortly after Python 3.13.0b1 and a final release to coincide with the official release of 3.13.0.

casevh commented 4 months ago

FYI. Error in the building of the Windows wheels. Here is the error message: Repairing wheel...

skirpichev commented 4 months ago

This is from https://github.com/aleaxit/gmpy/pull/474? I'm working on this. It seems, the delvewheel, like the delocate-wheel, uses special directory in this case.

casevh commented 4 months ago

Just let me know if I need to push #474.

Case

On Sun, Mar 24, 2024 at 10:06 PM Sergey B Kirpichev < @.***> wrote:

This is from #474 https://github.com/aleaxit/gmpy/pull/474? I'm working on this. It seems, the delvewheel https://github.com/adang1345/delvewheel/tree/bc55faf1ba382b7906b127ec9df15e35c44a941d/delvewheel, like the delocate-wheel, uses special directory in this case.

— Reply to this email directly, view it on GitHub https://github.com/aleaxit/gmpy/pull/469#issuecomment-2017221145, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMR23YISLDW4L2KTBSUPDDYZ6WDZAVCNFSM6AAAAABEKNSXRCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJXGIZDCMJUGU . You are receiving this because you modified the open/close state.Message ID: @.***>