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

Allow cimport for quicktions #5

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
479ad2d
Add pxd to allow cimport from other cython modules
nocarryr Apr 6, 2018
4ab0b49
Add test case for cimporting from another cython module
nocarryr Apr 6, 2018
2a10e91
Rename top-level src directory so it can be imported as a package
nocarryr Apr 6, 2018
0a6e8e2
Modify extension build methods to match layout
nocarryr Nov 12, 2018
cee0127
Merge branch 'master' into pxd-header
nocarryr Nov 12, 2018
b59f3ba
Merge branch 'master' into pxd-restructure
nocarryr Nov 12, 2018
278516c
Add language level directive to pxd and unittest cython module
nocarryr Nov 12, 2018
18f7389
Update paths in Makefile
nocarryr Nov 12, 2018
46daa88
Add language directive to pxd
nocarryr Nov 12, 2018
3be2c4e
Add module-level pxd
nocarryr Nov 12, 2018
13b03b5
Add language level to pyximport, handle extension teardown properly
nocarryr Nov 12, 2018
32fe9ea
Import docstrings from quicktions.pyx at package level
nocarryr Nov 12, 2018
53e1250
Handle py2 imports properly
nocarryr Nov 12, 2018
08ac125
Compatibility fix for py2.6
nocarryr Nov 12, 2018
f628e89
Merge branch 'master' into pxd-header
nocarryr Jan 22, 2019
6de8ea4
Use pre-built wheel to install and run tests against
nocarryr Jan 22, 2019
5cb185a
Move wrapper code and typedefs into pxd
nocarryr Jan 23, 2019
19ea21c
Merge branch 'pxd-header' into pxd-restructure
nocarryr Jan 24, 2019
b7d99f4
Move to src layout with subdirectory. Move test module to project root
nocarryr Jan 25, 2019
f8894e9
Remove unused imports and assertions
nocarryr Jan 25, 2019
fad41af
Correct paths for "make test"
nocarryr Jan 25, 2019
dde7654
Add comment explaining wheel installation step in travis config
nocarryr Jan 25, 2019
efcca15
Make __version__ and __all__ variables available at the package level
nocarryr Jan 25, 2019
0536e1e
Place Cython-related tests into separate module
nocarryr Feb 15, 2019
5a477cc
Use Cythonize module instead of pyximport to compile test modules
nocarryr Feb 15, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ __pycache__

