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

Mod experiment #65

Merged
merged 39 commits into from
Sep 6, 2023
Merged

Mod experiment #65

merged 39 commits into from
Sep 6, 2023

Conversation

deinst
Copy link
Contributor

@deinst deinst commented Sep 6, 2023

Ok This compiles and passes tests on my machine.

Most of the code that interfaces to flint has been separated into pxd files in the flintlib submodule. All that is left in _flint.pxd is some basic flint and gmp code that will stay in _flint or wbatever it gets renamed to and a bunch of statements of the form from flint.flintlib.foo cimport foo_t that just reexports interfaces from flintlib. These reexports should be easy to replace, but I have had no luck. Everytime I try to replace from flint._flint cimport foo_t with flint.flintlib.foo cimport foo_t errors pop up in unrelated places.

So I would like to know if this works on other machines, and if other people have weird problems. Regardless it is late, 90 degrees F and my brain is dead. I'd like to see if it works on the CI machines. I'd discourage merging it until the weird behavior is understood.

There are a few other things that I know are less than gracefully handled.

  1. I was not sure if functions.pyx belongs in utils or types so I did nothing. Once a decision is made this should be easy to deal with.

  2. I left the ctx -- thectx in pretty much the same state as it has always been. I tried a bunch or things and could not get consistent behavior between Python and Cython.

  3. I made FMPZ_UNKNOWN, FMPZ_REF and FMPZ_TMP into an anonymous enum. The DEF's do not get exported and I am loath to sprinkle includes in a bunch of files.

Made fmpz into its own module (which depends on the other code
which depends on it).  Hopefully this tedious approach works.
Also got rid of the pyflint/_flint twist.  I'm not sure how it worked, but
Things should be straightforward from now on.
This was straightforward.  (The acb doctests are hosed though)
slowly but surely we are advancing
For unknown reasons fmpz_mat and fmpq_mat were not being imported into
the doctest environment.
Added fmpq.pxd and rerouted imports
All the tests work.  The doctests run are diminishing for now
nmod_series is just a stub
The light at the end of the tunnel appears
Although this change sounds trivial, and mostly is, it involves changing the
module names so that they do not conflict with the names or the enclosed
classes, thereby managing to touch almost everything.

