-
Notifications
You must be signed in to change notification settings - Fork 27
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
Introduce submodules into python-flint #61
Conversation
This is the current error I get when running tests, but the compilation is OK
|
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: Lines 66 to 74 in 71bf110
|
Ahh ok, yeah so it's a problem with how 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. |
|
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 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!
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 But with the new extension in |
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: Lines 54 to 57 in 71bf110
That should mean that the Cython code follows Python 3 although maybe xrange is still allowed there...
|
OK I think the issue is not anything very complicated, it's just more relative import issues.
So when it tries to cythonize this as a submodule, it looks to:
then reads this from the current directory rather than the directory relative to the top of the module, so maybe rather than |
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 |
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 |
Good news! Sleep was the right choice. I realised the problem was that the attributes initialised in 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 For example, say I make a submodule for 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 This means I would need within 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 Advice appreciated |
setup.py
Outdated
library_dirs=default_lib_dirs, | ||
include_dirs=default_include_dirs, | ||
define_macros=define_macros, | ||
) | ||
), | ||
Extension( | ||
"flint.flint_base.flint_context", | ||
["src/flint/flint_base/flint_context.pyx"], | ||
libraries=libraries, | ||
library_dirs=default_lib_dirs, | ||
include_dirs=default_include_dirs, | ||
define_macros=define_macros, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to factor this out like:
ext_files = [
('"flint._flint", ["src/flint/pyflint.pyx"]),
...
]
ext_options = {
'libraries': libraries,
...
}
ext_modules = []
for mod_name, src_files in ext_files.items():
ext = Extension(mod_name, src_files, **ext_options)
ext_modules.append(ext)
Then it is easier to add to the ext_files
list to make more extension modules and we don't need to duplicate the arguments in ext_options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, addressed in commit f7c3bfc
You can see how that is done in CI here: python-flint/.github/workflows/buildwheel.yml Lines 60 to 77 in 71bf110
|
The problem is very stupid: I didn't add the new .pyx on the latest commit. |
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).
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:
Maybe there needs to be a runtime module for |
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 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 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! |
Introduce submodules into python-flint
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? |
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. |
The hope for this pull request is a fairly large refactor of the code to remove all include from the
pyflint.pyx
file, and place everything into submodules.I started by first factoring out
FlintContext
and some util functions frompyflint.pyx
, and everything compiles, but when I try and use the module, I get import errors.I can't seem to fix it, I don't know if this is a quirk of cython modules I'm not familiar with, or simply a dumb bug, but I'm hoping for some advice on how to properly import between submodules within the project. The module not found error doesn't make sense to me, as the functions are all found on compile time...
Sorry to have to ask for help so early!