Allow cimport for quicktions #5

nocarryr · 2019-01-25T19:04:03Z

Share the quicktions.Fraction extension type definitions with other Cython modules.

To make distribution possible from a pip install, the sources have to be included in the built wheels. The project layout prevents this (AFAIK) since only the compiled binaries would be included.

Placing quicktions into a sub-directory as a proper package (with __init__.py) seemed the best solution:

* src
  * quicktions
    * __init__.py
    * quicktions.pyx
    * quicktions.pxd

I realize this may be an undesirable change and can understand if it's not a direction you would like to go with this project.

With quicktions nested under 'src/', setuptools can create a proper distribution/wheel. Moving test_fractions.py was needed to avoid import errors from namespace collisions.

scoder

Thanks!
I think it's worth avoiding a package, though. Why not add quicktions.pxd to the package_data with package_data={'': ['quicktions.pxd']}) ? Could you maybe send a new (cleaned up) pull request for that, which avoids all the moving around of files?

scoder · 2019-01-25T20:24:26Z

test_fractions.py

+    def test_cimport(self):
+        self.build_test_module()
+        import pyximport
+        self.py_importer, self.pyx_importer = pyximport.install(inplace=True, language_level=3)


Please don't use pyximport for the tests. It's really outdated.
cython.inline() might work for this.

I did look into that as it would be much cleaner, but from what I've tested, cimport isn't supported:

import cython r = cython.inline('from quicktions cimport Fraction;return Fraction(a,b)', a=1,b=2)

Raises an exception:

AssertionError: Not yet supporting any cimports/includes from string code snippets

I agree that there should be a better way though

test_fractions.py

scoder · 2019-01-25T20:28:15Z

src/quicktions/quicktions.pyx

@@ -607,7 +586,7 @@ cdef class Fraction:
        "Real numbers have no imaginary component."
        return 0

-    def conjugate(self):
+    cpdef conjugate(self):


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

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.

scoder · 2019-01-25T20:29:26Z

src/quicktions/quicktions.pyx

@@ -70,16 +70,6 @@ cdef pow10(Py_ssize_t i):

 # Half-private GCD implementation.

-cdef extern from *:


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

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

src/quicktions/quicktions.pxd

scoder · 2019-01-25T20:31:52Z

src/quicktions/quicktions.pxd

+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)


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

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

test_fractions.py

scoder · 2019-01-25T20:45:15Z

src/quicktions/quicktions.pxd

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


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.

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).

nocarryr · 2019-01-25T22:32:00Z

Thanks!
I think it's worth avoiding a package, though. Why not add quicktions.pxd to the package_data with package_data={'': ['quicktions.pxd']}) ? Could you maybe send a new (cleaned up) pull request for that, which avoids all the moving around of files?

That was my initial endeavor, but I could never get the packaged wheel to include anything other than the compiled binary. This is expected due to the project not containing any valid packages. ("src layout", but no packages within "src/")
https://setuptools.readthedocs.io/en/latest/setuptools.html#using-a-src-layout

My prior (unsuccessful) attempts were here:
https://github.com/nocarryr/quicktions/tree/pxd-header

scoder · 2019-02-17T10:32:48Z

tox.ini

@@ -7,7 +7,7 @@ platform =
    linux: linux
    darwin: darwin
 passenv = *
-commands = coverage run --parallel-mode -m pytest test_fractions.py --capture=no --strict {posargs}
+commands = coverage run --parallel-mode -m pytest --capture=no --strict {posargs}
           coverage combine
           coverage report -m --include=test_fractions.py


This should include the new test file.

scoder · 2019-02-17T10:39:42Z

test_quicktions.py

+        with open(module_filename, 'w') as f:
+            f.write(module_code)
+
+        Cythonize.main(['-i', module_filename])


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

cjdsellers · 2020-09-23T10:51:35Z

Greetings,

I was wondering where we might be at with facilitating cimporting Fraction from quicktions?

scoder · 2020-09-23T14:33:14Z

@cjdsellers difficult to say. To me, it feels like this PR is trying to do way too many things, but @nocarryr apparently got there by trying simpler things first, so I don't know. If there is a simple way to allow from quicktions cimport Fraction, then I'd be happy to get that included.

nocarryr · 2020-09-23T16:35:29Z

@scoder I agree

Without changing the project's layout, it's not possible AFAIK to include and package the .pxd file so things are importable from pure Python and on Cython's search path.

I think I realized the simplicity of quicktions's layout (matching the Python stdlib as closely as possible) was more valuable than the (likely insignificant) performance increase of making it cimportable.

