This is the current error I get when running tests, but the compilation is OK

Jack: python-flint % source bin/activate                                
Jack: python-flint % python3 -m flint.test
Traceback (most recent call last):
  File "<frozen runpy>", line 189, in _run_module_as_main
  File "<frozen runpy>", line 112, in _get_module_details
  File "/Users/Jack/Documents/GitHub/python-flint/src/flint/__init__.py", line 1, in <module>
    from ._flint import *
  File "src/flint/pyflint.pyx", line 1, in init flint._flint
    """
ModuleNotFoundError: No module named 'flint.flint_base.flint_context

ModuleNotFoundError: No module named 'flint.flint_base.flint_context

This error message indicates that at runtime there is expected to be a module of this name because of this line:

from .flint_base.flint_context cimport FlintContext

I don't remember exactly how all of this works in Cython but the changes you have made here will not create that module because it either needs to be a .py file or an extension module but only one extension module is created:

Ahh ok, yeah so it's a problem with how setup.py is written and I need to properly learn the right way to do this.

A quick google linked to: https://stackoverflow.com/questions/44043620/how-to-prepare-cython-submodules which has an example: https://github.com/justou/cython_package_demo/blob/master/setup.py

import os
from setuptools import setup, find_packages
from Cython.Build import cythonize


def find_pyx(path='.'):
    pyx_files = []
    for root, dirs, filenames in os.walk(path):
        for fname in filenames:
            if fname.endswith('.pyx'):
                pyx_files.append(os.path.join(root, fname))
    return pyx_files


setup(
    name='dvedit',
    version='0.1',
    ext_modules=cythonize(find_pyx(), language_level=3),
    packages=find_packages(),
    )

But simply recursively searching like this seems like not the best practice, so maybe the best is simply to point to submodules as they are written.

From here:
https://cython.readthedocs.io/en/latest/src/userguide/sharing_declarations.html#the-cimport-statement

It is important to understand that the cimport statement can only be used to import C data types, C functions and variables, and extension types. It cannot be used to import any Python objects, and (with one exception) it doesn’t imply any Python import at run time. If you want to refer to any Python names from a module that you have cimported, you will have to include a regular import statement for it as well.

The exception is that when you use cimport to import an extension type, its type object is imported at run time and made available by the name under which you imported it. Using cimport to import extension types is covered in more detail below.

But simply recursively searching like this seems like not the best practice

Better to list them explicitly I think. We will need to translate this to another build system later.

The good news is simply modifying the setup as you would expect works, but I just move the error. From googling, this one seems to be a python3 vs. Python2 issue -- in fact, i see some xrange in certain places which is python2 syntax and generally this is EOL and shouldn't be supported IMO

Are you using python2 still? Confused about why my new module is making this get so upset... I'll debug, but probably tomorrow. My hope is once I get one submodule working, everything else will be the same. All this cython stuff is very new to me!

Jack: python-flint % python3 -m flint.test
Traceback (most recent call last):
  File "<frozen runpy>", line 189, in _run_module_as_main
  File "<frozen runpy>", line 112, in _get_module_details
  File "/Users/Jack/Documents/GitHub/python-flint/src/flint/__init__.py", line 1, in <module>
    from ._flint import *
  File "src/flint/pyflint.pyx", line 1, in init flint._flint
    """
ImportError: dynamic module does not define module export function (PyInit_flint_context)

EDIT:

There's something worse going on. If I comment out the above change to setup.py then my code nicely compiles the code and produces pyflint.c and finds all the functions in subdirectories.

But with the new extension in setup.py, without a .c in place at the beginning, I get dozens of errors when I attempt to cythonize the submodule so I must have something very wrong.

Are you using python2 still?

No.

Supported Python versions are 3.9, 3.10 and 3.11. A new build system is needed for 3.12 (#52).

Separately there is the concept of the Cython language level but that is set to 3:

OK I think the issue is not anything very complicated, it's just more relative import issues.

[1/1] Cythonizing src/flint/flint_base/flint_context.pyx

Error compiling Cython file:
------------------------------------------------------------
...
from ._flint cimport (
^
------------------------------------------------------------

src/flint/flint_base/flint_context.pyx:1:0: 'flint/flint_base/_flint.pxd' not found

So when it tries to cythonize this as a submodule, it looks to:

from ._flint cimport

then reads this from the current directory rather than the directory relative to the top of the module, so maybe rather than Extension I need something like SubExtension or something, I'm just making stupid mistakes now, so I should do this after a good nights sleep

Now I get a meaningful error:

Error compiling Cython file:
------------------------------------------------------------
...
    flint_set_num_threads
)
from flint.utils.conversion cimport prec_to_dps, dps_to_prec

cdef class FlintContext:
    cdef public bint pretty
                     ^
------------------------------------------------------------

src/flint/flint_base/flint_context.pyx:11:21: C attributes cannot be added in implementation part of extension type defined in a pxd

Which makes sense, as I've written a cython class and I'm trying to allow it to be a submodule and it's getting cross at the c-types, so I need to go back to the cython man

after a good nights sleep

Always worth having!

For what you are trying to do I would not be completely confident about exactly what code should be written or immediately how to diagnose any problem. In that situation I would make changes very carefully, fully testing things as I go, and attempting to confirm (or refute) my understanding at each stage so that I can learn as I go. It is easier to do this if you make very small changes one at a time.

Yeah, that was kind of my goal with these initial commits. My hope was taking a single class from one file and sticking it in a submodule would be enough, but I'm already fighting these annoying bugs.

Understanding when an import is really importing from a .so and when it's importing from python seems to be the key, and from answers like this: https://stackoverflow.com/questions/44043620/how-to-prepare-cython-submodules it's hard to know if it's a case of cythonizing more files (by adding extensions) or by modifying the __init__.py files to link things together.

Good news! Sleep was the right choice. I realised the problem was that the attributes initialised in FlintContext needed to be defined within the pxd rather than the pyx to ensure that the c-types were handled by C and not within python.

So in this example:

cdef class FlintContext:
    cdef public bint pretty
    cdef public long _prec
    cdef public long _dps
    cdef arf_rnd_t rnd
    cdef public bint unicode
    cdef public long _cap

cdef class FlintContext:
    def __init__(self):
        self.default()

    def default(self):
        self.pretty = True
        self.rnd = ARF_RND_DOWN
        self.prec = 53
        self.unicode = False
        self.threads = 1
        self.cap = 10

    # SNIP!

The code now compiles and all the tests pass, so I think I can just keep doing work like this and the code should get cleaned up :)