/build
/dist
src/*.c
src/*.html
src/quicktions/*.c
src/quicktions/*.html
MANIFEST

.tox
Expand Down
6 changes: 4 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ language: python
install:
- pip install -r requirements-appveyor.txt tox-travis
- python setup.py build_ext --inplace --with-cython

- python setup.py sdist bdist_wheel
- make clean
# Install quicktions from the wheel built above (into site-packages)
- pip install --no-index --find-links=dist/ quicktions
script:
- tox
- python setup.py sdist bdist_wheel
# the following is stolen from https://github.com/joerick/pyinstrument_cext/blob/master/.travis.yml
# uncomment to push wheels automatically to pypi for tagged releases only (requires TWINE_PASSWORD to be set)
# - |
Expand Down
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
include MANIFEST.in LICENSE *.rst
include setup.py *.yml tox.ini *.cmd *.txt
include test_fractions.py
recursive-include src *.py *.pyx *.pxd *.c *.html
recursive-include benchmark *.py telco-bench.b
10 changes: 6 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
PYTHON?=python
VERSION?=$(shell sed -ne "s|^__version__\s*=\s*'\([^']*\)'.*|\1|p" src/quicktions.pyx)
PKG_ROOT?=src/quicktions
VERSION?=$(shell sed -ne "s|^__version__\s*=\s*'\([^']*\)'.*|\1|p" $(PKG_ROOT)/quicktions.pyx)
PACKAGE=quicktions
WITH_CYTHON := $(shell python -c 'from Cython.Build import cythonize' 2>/dev/null && echo "--with-cython")

Expand All @@ -19,13 +20,14 @@ dist/$(PACKAGE)-$(VERSION).tar.gz:
$(PYTHON) setup.py sdist $(WITH_CYTHON)

test: local
PYTHONPATH=src $(PYTHON) src/test_fractions.py
PYTHONPATH=$(PKG_ROOT) $(PYTHON) test_fractions.py

clean:
rm -fr build src/*.so
rm -fr build $(PKG_ROOT)/*.so
rm -r src/quicktions.egg-info

realclean: clean
rm -fr src/*.c src/*.html
rm -fr $(PKG_ROOT)/*.c $(PKG_ROOT)/*.html

wheel_manylinux: wheel_manylinux64 wheel_manylinux32

Expand Down
40 changes: 22 additions & 18 deletions setup.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@

import os
import sys
import re

Expand All @@ -9,10 +10,16 @@

from distutils.core import setup, Extension

try:
from Cython.Build import cythonize
import Cython.Compiler.Options as cython_options
cython_options.annotate = True
cython_available = True
except ImportError:
cython_available = False
cython = None

ext_modules = [
Extension("quicktions", ["src/quicktions.pyx"]),
]
PKG_ROOT = os.path.join('src', 'quicktions')

try:
sys.argv.remove("--with-profile")
Expand All @@ -21,29 +28,23 @@
else:
enable_profiling = True

ext_modules = None
try:
sys.argv.remove("--with-cython")
except ValueError:
cythonize = None
else:
try:
from Cython.Build import cythonize
import Cython.Compiler.Options as cython_options
cython_options.annotate = True
except ImportError:
cythonize = None
else:
if cython_available:
compiler_directives = {}
if enable_profiling:
compiler_directives['profile'] = True
ext_modules = cythonize(ext_modules, compiler_directives=compiler_directives)

if cythonize is None:
for ext_module in ext_modules:
ext_module.sources[:] = [m.replace('.pyx', '.c') for m in ext_module.sources]

ext_modules = cythonize(os.path.join(PKG_ROOT, '*.pyx'), compiler_directives=compiler_directives)
if ext_modules is None:
ext_modules = [
Extension("quicktions.quicktions", [os.path.join(PKG_ROOT, "quicktions.c")]),
]

with open('src/quicktions.pyx') as f:
with open(os.path.join(PKG_ROOT, 'quicktions.pyx')) as f:
version = re.search("__version__\s*=\s*'([^']+)'", f.read(2048)).group(1)

with open('README.rst') as f:
Expand All @@ -65,7 +66,10 @@
#bugtrack_url="https://github.com/scoder/quicktions/issues",

ext_modules=ext_modules,
package_dir={'': 'src'},
package_dir={'':'src'},
packages=['quicktions'],
package_data={'quicktions':['*.pxd']},
include_package_data=True,

classifiers=[
"Development Status :: 6 - Mature",
Expand Down
2 changes: 2 additions & 0 deletions src/quicktions/__init__.pxd
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
from .quicktions import Fraction, _gcd
from quicktions.quicktions cimport Fraction, _gcd
2 changes: 2 additions & 0 deletions src/quicktions/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
from .quicktions import Fraction, _gcd
from .quicktions import __doc__, __version__, __all__
39 changes: 39 additions & 0 deletions src/quicktions/quicktions.pxd
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# cython: language_level=3str
## cython: profile=True

cdef extern from *:
"""
#if PY_VERSION_HEX < 0x030500F0 || !CYTHON_COMPILING_IN_CPYTHON
#define _PyLong_GCD(a, b) (NULL)
#endif
"""
# CPython 3.5+ has a fast PyLong GCD implementation that we can use.
int PY_VERSION_HEX
int IS_CPYTHON "CYTHON_COMPILING_IN_CPYTHON"
_PyLong_GCD(a, b)

ctypedef unsigned long long ullong
ctypedef unsigned long ulong
ctypedef unsigned int uint

ctypedef fused cunumber:
ullong
ulong
uint

cpdef _gcd(a, b)
cdef ullong _abs(long long x)
cdef cunumber _igcd(cunumber a, cunumber b)
cdef cunumber _ibgcd(cunumber a, cunumber b)
cdef _py_gcd(ullong a, ullong b)
cdef _gcd_fallback(a, b)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think any of these should be exported. They are really just implementation details and subject to change at any time.

Copy link
Author

Choose a reason for hiding this comment

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

Similarly to above this may not be necessary. I'll do some testing


cdef class Fraction:
cdef _numerator
cdef _denominator
cdef Py_hash_t _hash
scoder marked this conversation as resolved.
Show resolved Hide resolved

cpdef limit_denominator(self, max_denominator=*)
cpdef conjugate(self)
cdef _eq(a, b)
cdef _richcmp(self, other, int op)
Copy link
Owner

@scoder scoder Jan 25, 2019

Choose a reason for hiding this comment

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

I wonder if it's possible to leave out these two (_eq() and _richcmp()). They are really not public, and they are also @final methods. In the worst case, we could turn them into functions instead of methods.

Copy link
Author

Choose a reason for hiding this comment

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

All methods and attributes in extension types must be declared in the pxd.

It doesn't alter the behavior outside of Cython, but it does allow the accelerated use of those methods by other Cython modules (whether directly or indirectly).

25 changes: 2 additions & 23 deletions src/quicktions.pyx → src/quicktions/quicktions.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,6 @@ cdef pow10(Py_ssize_t i):

# Half-private GCD implementation.

cdef extern from *:
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep this in here. It's strictly an internal implementation detail of the module.

Copy link
Author

Choose a reason for hiding this comment

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

I am trying to remember if this being in the pyx caused compilation errors or not, but will test

"""
#if PY_VERSION_HEX < 0x030500F0 || !CYTHON_COMPILING_IN_CPYTHON
#define _PyLong_GCD(a, b) (NULL)
#endif
"""
# CPython 3.5+ has a fast PyLong GCD implementation that we can use.
int PY_VERSION_HEX
int IS_CPYTHON "CYTHON_COMPILING_IN_CPYTHON"
_PyLong_GCD(a, b)


cpdef _gcd(a, b):
Expand All @@ -94,14 +84,6 @@ cpdef _gcd(a, b):
return _PyLong_GCD(a, b)


ctypedef unsigned long long ullong
ctypedef unsigned long ulong
ctypedef unsigned int uint

ctypedef fused cunumber:
ullong
ulong
uint


cdef ullong _abs(long long x):
Expand Down Expand Up @@ -244,9 +226,6 @@ cdef class Fraction:
Fraction(147, 100)

"""
cdef _numerator
cdef _denominator
cdef Py_hash_t _hash

def __cinit__(self, numerator=0, denominator=None, *, bint _normalize=True):
cdef Fraction value
Expand Down Expand Up @@ -388,7 +367,7 @@ cdef class Fraction:
else:
return cls(digits, pow10(-exp))

def limit_denominator(self, max_denominator=1000000):
cpdef limit_denominator(self, max_denominator=1000000):
"""Closest Fraction to self with denominator at most max_denominator.

>>> Fraction('3.141592653589793').limit_denominator(10)
Expand Down Expand Up @@ -607,7 +586,7 @@ cdef class Fraction:
"Real numbers have no imaginary component."
return 0

def conjugate(self):
cpdef conjugate(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Do you really think this method is worth cpdef-ing?

Copy link
Author

Choose a reason for hiding this comment

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

Probably not since there's no typing or any other cython-related code there. It was mainly to reduce context switches between Python and Cython calls, but there's really not much going on in that method.

"""Conjugate is a no-op for Reals."""
return +self

Expand Down
1 change: 1 addition & 0 deletions src/test_fractions.py → test_fractions.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

from __future__ import division

import os
from decimal import Decimal
import math
import numbers
Expand Down
59 changes: 59 additions & 0 deletions test_quicktions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import os
import glob
import unittest

from Cython.Build import Cythonize

import quicktions
F = quicktions.Fraction
gcd = quicktions._gcd

class CImportTest(unittest.TestCase):

def setUp(self):
self.module_files = []

def tearDown(self):
for fn in self.module_files:
if os.path.exists(fn):
os.remove(fn)

def build_test_module(self):
module_code = '\n'.join([
'# cython: language_level=3str',
'from quicktions cimport Fraction',
'def get_fraction():',
' return Fraction(1, 2)',
])
base_path = os.path.abspath(os.path.dirname(__file__))
module_name = 'quicktions_importtest'
module_filename = os.path.join(base_path, '.'.join([module_name, 'pyx']))
with open(module_filename, 'w') as f:
f.write(module_code)

Cythonize.main(['-i', module_filename])
Copy link
Owner

Choose a reason for hiding this comment

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

You can pass a (temporary) build directory here via -s build_dir=.... That should simplify the cleanup.


for fn in glob.glob(os.path.join(base_path, '.'.join([module_name, '*']))):
self.module_files.append(os.path.abspath(fn))

def test_cimport(self):
self.build_test_module()

from quicktions_importtest import get_fraction

self.assertEqual(get_fraction(), F(1,2))


def test_main():
suite = unittest.TestSuite()
suite.addTest(unittest.makeSuite(CImportTest))
return suite

def main():
suite = test_main()
runner = unittest.TextTestRunner(sys.stdout, verbosity=2)
result = runner.run(suite)
sys.exit(not result.wasSuccessful())

if __name__ == '__main__':
main()
4 changes: 2 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ platform =
linux: linux
darwin: darwin
passenv = *
commands = coverage run --parallel-mode -m pytest src/test_fractions.py --capture=no --strict {posargs}
commands = coverage run --parallel-mode -m pytest --capture=no --strict {posargs}
coverage combine
coverage report -m --include=src/test_fractions.py
coverage report -m --include=test_fractions.py
Copy link
Owner

Choose a reason for hiding this comment

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

This should include the new test file.

{windows,linux}: codecov