The other minor change is importing newly separated modules into the doctests
that need them.
getting close
This took a lot of adding showgood into to doctests.  Not sure how to do it better.
Also changed FMPZ_ENUM etc to be an enum, and deleted bunches of commented dead code.
This should have gone in the last commit.
Going to commit after each include because cython is getting weird (or I'm going insane).
@oscarbenjamin
Copy link
Collaborator

Is this also a more complete listing of the C functions exposed by the FLINT headers?

@oscarbenjamin
Copy link
Collaborator

So the src layout here is:

$ tree -v --filesfirst src
src
└── flint
    ├── __init__.py
    ├── _flint.pxd
    ├── functions.pyx
    ├── pyflint.pxd
    ├── pyflint.pyx
    ├── flint_base
    │   ├── __init__.py
    │   ├── flint_base.pxd
    │   ├── flint_base.pyx
    │   ├── flint_context.pxd
    │   └── flint_context.pyx
    ├── flintlib
    │   ├── __init__.pxd
    │   ├── __init__.py
    │   ├── acb.pxd
    │   ├── acb_calc.pxd
    │   ├── acb_dft.pxd
    │   ├── acb_dirichlet.pxd
    │   ├── acb_elliptic.pxd
    │   ├── acb_hypgeom.pxd
    │   ├── acb_mat.pxd
    │   ├── acb_modular.pxd
    │   ├── acb_poly.pxd
    │   ├── arb.pxd
    │   ├── arb_fmpz_poly.pxd
    │   ├── arb_hypgeom.pxd
    │   ├── arb_mat.pxd
    │   ├── arb_poly.pxd
    │   ├── arf.pxd
    │   ├── arith.pxd
    │   ├── bernoulli.pxd
    │   ├── dirichlet.pxd
    │   ├── fmpq.pxd
    │   ├── fmpq_mat.pxd
    │   ├── fmpq_poly.pxd
    │   ├── fmpz.pxd
    │   ├── fmpz_factor.pxd
    │   ├── fmpz_lll.pxd
    │   ├── fmpz_mat.pxd
    │   ├── fmpz_mpoly.pxd
    │   ├── fmpz_poly.pxd
    │   ├── fmpz_poly_factor.pxd
    │   ├── mag.pxd
    │   ├── mpoly.pxd
    │   ├── nmod_mat.pxd
    │   ├── nmod_poly.pxd
    │   ├── nmod_vec.pxd
    │   ├── partitions.pxd
    │   └── ulong_extras.pxd
    ├── test
    │   ├── __init__.py
    │   ├── __main__.py
    │   └── test.py
    ├── types
    │   ├── __init__.py
    │   ├── acb.pxd
    │   ├── acb.pyx
    │   ├── acb_mat.pxd
    │   ├── acb_mat.pyx
    │   ├── acb_poly.pxd
    │   ├── acb_poly.pyx
    │   ├── acb_series.pxd
    │   ├── acb_series.pyx
    │   ├── arb.pxd
    │   ├── arb.pyx
    │   ├── arb_mat.pxd
    │   ├── arb_mat.pyx
    │   ├── arb_poly.pxd
    │   ├── arb_poly.pyx
    │   ├── arb_series.pxd
    │   ├── arb_series.pyx
    │   ├── arf.pxd
    │   ├── arf.pyx
    │   ├── dirichlet.pxd
    │   ├── dirichlet.pyx
    │   ├── fmpq.pxd
    │   ├── fmpq.pyx
    │   ├── fmpq_mat.pxd
    │   ├── fmpq_mat.pyx
    │   ├── fmpq_poly.pxd
    │   ├── fmpq_poly.pyx
    │   ├── fmpq_series.pxd
    │   ├── fmpq_series.pyx
    │   ├── fmpz.pxd
    │   ├── fmpz.pyx
    │   ├── fmpz_mat.pxd
    │   ├── fmpz_mat.pyx
    │   ├── fmpz_mpoly.pxd
    │   ├── fmpz_mpoly.pyx
    │   ├── fmpz_poly.pxd
    │   ├── fmpz_poly.pyx
    │   ├── fmpz_series.pxd
    │   ├── fmpz_series.pyx
    │   ├── nmod.pxd
    │   ├── nmod.pyx
    │   ├── nmod_mat.pxd
    │   ├── nmod_mat.pyx
    │   ├── nmod_poly.pxd
    │   ├── nmod_poly.pyx
    │   ├── nmod_series.pxd
    │   └── nmod_series.pyx
    └── utils
        ├── __init__.py
        ├── conversion.pxd
        └── typecheck.pxd

6 directories, 100 files

After an in-place build (bin/build_inplace.sh) I have:

$ tree -v --filesfirst src
src
└── flint
    ├── __init__.py
    ├── _flint.pxd
    ├── functions.pyx
    ├── pyflint.c
    ├── pyflint.cpython-311-x86_64-linux-gnu.so
    ├── pyflint.pxd
    ├── pyflint.pyx
    ├── __pycache__
    │   └── __init__.cpython-311.pyc
    ├── flint_base
    │   ├── __init__.py
    │   ├── flint_base.c
    │   ├── flint_base.cpython-311-x86_64-linux-gnu.so
    │   ├── flint_base.pxd
    │   ├── flint_base.pyx
    │   ├── flint_context.c
    │   ├── flint_context.cpython-311-x86_64-linux-gnu.so
    │   ├── flint_context.pxd
    │   ├── flint_context.pyx
    │   └── __pycache__
    │       └── __init__.cpython-311.pyc
    ├── flintlib
    │   ├── __init__.pxd
    │   ├── __init__.py
    │   ├── acb.pxd
    │   ├── acb_calc.pxd
    │   ├── acb_dft.pxd
    │   ├── acb_dirichlet.pxd
    │   ├── acb_elliptic.pxd
    │   ├── acb_hypgeom.pxd
    │   ├── acb_mat.pxd
    │   ├── acb_modular.pxd
    │   ├── acb_poly.pxd
    │   ├── arb.pxd
    │   ├── arb_fmpz_poly.pxd
    │   ├── arb_hypgeom.pxd
    │   ├── arb_mat.pxd
    │   ├── arb_poly.pxd
    │   ├── arf.pxd
    │   ├── arith.pxd
    │   ├── bernoulli.pxd
    │   ├── dirichlet.pxd
    │   ├── fmpq.pxd
    │   ├── fmpq_mat.pxd
    │   ├── fmpq_poly.pxd
    │   ├── fmpz.pxd
    │   ├── fmpz_factor.pxd
    │   ├── fmpz_lll.pxd
    │   ├── fmpz_mat.pxd
    │   ├── fmpz_mpoly.pxd
    │   ├── fmpz_poly.pxd
    │   ├── fmpz_poly_factor.pxd
    │   ├── mag.pxd
    │   ├── mpoly.pxd
    │   ├── nmod_mat.pxd
    │   ├── nmod_poly.pxd
    │   ├── nmod_vec.pxd
    │   ├── partitions.pxd
    │   └── ulong_extras.pxd
    ├── test
    │   ├── __init__.py
    │   ├── __main__.py
    │   ├── test.py
    │   └── __pycache__
    │       ├── __init__.cpython-311.pyc
    │       ├── __main__.cpython-311.pyc
    │       └── test.cpython-311.pyc
    ├── types
    │   ├── __init__.py
    │   ├── acb.c
    │   ├── acb.cpython-311-x86_64-linux-gnu.so
    │   ├── acb.pxd
    │   ├── acb.pyx
    │   ├── acb_mat.c
    │   ├── acb_mat.cpython-311-x86_64-linux-gnu.so
    │   ├── acb_mat.pxd
    │   ├── acb_mat.pyx
    │   ├── acb_poly.c
    │   ├── acb_poly.cpython-311-x86_64-linux-gnu.so
    │   ├── acb_poly.pxd
    │   ├── acb_poly.pyx
    │   ├── acb_series.c
    │   ├── acb_series.cpython-311-x86_64-linux-gnu.so
    │   ├── acb_series.pxd
    │   ├── acb_series.pyx
    │   ├── arb.c
    │   ├── arb.cpython-311-x86_64-linux-gnu.so
    │   ├── arb.pxd
    │   ├── arb.pyx
    │   ├── arb_mat.c
    │   ├── arb_mat.cpython-311-x86_64-linux-gnu.so
    │   ├── arb_mat.pxd
    │   ├── arb_mat.pyx
    │   ├── arb_poly.c
    │   ├── arb_poly.cpython-311-x86_64-linux-gnu.so
    │   ├── arb_poly.pxd
    │   ├── arb_poly.pyx
    │   ├── arb_series.c
    │   ├── arb_series.cpython-311-x86_64-linux-gnu.so
    │   ├── arb_series.pxd
    │   ├── arb_series.pyx
    │   ├── arf.c
    │   ├── arf.cpython-311-x86_64-linux-gnu.so
    │   ├── arf.pxd
    │   ├── arf.pyx
    │   ├── dirichlet.c
    │   ├── dirichlet.cpython-311-x86_64-linux-gnu.so
    │   ├── dirichlet.pxd
    │   ├── dirichlet.pyx
    │   ├── fmpq.c
    │   ├── fmpq.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpq.pxd
    │   ├── fmpq.pyx
    │   ├── fmpq_mat.c
    │   ├── fmpq_mat.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpq_mat.pxd
    │   ├── fmpq_mat.pyx
    │   ├── fmpq_poly.c
    │   ├── fmpq_poly.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpq_poly.pxd
    │   ├── fmpq_poly.pyx
    │   ├── fmpq_series.c
    │   ├── fmpq_series.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpq_series.pxd
    │   ├── fmpq_series.pyx
    │   ├── fmpz.c
    │   ├── fmpz.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpz.pxd
    │   ├── fmpz.pyx
    │   ├── fmpz_mat.c
    │   ├── fmpz_mat.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpz_mat.pxd
    │   ├── fmpz_mat.pyx
    │   ├── fmpz_mpoly.c
    │   ├── fmpz_mpoly.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpz_mpoly.pxd
    │   ├── fmpz_mpoly.pyx
    │   ├── fmpz_poly.c
    │   ├── fmpz_poly.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpz_poly.pxd
    │   ├── fmpz_poly.pyx
    │   ├── fmpz_series.c
    │   ├── fmpz_series.cpython-311-x86_64-linux-gnu.so
    │   ├── fmpz_series.pxd
    │   ├── fmpz_series.pyx
    │   ├── nmod.c
    │   ├── nmod.cpython-311-x86_64-linux-gnu.so
    │   ├── nmod.pxd
    │   ├── nmod.pyx
    │   ├── nmod_mat.c
    │   ├── nmod_mat.cpython-311-x86_64-linux-gnu.so
    │   ├── nmod_mat.pxd
    │   ├── nmod_mat.pyx
    │   ├── nmod_poly.c
    │   ├── nmod_poly.cpython-311-x86_64-linux-gnu.so
    │   ├── nmod_poly.pxd
    │   ├── nmod_poly.pyx
    │   ├── nmod_series.c
    │   ├── nmod_series.cpython-311-x86_64-linux-gnu.so
    │   ├── nmod_series.pxd
    │   ├── nmod_series.pyx
    │   └── __pycache__
    │       └── __init__.cpython-311.pyc
    └── utils
        ├── __init__.py
        ├── conversion.pxd
        └── typecheck.pxd

10 directories, 158 files

So we now have 26 extension modules instead of previously one (or actually two or three already since previous changes):

$ find src -name '*.so'
src/flint/flint_base/flint_base.cpython-311-x86_64-linux-gnu.so
src/flint/flint_base/flint_context.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpz_series.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpz_poly.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpq.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpq_poly.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpz.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpq_series.cpython-311-x86_64-linux-gnu.so
src/flint/types/arb_series.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpz_mat.cpython-311-x86_64-linux-gnu.so
src/flint/types/arb.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpz_mpoly.cpython-311-x86_64-linux-gnu.so
src/flint/types/arf.cpython-311-x86_64-linux-gnu.so
src/flint/types/nmod_series.cpython-311-x86_64-linux-gnu.so
src/flint/types/acb_series.cpython-311-x86_64-linux-gnu.so
src/flint/types/arb_mat.cpython-311-x86_64-linux-gnu.so
src/flint/types/nmod_mat.cpython-311-x86_64-linux-gnu.so
src/flint/types/arb_poly.cpython-311-x86_64-linux-gnu.so
src/flint/types/acb_poly.cpython-311-x86_64-linux-gnu.so
src/flint/types/acb_mat.cpython-311-x86_64-linux-gnu.so
src/flint/types/acb.cpython-311-x86_64-linux-gnu.so
src/flint/types/dirichlet.cpython-311-x86_64-linux-gnu.so
src/flint/types/nmod.cpython-311-x86_64-linux-gnu.so
src/flint/types/fmpq_mat.cpython-311-x86_64-linux-gnu.so
src/flint/types/nmod_poly.cpython-311-x86_64-linux-gnu.so
src/flint/pyflint.cpython-311-x86_64-linux-gnu.so

The total size of the src directory is 43M but that also includes .c files:

$ du -sh src
43M	src

Total size of .so files is 21MB:

$ du -c `find src -name '*.so'`
908	src/flint/flint_base/flint_base.cpython-311-x86_64-linux-gnu.so
480	src/flint/flint_base/flint_context.cpython-311-x86_64-linux-gnu.so
660	src/flint/types/fmpz_series.cpython-311-x86_64-linux-gnu.so
684	src/flint/types/fmpz_poly.cpython-311-x86_64-linux-gnu.so
644	src/flint/types/fmpq.cpython-311-x86_64-linux-gnu.so
680	src/flint/types/fmpq_poly.cpython-311-x86_64-linux-gnu.so
788	src/flint/types/fmpz.cpython-311-x86_64-linux-gnu.so
720	src/flint/types/fmpq_series.cpython-311-x86_64-linux-gnu.so
1208	src/flint/types/arb_series.cpython-311-x86_64-linux-gnu.so
736	src/flint/types/fmpz_mat.cpython-311-x86_64-linux-gnu.so
2028	src/flint/types/arb.cpython-311-x86_64-linux-gnu.so
896	src/flint/types/fmpz_mpoly.cpython-311-x86_64-linux-gnu.so
472	src/flint/types/arf.cpython-311-x86_64-linux-gnu.so
248	src/flint/types/nmod_series.cpython-311-x86_64-linux-gnu.so
1036	src/flint/types/acb_series.cpython-311-x86_64-linux-gnu.so
820	src/flint/types/arb_mat.cpython-311-x86_64-linux-gnu.so
696	src/flint/types/nmod_mat.cpython-311-x86_64-linux-gnu.so
696	src/flint/types/arb_poly.cpython-311-x86_64-linux-gnu.so
732	src/flint/types/acb_poly.cpython-311-x86_64-linux-gnu.so
856	src/flint/types/acb_mat.cpython-311-x86_64-linux-gnu.so
2336	src/flint/types/acb.cpython-311-x86_64-linux-gnu.so
432	src/flint/types/dirichlet.cpython-311-x86_64-linux-gnu.so
352	src/flint/types/nmod.cpython-311-x86_64-linux-gnu.so
680	src/flint/types/fmpq_mat.cpython-311-x86_64-linux-gnu.so
652	src/flint/types/nmod_poly.cpython-311-x86_64-linux-gnu.so
644	src/flint/pyflint.cpython-311-x86_64-linux-gnu.so
21084	total

@oscarbenjamin
Copy link
Collaborator

Total time for a full build from scratch seems about the same.

An incremental rebuild for a single module is much faster though:

$ touch src/flint/types/fmpz.pyx
$ time bin/build_inplace.sh
...
Compiling src/flint/types/fmpz.pyx because it changed.
[1/1] Cythonizing src/flint/types/fmpz.pyx
...
real	0m11.716s
user	0m10.843s
sys	0m1.015s

That's 10 seconds rather than a few minutes previously which is much more manageable for development.

@oscarbenjamin
Copy link
Collaborator

Total time for a full build from scratch seems about the same.

With meson I guess it would be easier to parallelise the build now since it is many smaller modules.

@oscarbenjamin
Copy link
Collaborator

Total size of all wheels is now 159MB vs 142MB previously.

$ du -h old/*
142M	old/artifact.zip
6.4M	old/python_flint-0.4.2-cp310-cp310-macosx_10_9_x86_64.whl
32M	old/python_flint-0.4.2-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
9.5M	old/python_flint-0.4.2-cp310-cp310-win_amd64.whl
6.4M	old/python_flint-0.4.2-cp311-cp311-macosx_10_9_x86_64.whl
32M	old/python_flint-0.4.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
9.5M	old/python_flint-0.4.2-cp311-cp311-win_amd64.whl
6.4M	old/python_flint-0.4.2-cp39-cp39-macosx_10_9_x86_64.whl
32M	old/python_flint-0.4.2-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
9.5M	old/python_flint-0.4.2-cp39-cp39-win_amd64.whl
1.2M	old/python-flint-0.4.2.tar.gz
$ du -h new/*
159M	new/artifact.zip
6.8M	new/python_flint-0.4.2-cp310-cp310-macosx_10_9_x86_64.whl
34M	new/python_flint-0.4.2-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
12M	new/python_flint-0.4.2-cp310-cp310-win_amd64.whl
6.8M	new/python_flint-0.4.2-cp311-cp311-macosx_10_9_x86_64.whl
35M	new/python_flint-0.4.2-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
12M	new/python_flint-0.4.2-cp311-cp311-win_amd64.whl
6.8M	new/python_flint-0.4.2-cp39-cp39-macosx_10_9_x86_64.whl
34M	new/python_flint-0.4.2-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
12M	new/python_flint-0.4.2-cp39-cp39-win_amd64.whl
2.5M	new/python-flint-0.4.2.tar.gz

Most wheels are about 5-10% bigger.

Looks like the sdist doubled in size. I guess that is because it includes all of the C code:

$ tree python-flint-0.4.2
python-flint-0.4.2
├── doc
│   ├── Makefile
│   └── source
│       ├── acb_mat.rst
│       ├── acb_poly.rst
│       ├── acb.rst
│       ├── acb_series.rst
│       ├── arb_mat.rst
│       ├── arb_poly.rst
│       ├── arb.rst
│       ├── arb_series.rst
│       ├── conf.py
│       ├── dirichlet.rst
│       ├── fmpq_mat.rst
│       ├── fmpq_poly.rst
│       ├── fmpq.rst
│       ├── fmpq_series.rst
│       ├── fmpz_mat.rst
│       ├── fmpz_poly.rst
│       ├── fmpz.rst
│       ├── fmpz_series.rst
│       ├── general.rst
│       ├── index.rst
│       ├── nmod_mat.rst
│       ├── nmod_poly.rst
│       ├── nmod.rst
│       ├── nmod_series.rst
│       └── setup.rst
├── LICENSE
├── MANIFEST.in
├── PKG-INFO
├── README.md
├── setup.cfg
├── setup.py
└── src
    ├── flint
    │   ├── flint_base
    │   │   ├── flint_base.c
    │   │   └── flint_context.c
    │   ├── _flint.pxd
    │   ├── functions.pyx
    │   ├── __init__.py
    │   ├── pyflint.c
    │   ├── pyflint.pxd
    │   ├── pyflint.pyx
    │   ├── test
    │   │   ├── __init__.py
    │   │   ├── __main__.py
    │   │   └── test.py
    │   └── types
    │       ├── acb.c
    │       ├── acb_mat.c
    │       ├── acb_poly.c
    │       ├── acb_series.c
    │       ├── arb.c
    │       ├── arb_mat.c
    │       ├── arb_poly.c
    │       ├── arb_series.c
    │       ├── arf.c
    │       ├── dirichlet.c
    │       ├── fmpq.c
    │       ├── fmpq_mat.c
    │       ├── fmpq_poly.c
    │       ├── fmpq_series.c
    │       ├── fmpz.c
    │       ├── fmpz_mat.c
    │       ├── fmpz_mpoly.c
    │       ├── fmpz_poly.c
    │       ├── fmpz_series.c
    │       ├── nmod.c
    │       ├── nmod_mat.c
    │       ├── nmod_poly.c
    │       └── nmod_series.c
    └── python_flint.egg-info
        ├── dependency_links.txt
        ├── PKG-INFO
        ├── SOURCES.txt
        └── top_level.txt

8 directories, 70 files

Actually the sdist does not have any of the Cython code. I guess in principle this should mean that it can build without Cython. Probably it would be better though for the sdist to include the Cython files and just depend on Cython as a build-time dependency.

@oscarbenjamin
Copy link
Collaborator

Installing the CI built wheel I run python and check the memory usage:

$ python
>>>

That's 5.8MB according to the system monitor. Then

>>> import flint

It goes up to 6.9MB

I'm not really sure how all these many megabytes of .so files only amount to 1.1MB of memory at runtime.

Previously importing flint would add about 0.8MB but with the changes here it is about 1.1MB. I think that is fine: I just wanted to check that it would not grow massively.

@oscarbenjamin
Copy link
Collaborator

All tests are passing in CI and locally. I tried both local build and also downloading the CI built wheels.

@oscarbenjamin
Copy link
Collaborator

The installed files (from the CI build wheel look like:

$ tree venv/lib/python3.11/site-packages/flint/
venv/lib/python3.11/site-packages/flint/
├── flint_base
│   ├── flint_base.cpython-311-x86_64-linux-gnu.so
│   └── flint_context.cpython-311-x86_64-linux-gnu.so
├── __init__.py
├── __pycache__
│   └── __init__.cpython-311.pyc
├── pyflint.cpython-311-x86_64-linux-gnu.so
├── test
│   ├── __init__.py
│   ├── __main__.py
│   ├── __pycache__
│   │   ├── __init__.cpython-311.pyc
│   │   ├── __main__.cpython-311.pyc
│   │   └── test.cpython-311.pyc
│   └── test.py
└── types
    ├── acb.cpython-311-x86_64-linux-gnu.so
    ├── acb_mat.cpython-311-x86_64-linux-gnu.so
    ├── acb_poly.cpython-311-x86_64-linux-gnu.so
    ├── acb_series.cpython-311-x86_64-linux-gnu.so
    ├── arb.cpython-311-x86_64-linux-gnu.so
    ├── arb_mat.cpython-311-x86_64-linux-gnu.so
    ├── arb_poly.cpython-311-x86_64-linux-gnu.so
    ├── arb_series.cpython-311-x86_64-linux-gnu.so
    ├── arf.cpython-311-x86_64-linux-gnu.so
    ├── dirichlet.cpython-311-x86_64-linux-gnu.so
    ├── fmpq.cpython-311-x86_64-linux-gnu.so
    ├── fmpq_mat.cpython-311-x86_64-linux-gnu.so
    ├── fmpq_poly.cpython-311-x86_64-linux-gnu.so
    ├── fmpq_series.cpython-311-x86_64-linux-gnu.so
    ├── fmpz.cpython-311-x86_64-linux-gnu.so
    ├── fmpz_mat.cpython-311-x86_64-linux-gnu.so
    ├── fmpz_mpoly.cpython-311-x86_64-linux-gnu.so
    ├── fmpz_poly.cpython-311-x86_64-linux-gnu.so
    ├── fmpz_series.cpython-311-x86_64-linux-gnu.so
    ├── nmod.cpython-311-x86_64-linux-gnu.so
    ├── nmod_mat.cpython-311-x86_64-linux-gnu.so
    ├── nmod_poly.cpython-311-x86_64-linux-gnu.so
    └── nmod_series.cpython-311-x86_64-linux-gnu.so

5 directories, 34 files

That takes 31MB. Also:

$ tree venv/lib/python3.11/site-packages/python_flint.libs/
venv/lib/python3.11/site-packages/python_flint.libs/
├── libarb-4c2cc9a4.so.2.14.0
├── libflint-c1ff2639.so.17.0.0
├── libgmp-e0c82b6b.so.10.5.0
└── libmpfr-90ec1309.so.6.1.0

0 directories, 4 files

Those are huge:

$ du -sh venv/lib/python3.11/site-packages/python_flint.libs/*
25M	venv/lib/python3.11/site-packages/python_flint.libs/libarb-4c2cc9a4.so.2.14.0
57M	venv/lib/python3.11/site-packages/python_flint.libs/libflint-c1ff2639.so.17.0.0
728K	venv/lib/python3.11/site-packages/python_flint.libs/libgmp-e0c82b6b.so.10.5.0
2.8M	venv/lib/python3.11/site-packages/python_flint.libs/libmpfr-90ec1309.so.6.1.0

Is it normal for libflint to be 57MB? Actually my local build is 45MB so I guess so (probably the CI built wheels are larger because of extra bundling or something).

I guess when the .so is loaded we don't have to load the whole thing into memory somehow? Or maybe the system monitor does not include the memory for libflint...

@oscarbenjamin
Copy link
Collaborator

It looks like this change brings a small increase in wheel size and installed disk usage as well as a small increase in runtime memory. Those are all fine I think but still if there is some way to significantly reduce any of those then that would be good. (This is likely an orthogonal point to the PR here though...)

Currently the installed disk usage of python-flint is about 120MB with half of that just being libflint.so. Runtime memory consumption is significantly lower (1MB) which is good. I often wonder how my virtual environments end up using so much disk space but when something like libflint is a single 60MB file I guess that explains it.

@oscarbenjamin
Copy link
Collaborator

Everytime I try to replace from flint._flint cimport foo_t with flint.flintlib.foo cimport foo_t errors pop up in unrelated places.

I'm not sure what you mean by this. I tried this and everything seemed fine:

diff --git a/src/flint/_flint.pxd b/src/flint/_flint.pxd
index 09ba307..8806fb5 100644
--- a/src/flint/_flint.pxd
+++ b/src/flint/_flint.pxd
@@ -70,7 +70,6 @@ cdef extern from *:
     """
     slong pylong_as_slong(PyObject *pylong, int *overflow)
 
-from flint.flintlib.nmod_vec cimport nmod_t
 from flint.flintlib.nmod_poly cimport nmod_poly_t
 from flint.flintlib.nmod_mat cimport nmod_mat_t
 from flint.flintlib.fmpz cimport fmpz_t
diff --git a/src/flint/types/nmod.pxd b/src/flint/types/nmod.pxd
index 4ece832..0a441fe 100644
--- a/src/flint/types/nmod.pxd
+++ b/src/flint/types/nmod.pxd
@@ -1,6 +1,6 @@
 from flint.flint_base.flint_base cimport flint_scalar
 from flint._flint cimport mp_limb_t
-from flint._flint cimport nmod_t
+from flint.flintlib.nmod_vec cimport nmod_t
 
 cdef int any_as_nmod(mp_limb_t * val, obj, nmod_t mod) except -1
 

Btw why is nmod_t in nmod_vec.pxd?

@deinst
Copy link
Contributor Author

deinst commented Sep 6, 2023

Is this also a more complete listing of the C functions exposed by the FLINT headers?
Not yet. I figured I'd do the simple copy/paste thing first, then add the missing functions later.

Btw why is nmod_t in nmod_vec.pxd?
Again this is from the simple copy/paste, because that is the include file that we read it from originally. It is now on my todo list to split them.

Could you try to add this patch and build?

--- a/src/flint/types/arb_mat.pxd
+++ b/src/flint/types/arb_mat.pxd
@@ -1,5 +1,5 @@
 from flint.flint_base.flint_base cimport flint_mat
-from flint._flint cimport arb_mat_t
+from flint.flintlib.arb_mat cimport arb_mat_t

  cdef class arb_mat(flint_mat):
      cdef arb_mat_t val

When I do this, I get an error of

src/flint/_flint.pxd:85:0: 'flint/flintlib/arb/arb_ptr.pxd' not found

Which makes absolutely no sense to me, as the change should have had no effect there.

@oscarbenjamin
Copy link
Collaborator

When I do this, I get an error of
src/flint/_flint.pxd:85:0: 'flint/flintlib/arb/arb_ptr.pxd' not found

I worked through it a bit and saw the same errors. I think that the error is basically just the equivalent of an ImportError. Cython did not find the arb_ptr object in the arb "module" (.pxd file) and so it looked for an arb_ptr.pxd file as if arb was a package. Then it fails and gives this error.

I think that the reason it fails is effectively something like a circular import. There are a bunch of different files importing each other and some point we get back to the importing from the same module but the expected name has not been defined yet (or something like that).

The solution I went for is just to delete everything from _flint.pxd and then patch up the other imports one at a time and it seems to build fine locally now. I've just pushed that here.

@oscarbenjamin
Copy link
Collaborator

  1. I was not sure if functions.pyx belongs in utils or types

The contents of utils does not depend on anything else in python-flint whereas functions.pyx cimports most of the contents of flint.types. From a circularity perspective it does not make sense to put functions.pyx int utils.

Looking at the contents of functions.pyx I think I would break it up. The references to specific types could be replaced by just defining some sort of method that different types can implement. Then it would not have any direct reference to anything apart from ctx. That does not have to be done right now. I'm not sure if it particular gains much from being Cython rather than Python either.

@oscarbenjamin
Copy link
Collaborator

The main thing to with functions.pyx is just to make it a separate module somewhere rather having it as the final remaining include:

include "functions.pyx"

There are various problems with this like it breaks coverage measurement under Cython (see bin/coverage.sh).

Its code could even just be moved into pyflint.pyx. I'm not sure what we otherwise would want to do with pyflint.pyx after you have removed almost everything from it here.

@oscarbenjamin
Copy link
Collaborator

Is there any strong reason not to merge this right now?

I think it looks good and achieves the main part of #15.

This is a huge diff that moves virtually every line in the codebase. If we want it in then I think it is best to merge immediately and make any smaller followup improvements later.

@GiacomoPope
Copy link
Contributor

Maybe one thing before merging is fixing the "functions" file, which also could do with a more descriptive name? I would have suggested to move it into utils, but I see your message above.

As far as I can tell the only function which we need to export is showgood, which is used between:

  • src/flint/dirichlet.pyx‎
  • ‎src/flint/arb_mat.pyx‎
  • ‎src/flint/acb_mat.pyx‎
  • ‎src/flint/acb.pyx‎
  • ‎src/flint/arb.pyx‎

All other functions in functions.pyx are internal. I would call this something like accurate_good_printing.pyx or good_printing.pyx or something

@oscarbenjamin
Copy link
Collaborator

Maybe one thing before merging is fixing the "functions" file, which also could do with a more descriptive name?

I think that we can equally do that after merging though. The question is really just whether anything is majorly wrong with this bulk movement of code that we might regret (I don't think there is).

Leaving a PR like this open for any length of time is problematic because it is difficult to keep it in sync with any other changes. After this is merged a smaller PR that moves a few functions around is fine.

As far as I can tell the only function which we need to export is showgood, which is used between

I think it is only used in doctests so actually there is nowhere in the codebase that uses showgood internally. It could be considered just a function for testing but since it is widely shown in the docs it should probably be thought of as a function for end users.

@GiacomoPope
Copy link
Contributor

Leaving a PR like this open for any length of time is problematic because it is difficult to keep it in sync with any other changes. After this is merged a smaller PR that moves a few functions around is fine.

I agree with this, it's a good point. Once this is merged I can also start trying to help (deinst did such great work here I thought it best for this to be merged before doing work myself again)

@deinst
Copy link
Contributor Author

deinst commented Sep 6, 2023

I don't see any real good reason not to merge it. I have tried to be conscientious about not breaking anything, and if it works merge it. That said, I would not trust my life on the fact that I haven't screwed something up, but if I have we can probably fix it later.

@oscarbenjamin
Copy link
Collaborator

I thought it best for this to be merged before doing work myself again

Exactly. This is a great PR but when something like this is open it sort of blocks other work.

Luckily python-flint does not currently have large numbers of open PRs although some like #43 will need updating after.

I would not trust my life on the fact that I haven't screwed something up, but if I have we can probably fix it later.

I'm confident it will be fine and yes we can definitely fix anything later if needed.

Usually with something like this the problems are fairly obvious so once it looks like it is working it probably is totally fine.

@oscarbenjamin oscarbenjamin merged commit 4fc536d into flintlib:master Sep 6, 2023
@oscarbenjamin
Copy link
Collaborator

Thanks @deinst and @GiacomoPope !

@GiacomoPope
Copy link
Contributor

GiacomoPope commented Sep 6, 2023

Deinst did all the hard work!! I'm really excited to keep working on this repo :)

This was referenced Sep 6, 2023
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