I'm a bit stumped onto how to refactor with these global ctx and thectx.

For example, say I make a submodule for flint_elem, this needs access to ctx

cdef class FlintElem:
    def __repr__(self):
        if ctx.pretty:
            return self.str()
        else:
            return self.repr()

    def __str__(self):
        return self.str()

Let's say the above is in /flint/flint_base/flint_elem.pyx and I include this Extension so that I can do from flint.flint_base.flint_elem import FlintElem. This is what we would want to make on the child extensions, such as FlintScalar(FlintElem) and ultimately something like `nmod(FlintScalar), which we would want to make accessible within the module for people to use.

This means I would need within pyflint.pyx to have something like from flint.nmod.nmod import nmod, but if I'm importing ctx from pyflint.pyx to access the global for FlintElem I'll end up having circular imports.

I've tried searching around, but I can't see an example of how to handle these module globals. I wondered if I need to declare something within flint/__init__.py or something, but it's not obvious.

Advice appreciated

It looks like there is a problem building the sdist though:

You can see how that is done in CI here:

The problem is very stupid: I didn't add the new .pyx on the latest commit.

The problem is very stupid

We've all been there. Looks like it's working now!

I'll probably just merge this once CI passes. I think once I've merged the PR that will mean that CI runs automatically for your PRs in future (without me needing to "approve" each run).

I guess the next thing is to take one of the proper flint classes and put it into its own submodule.

Yes, I agree. First though it would be good to spell out a plan in #15 for what the overall module layout should be so that we are clear what the expected end state looks like both in terms of what runtime modules there would be, and what the source layout would be.

OK, let's make sure everything passes, if there are errors I can try and correct for them but I will leave further refactoring of the repo until we have a discussion in #15. I'm also going on vacation soon, so potentially depending on timings the work will be done somewhere other than this branch.

You need to mark the PR as "ready for review" before it can be merged.

I guess another thing to think about is adding something like an AUTHORS file so we can record attributions.

Looks like there is a problem with using the built wheels:

      + sh -c 'python -c "import flint; print(str(flint.fmpz(2)))"'
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
    File "/tmp/tmp.k9DHkafGnO/venv/lib/python3.9/site-packages/flint/__init__.py", line 1, in <module>
      from ._flint import *
    File "src/flint/pyflint.pyx", line 1, in init flint._flint
    File "src/flint/flint_base/flint_base.pyx", line 1, in init flint.flint_base.flint_base
  ModuleNotFoundError: No module named 'flint._global_context'
                                                             ✕ 11.01s
Error: Command ['sh', '-c', 'python -c "import flint; print(str(flint.fmpz(2)))"'] failed with code 1. 

Maybe there needs to be a runtime module for _global_context or something?

Hmm ok. I actually don't really like this global_context file. It seems redundant. I want to have the class definition within flint_base.flint_context and then import thectx from here, but when I tried that thectx was None so I obviously was doing something stupid.

I'll see if I can simplify the current context handling and that should fix the current issue with wheels too.

OK, so I found a way to delete the strange global file and everything seems to be OK.

I have removed flint_poly from the refactor until we can import acb_poly to address issue #62, so the code is there, but commented out.

Happy for this to be reviewed for merging now, it seems to be working my end and I dont think I've made any breaking changes! (Famous last words??)

Okay, I think that this looks good.

Let's get it merged. Thanks!

Oh, there are merge conflicts.

I think it is because this predates #58.

I can address those in a few hours. Also we have the option to depreciate roots() in this merge?

Also we have the option to depreciate roots() in this merge?

Yes, that could be done as well.

I'm very confused by what happened here. I clicked the merge button but then GitHub told me there were merge conflicts. Now I see that it was merged and the changes from here are in master but this shows as "closed" rather than "merged".

Yeah I have no idea. When I saw it was closed, I was going to ping you to ask you opened it again, then I noticed it had been merged... Maybe when I resolved the merge conflicts my end GitHub automatically finished the merge? I have no idea.