It would seem more beneficial to create that functionality as a completely separate project (which I've started working on here and there). That way it can be designed with much less GIL interaction required.

Should I just close this PR?

cjdsellers · 2020-09-24T20:46:23Z

Ok, I thought it might have been a simple fix with just writing a .pxd. However I understand there are other complexities here around the packaging, and I understand that one of the main values of the library is a drop in replacement for Fraction.

I'm looking for a high performance decimal type object which I can inherit from for C extension modules compiled with Cython. It would provide the base for some value type domain objects for a trading platform I've been developing. All production code has been written with Cython.

https://github.com/nautechsystems/nautilus_trader

I appreciate your response and look forward to anything you may come up with regarding the above.

nocarryr · 2020-09-24T22:47:09Z

@scoder do you think cython/cython#2910 would be a viable alternative to this approach?

scoder · 2020-09-25T06:16:42Z

do you think cython/cython#2910 would be a viable alternative to this approach?

That does not seem related. If quicktions ships a .pxd file, it will be picked up. Cython's own .pxd files are not relevant here.

scoder · 2020-09-25T06:20:11Z

I appreciate your response and look forward to anything you may come up with regarding the above.

Well, you can always define your own quicktions.pxd and use it. Cython does not care where the file came from, as long as it finds it somewhere on the Python import path.

cjdsellers · 2020-09-26T03:30:56Z

Well, you can always define your own quicktions.pxd and use it. Cython does not care where the file came from, as long as it finds it somewhere on the Python import path.

I'll try that approach, thank you!

nocarryr added 23 commits April 6, 2018 11:18

Add pxd to allow cimport from other cython modules

479ad2d

Add test case for cimporting from another cython module

4ab0b49

Rename top-level src directory so it can be imported as a package

2a10e91

Modify extension build methods to match layout

0a6e8e2

Merge branch 'master' into pxd-header

cee0127

Merge branch 'master' into pxd-restructure

b59f3ba

Add language level directive to pxd and unittest cython module

278516c

Update paths in Makefile

18f7389

Add language directive to pxd

46daa88

Add module-level pxd

3be2c4e

Add language level to pyximport, handle extension teardown properly

13b03b5

Import docstrings from quicktions.pyx at package level

32fe9ea

Handle py2 imports properly

53e1250

Compatibility fix for py2.6

08ac125

Merge branch 'master' into pxd-header

f628e89

Use pre-built wheel to install and run tests against

6de8ea4

Move wrapper code and typedefs into pxd

5cb185a

Merge branch 'pxd-header' into pxd-restructure

19ea21c

Move to src layout with subdirectory. Move test module to project root

b7d99f4

With quicktions nested under 'src/', setuptools can create a proper distribution/wheel. Moving test_fractions.py was needed to avoid import errors from namespace collisions.

Remove unused imports and assertions

f8894e9

Correct paths for "make test"

fad41af

Add comment explaining wheel installation step in travis config

dde7654

Make __version__ and __all__ variables available at the package level

efcca15

scoder requested changes Jan 25, 2019

View reviewed changes

nocarryr added 2 commits February 15, 2019 13:13

Place Cython-related tests into separate module

0536e1e

Use Cythonize module instead of pyximport to compile test modules

5a477cc

scoder reviewed Feb 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow cimport for quicktions #5

Allow cimport for quicktions #5

nocarryr commented Jan 25, 2019

scoder left a comment

scoder Jan 25, 2019

nocarryr Jan 25, 2019

scoder Jan 25, 2019

nocarryr Jan 25, 2019

scoder Jan 25, 2019

nocarryr Jan 25, 2019

scoder Jan 25, 2019

nocarryr Jan 25, 2019

scoder Jan 25, 2019 •

edited

Loading

nocarryr Jan 25, 2019

nocarryr commented Jan 25, 2019

scoder Feb 17, 2019

scoder Feb 17, 2019

cjdsellers commented Sep 23, 2020

scoder commented Sep 23, 2020

nocarryr commented Sep 23, 2020

cjdsellers commented Sep 24, 2020

nocarryr commented Sep 24, 2020

scoder commented Sep 25, 2020 via email

scoder commented Sep 25, 2020 via email

cjdsellers commented Sep 26, 2020

		@@ -70,16 +70,6 @@ cdef pow10(Py_ssize_t i):

		# Half-private GCD implementation.

		cdef extern from *:

Allow cimport for quicktions #5

Are you sure you want to change the base?

Allow cimport for quicktions #5

Conversation

nocarryr commented Jan 25, 2019

scoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scoder Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nocarryr commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjdsellers commented Sep 23, 2020

scoder commented Sep 23, 2020

nocarryr commented Sep 23, 2020

cjdsellers commented Sep 24, 2020

nocarryr commented Sep 24, 2020

scoder commented Sep 25, 2020 via email

scoder commented Sep 25, 2020 via email

cjdsellers commented Sep 26, 2020

scoder Jan 25, 2019 •

edited

Loading