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

Bump flint to version 3 #43

Closed
wants to merge 13 commits into from

Conversation

oscarbenjamin
Copy link
Collaborator

Following on from #35 (comment)

Bumps the flint version to 3.0.0-alpha1. Mostly all that is needed is removing -larb from setup.py and renaming the Arb headers. Also the build scripts need to not build Arb separately any more.

We can use this to see in CI that python_flint is ready for flint 3 but it should only be merged after flint 3 is released.

At first CI will fail because of Cython 3 (gh-35 needs to be merged first or the Cython version should be pinned again).

@oscarbenjamin
Copy link
Collaborator Author

The extension module fails to compile on OSX:

     INFO: gcc: src/flint/pyflint.c
      src/flint/pyflint.c:97845:3: error: implicit declaration of function 'nmod_poly_factor_init' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        nmod_poly_factor_init(__pyx_v_fac);
        ^
      src/flint/pyflint.c:97845:3: note: did you mean 'fmpz_poly_factor_init'?
      /private/var/folders/76/zy5ktkns50v6gt5g8r0sf6sc0000gn/T/cirrus-ci-build/.local/include/flint/fmpz_poly_factor.h:30:6: note: 'fmpz_poly_factor_init' declared here
      void fmpz_poly_factor_init(fmpz_poly_factor_t fac);
           ^
      src/flint/pyflint.c:97864:20: error: implicit declaration of function 'nmod_poly_factor_with_berlekamp' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          __pyx_v_lead = nmod_poly_factor_with_berlekamp(__pyx_v_fac, __pyx_v_self->val);
                         ^
      src/flint/pyflint.c:97893:20: error: implicit declaration of function 'nmod_poly_factor_with_cantor_zassenhaus' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          __pyx_v_lead = nmod_poly_factor_with_cantor_zassenhaus(__pyx_v_fac, __pyx_v_self->val);
                         ^
      src/flint/pyflint.c:97913:20: error: implicit declaration of function 'nmod_poly_factor' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          __pyx_v_lead = nmod_poly_factor(__pyx_v_fac, __pyx_v_self->val);
                         ^
      src/flint/pyflint.c:97913:20: note: did you mean 'fmpz_poly_factor'?
      /private/var/folders/76/zy5ktkns50v6gt5g8r0sf6sc0000gn/T/cirrus-ci-build/.local/include/flint/fmpz_poly_factor.h:82:6: note: 'fmpz_poly_factor' declared here
      void fmpz_poly_factor(fmpz_poly_factor_t fac, const fmpz_poly_t G);
           ^
      src/flint/pyflint.c:98044:3: error: implicit declaration of function 'nmod_poly_factor_clear' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        nmod_poly_factor_clear(__pyx_v_fac);
        ^
      src/flint/pyflint.c:98044:3: note: did you mean 'fmpz_poly_factor_clear'?
      /private/var/folders/76/zy5ktkns50v6gt5g8r0sf6sc0000gn/T/cirrus-ci-build/.local/include/flint/fmpz_poly_factor.h:38:6: note: 'fmpz_poly_factor_clear' declared here
      void fmpz_poly_factor_clear(fmpz_poly_factor_t fac);
           ^
      src/flint/pyflint.c:230687:22: warning: comparison of integers of different signs: 'long' and 'mp_limb_t' (aka 'unsigned long') [-Wsign-compare]
            if ((__pyx_t_5 > __pyx_t_6)) {
                 ~~~~~~~~~ ^ ~~~~~~~~~
      src/flint/pyflint.c:259851:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:259862:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:260148:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:260159:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:262322:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:262333:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:262452:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:262463:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:264143:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      src/flint/pyflint.c:264149:21: warning: fallthrough annotation in unreachable code [-Wunreachable-code-fallthrough]
                          CYTHON_FALLTHROUGH;
                          ^
      src/flint/pyflint.c:515:34: note: expanded from macro 'CYTHON_FALLTHROUGH'
            #define CYTHON_FALLTHROUGH __attribute__((fallthrough))
                                       ^
      11 warnings and 5 errors generated.
      error: Command "gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -g -arch arm64 -arch arm64 -I/private/var/folders/76/zy5ktkns50v6gt5g8r0sf6sc0000gn/T/cibw-run-ku3c16ly/cp39-macosx_arm64/build/venv/include -I/Library/Frameworks/Python.framework/Versions/3.9/include/python3.9 -c src/flint/pyflint.c -o build/temp.macosx-11.0-arm64-cpython-39/src/flint/pyflint.o" failed with exit status 1
      [end of output]
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for python-flint
  Running setup.py clean for python-flint
Failed to build python-flint
ERROR: Failed to build one or more wheels

I saw at least some of those errors but as warnings locally on Linux and the Linux job completes successfully in CI. I guess this is clang being stricter than gcc.

@oscarbenjamin
Copy link
Collaborator Author

The functions being complained about are all after the newline here:
https://github.com/fredrik-johansson/python-flint/blob/027dd1fdae698c95ca8b7f8cadb095536e5c013b/src/flint/_flint.pxd#L154-L163
It complains about each of them except for the first nmod_poly_is_irreducible.

@oscarbenjamin
Copy link
Collaborator Author

The functions being complained about are all after the newline here:

I guess it is because these were moved from nmod_poly.h to nmod_poly_factor.h.

@oscarbenjamin
Copy link
Collaborator Author

Okay, I think that might be it. In the previous commit Linux and OSX were working but the Windows build failed at the final repair wheel step because the DLLs for Arb are now placed in bin rather than lib (I think that is correct for Windows - GMP, MPFR and Flint already put their DLLs in bin).

Hopefully the last commit will have the Windows build working and then everything here is ready and working with flint 3.0.0-alpha1.

It is good to have a branch here with CI passing ready for the flint 3.0.0 release but I don't propose to merge this until flint itself is released. When there is either a new prerelease or a final release of 3.0.0 the version that is pinned here to 3.0.0-alpha1 in bin/build_variables.sh should be updated to see that CI still passes.

@oscarbenjamin
Copy link
Collaborator Author

So the Windows build succeeded and the normal tests passed but the doctests have crashed somehow:

python test/dtest.py
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    pythonLocation: C:\hostedtoolcache\windows\Python\[3](https://github.com/fredrik-johansson/python-flint/actions/runs/5761150407/job/15619523225?pr=43#step:6:3).9.13\x6[4](https://github.com/fredrik-johansson/python-flint/actions/runs/5761150407/job/15619523225?pr=43#step:6:4)
    PKG_CONFIG_PATH: C:\hostedtoolcache\windows\Python\3.9.13\x[6](https://github.com/fredrik-johansson/python-flint/actions/runs/5761150407/job/15619523225?pr=43#step:6:6)4/lib/pkgconfig
    Python_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.9.13\x64
    Python2_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.9.13\x64
    Python3_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.[9](https://github.com/fredrik-johansson/python-flint/actions/runs/5761150407/job/15619523225?pr=43#step:6:9).13\x64
Flint exception (General error):
    mkstemp failed
Error: Process completed with exit code 1.

I'll see if I can reproduce that locally.

@oscarbenjamin
Copy link
Collaborator Author

I can reproduce the doctest crash locally on Windows with the wheel downloaded from CI (we should add -v in CI):

$ python ../../test/dtest.py -v
...
Trying:
    fmpz(10**35 + 1).factor()
Expecting:
    [(11, 1), (9091, 1), (909091, 1), (4147571, 1), (265212793249617641, 1)]
Flint exception (General error):
    mkstemp failed

@oscarbenjamin
Copy link
Collaborator Author

I guess that error comes from here:
https://github.com/flintlib/flint2/blob/ef89816abda221fc40cb4a960430cb570ef0f43c/src/qsieve/factor.c#L222-L225
The question is what exactly has changed in flint-3.0.0-alpha1 that would cause that to fail.

@oscarbenjamin
Copy link
Collaborator Author

git blame points to flintlib/flint#1334

@oscarbenjamin
Copy link
Collaborator Author

It is expected that the "Build / Install from PyPI sdist on Ubuntu" CI job would fail here. Until a sdist containing this PR is pushed to PyPI the build instructions in the script doing that install will not work because they do not build arb separately and that will not work with the python-flint 0.4.1 sdist that is still on PyPI. The Build / Install from git checkout on Ubuntu job works and so that (hopefully) implies that the install from PyPI script will work after python-flint issues a flint-3 compatible release.

@oscarbenjamin
Copy link
Collaborator Author

It is also expected that the tests will fail on Windows with mkstemp failed. That was a Flint bug which was fixed in flintlib/flint#1416. When this PR can bump from Flint 3.0.0-alpha1 to the next prerelease of Flint then the Windows tests should work (I have tested this locally).

@oscarbenjamin
Copy link
Collaborator Author

It is also expected that the tests will fail on Windows with mkstemp failed.

Actually now that CI completes the Windows tests have not failed. I am not sure why they work now when they did not before...

The PR still builds with Flint: 3.0.0-alpha1 that does not contain the fix for the failure seen previously and I haven't changed anything in python-flint that should affect this so having the Windows tests pass in CI is a mystery.

The one failure that is seen (build from PyPI) is still expected though. I just changed the Linux build instructions/demo script in this PR in a way that is not compatible with the current release of python-flint and won't be until a new release is made based on flint-3.

@oscarbenjamin
Copy link
Collaborator Author

Rebased after #65

@@ -1,6 +1,6 @@
from flint._flint cimport mp_limb_t, mp_bitcnt_t

cdef extern from "flint/nmod_vec.h":
cdef extern from "flint/nmod.h":
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deinst I needed to change this for clang (OSX) and Flint 3.

I'm not sure why the problem only shows up with clang and Flint 3. Probably it should changed on master as well: see the last commit in this PR. I don't think it's urgent given that a fix is here ready for Flint 3 when it gets released (I guess that is soon).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My wife's sister and her husband are visiting for a few days so my work will be sporadic, but the new nmod.pxd will land next week at the latest. I have been updating the flintlib submodule with all the functions from the documentation, and have found a couple of doc bugs. This should be done in the next few days

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to rush!

I doubt there will be a release of python-flint before flint 3 so at least the change above will be merged by then from this PR if not another.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'm putting out a release right now (#69) due to (#66).

It's all fine though I think for now.

@GiacomoPope
Copy link
Contributor

With the new scraping tool for flintlib, is the migration from flint2 -> flint3 going to happen soon? Happy to help if this is the case :)

@oscarbenjamin
Copy link
Collaborator Author

I you would like to fix this PR (which has merge conflicts now) and open a new PR I would be happy to merge it and move to flint3.

@GiacomoPope
Copy link
Contributor

I guess this PR can now be closed?

@oscarbenjamin oscarbenjamin deleted the pr_flint3 branch February 1, 2024 23:26
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