Repository navigation
Split CPython and begin the Cython revolution - #1595
Conversation
static qualifiers from all CPython declarations and adds externs to PyType and PyMethodDef structs.
|
|
||
| CMDCLASS = versioneer.get_cmdclass() | ||
|
|
||
| from Cython.Build import cythonize |
There was a problem hiding this comment.
Recent setuptools (>18.0) can detect if Cython is installed, so no need to import Cython. This is the line that makes it work (in sourmash): https://github.com/dib-lab/sourmash/blob/9b9f7f467b9e99a4bba2e73869058da6021a0835/setup.py#L67
|
Note, if #1597 is merged before this is, I'll be happy to take the first stab at resolving the merge conflicts while the changes are still fresh in my head. |
|
Considering how much we have discussed making small movements and avoiding change for changes sake: Is this the minimal PR needed to start using cython for the python <-> c++ bridge? |
|
@betatim from casual offline conversations, it seems like the big problem was resolving the borked way that I/we wrote CPython using lot of static declarations (as recommended in the CPython documentation, sigh). So this is mostly a reorganization of khmer/ code. Since the tests are not changed at all, I am reasonably confident about the prospect of merging it, but that will have to wait until 2.1 comes out in any case. |
|
A more constructive comment: my idea of a gentle transition from the current CPython wrapper to one based on cython would be to pick one of the In this PR we have:
From the viewpoint of a reviewer it is essentially impossible to tell where code was moved, modified, deleted or added. A huge amount of work has gone into this so it deserves a better review than "travis seems happy so let's merge". |
|
On Fri, Jan 27, 2017 at 06:19:56AM -0800, Tim Head wrote:
A more constructive comment: my idea of a gentle transition from the current CPython wrapper to one based on cython would be to pick one of the `khmer` classes, remove the CPython wrapper and replace it by cython. Merge, repeat for the next class.
In this PR we have:
* reorganisation of the CPython code,
* addition of an assembler, and
* addition of a wrapper for `khmer` classes in the `oxli` namespace.
>From the viewpoint of a reviewer it is essentially impossible to tell where code was moved, modified, deleted or added. A huge amount of work has gone into this so it deserves a better review than "travis seems happy so let's merge".
;) sure. On the other hand, the CPython code is basically an ephemeral
wrapper around the C++ code, and (if we trust our tests to cover the
majority of the interesting behavior) then the question is not, "does this
work properly", but rather, "is there a better possible reorganization"?
From that perspective the idea of incrementally doing one class at a time
is not particularly relevant.
I do agree that adding the assembler seems like an unnecessary complication at
this point, but isn't prohibitive in my view since it's all new functionality
anyway - it will have to be reviewed and worked on more, I'm sure, but it's
not going to break anything we have currently.
|
|
Can't we delete some of the CPython wrapper? If we add a cython bridge to the c++ we now have two bridges to keep in sync. So why not get rid of the old one? In order not to have to instantly rewrite all the scripts we can add |
|
You won't have to twist my arm ;)
…On Fri, Jan 27, 2017 at 12:13 AM, Daniel Standage ***@***.***> wrote:
Note, if #1597 <#1597> is merged
before this is, I'll be happy to take the first stab at resolving the merge
conflicts while the changes are still fresh in my head.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1595 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwxrRdTHVsg_h6TatZO98l_oJR3QeNuks5rWadEgaJpZM4Lu8X4>
.
--
Camille Scott
Graduate Group for Computer Science
Lab for Data Intensive Biology
University of California, Davis
camille.scott.w@gmail.com
|
Codecov Report
@@ Coverage Diff @@
## master #1595 +/- ##
==========================================
- Coverage 64.06% 0.05% -64.02%
==========================================
Files 79 91 +12
Lines 12203 11500 -703
Branches 3085 3063 -22
==========================================
- Hits 7818 6 -7812
- Misses 2675 11494 +8819
+ Partials 1710 0 -1710
Continue to review full report at Codecov.
|
|
Odd, when I made previous comment it was directed at @standage, none of @betatim's comments had appeared in my inbox. So, I'll point out that the assembler is not new; it already has a CPython wrapper. Regardless, point taken. Unfortunately, the "minimal" PR to convert is necessarily quite large either way, as it requires splitting up the CPython stuff and adding all the definitions in |
|
Also: for now I haven't wrapped the entirety of Hashgraph; it's only available within the Cython wrapper. That would considerably increase the code size. The Assembler is in there as a proof of concept for taking incremental steps: it takes an old-style CPython Nodegraph object. |
|
So how do we go about merging this? Are there more things that need to be added or should we fix the merge conflicts, review it, travis it, merge it? What are your opinions on starting to get rid of the CPython wrapper code? My vote would be to write a cython wrapper in oxli, remove the CPython code, in |
|
Another thing that might become possible/easier after this PR is to try and find all the instances where a new string object is created but doesn't have to. Once we read a bit of sequence in, we should never have to copy it. Related question, can we keep things as bytes and save ourselves encoding/decoding steps? |
|
On Tue, Jan 31, 2017 at 05:14:09AM -0800, Tim Head wrote:
So how do we go about merging this? Are there more things that need to be added or should we fix the merge conflicts, review it, travis it, merge it?
What are your opinions on starting to get rid of the CPython wrapper code? My vote would be to write a cython wrapper in oxli, remove the CPython code, in `khmer` replace things with imports from oxli, and in the scripts switch to importing from `oxli` directly. Do this in a new PR (for N classes at a time).
Reminder: khmer 2.1 needs to come out before merging Cython foo.
Right now khmer 2.1 (and at least one PR) is partly block on a screed release.
The oxli path is good.
|
|
Issues re: the new release are another reason why I didn't remove parts of the CPython wrapper. I figured if it stays on its own it can be merged whenever, and just be there in the background. |
|
It does occur to me that the stuff in |
|
What problems are there with:
? |
|
Are those separate options?
…On Jan 31, 2017 11:40 AM, "C. Titus Brown" ***@***.***> wrote:
What problems are there with:
- completely removing the CPython layer in this PR;
- updating incrementally from master as we move towards 2.1;
- merging immediately after 2.1 release;
?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1595 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwxrVwN7Wa7oPpDKyzt2Bv0x2HQiys2ks5rX45FgaJpZM4Lu8X4>
.
|
|
No, 1, 2, then 3.
|
|
So @camillescott and I talked a bit about this today, and it seems like there are a few things going on. Hopefully she can correct me if I get this wrong -- The JunctionCountAssembler stuff is wrapped in Cython. That's fine, nothing relies on it yet, +1. There's a bunch of Cython declarations (but not wrapping) for the existing CPython interface. Then there is some "old" stuff (C++ and Python) that got reimplemented as Cython inside of oxli - (Above, all that "update incrementally" means is that as we add new stuff before releasing 2.1, we update this branch.) |
|
How do you feel about merging in #1590? That's gotta happen at some point... |
|
(I can update it from master if you like.) |
|
That'll require more thought -- the cython branch moved most of the
validation stuff into the parser itself.
…On Tue, May 23, 2017 at 2:21 PM, C. Titus Brown ***@***.***> wrote:
(I can update it from master if you like.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1595 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwxrUXYXd31pvTpdk2qHzv51mlQln7Gks5r803EgaJpZM4Lu8X4>
.
--
Camille Scott
Graduate Group for Computer Science
Lab for Data Intensive Biology
University of California, Davis
camille.scott.w@gmail.com
|
|
I am fine with only the tests being moved over, as long as you can get them
to pass.
|
|
I mean, sure, go ahead. I'm -1 on making merger of this contingent on merger of the many other open PRs that are up currently though... seems like a roundabout way of doing things. |
|
Whichever way you want to go is fine by me.
…--
Titus Brown, ctbrown@ucdavis.edu
--
Titus Brown, ctbrown@ucdavis.edu
On May 23, 2017, at 4:06 PM, Camille Scott ***@***.***> wrote:
I mean, sure, go ahead. I'm -1 on making merger of this contingent on merger of the many other open PRs that are up currently though... seems like a roundabout way of doing things.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
In that case, let's just get it merged.
…On May 23, 2017 5:03 PM, "C. Titus Brown" ***@***.***> wrote:
Whichever way you want to go is fine by me.
--
Titus Brown, ***@***.***
--
Titus Brown, ***@***.***
> On May 23, 2017, at 4:06 PM, Camille Scott ***@***.***>
wrote:
>
> I mean, sure, go ahead. I'm -1 on making merger of this contingent on
merger of the many other open PRs that are up currently though... seems
like a roundabout way of doing things.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1595 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwxrfp5ZPxtslyvVpkxdkVbhTkGGwhxks5r82eLgaJpZM4Lu8X4>
.
|
|
Taking a look at the failing cpp-demos |
|
I had to apply the following to get things to compile on my mac: @@ -67,6 +67,7 @@ CMDCLASS = versioneer.get_cmdclass()
HAS_CYTHON = False
try:
import Cython
+ from Cython.Build import cythonize
HAS_CYTHON = True
except ImportError:
pass
@@ -194,6 +195,7 @@ CP_EXTENSION_MOD_DICT = \
EXTENSION_MODS = [Extension("khmer._khmer", ** CP_EXTENSION_MOD_DICT)]
+mods = []
for cython_ext in glob.glob(os.path.join("khmer", "_oxli",
"*.{0}".format(cy_ext))):
@@ -212,7 +214,10 @@ for cython_ext in glob.glob(os.path.join("khmer", "_oxli",
ext_name = "khmer._oxli.{0}".format(
splitext(os.path.basename(cython_ext))[0])
- EXTENSION_MODS.append(Extension(ext_name, ** CY_EXTENSION_MOD_DICT))
+ mods.append(Extension(ext_name, ** CY_EXTENSION_MOD_DICT))
+
+EXTENSION_MODS.extend(cythonize(mods))
SCRIPTS = []
SCRIPTS.extend([path_join("scripts", script)but travis seems happy without?? |
|
The above modification to |
|
You must have an outdated setuptools -- new versions support cython out of the box. Seems we should update the requirements file to reflect that. |
|
Unleash the Cython! |
|
@luizirber (for one) welcomes our new Cython overlords! |
make testDid it pass the tests?make clean diff-coverIf it introduces new functionality inscripts/is it tested?make format diff_pylint_report cppcheck doc pydocstyleIs it wellformatted?
additions are allowed without a major version increment. Changing file
formats also requires a major version number increment.
documented in
CHANGELOG.md? See keepachangelogfor more details.
changes were made?
tested for streaming IO?)