[BLD] enable cython coverage, use cythonize #21879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbrockmendel
commented
Jul 12, 2018
| @@ -534,8 +582,7 @@ def pxd(name): | |||
| 'depends': _pxi_dep['sparse']}, | |||
| '_libs.tslib': { | |||
There was a problem hiding this comment.
For demonstration purposes: note that util has been removed entirely from the pxdfiles entries for tslib/tslibs (as opposed to changed to _libs/util). If you go and edit tslibs/util.pxd and re-run build_ext, you'll messages to the effect of:
Compiling pandas/_libs/tslibs/resolution.pyx because it depends on ./pandas/_libs/tslibs/util.pxd.
Compiling pandas/_libs/algos.pyx because it depends on ./pandas/_libs/tslibs/util.pxd.
Compiling pandas/_libs/tslibs/nattype.pyx because it depends on ./pandas/_libs/tslibs/util.pxd.
[...]
|
Full results, took 108 minutes (yikes!) |
|
It will be easier to tell if/when the cdef-lines issue is resolved, but tentatively it looks like a bunch of the not-hit code is generate code for uncommon dtypes. |
Codecov Report
@@ Coverage Diff @@
## master #21879 +/- ##
=======================================
Coverage 91.96% 91.96%
=======================================
Files 166 166
Lines 50334 50334
=======================================
Hits 46292 46292
Misses 4042 4042
Continue to review full report at Codecov.
|
|
Without cython coverage enabled, the test suite took took 29 minutes. |
gfyoung
added
Testing
Hmm...that's awkward...can you try adding an allowed failure build to Travis to see how long it takes? |
Keep in mind this is without Especially if this is slow, this probably isn't something to run in the CI, but something to run daily like the asvs. |
Actually, let's cross that bridge once we have a better idea of the timing. Could you run the tests locally with |
|
With |
|
Hmmm...I might consider this in the @jreback : Thoughts? |
|
can you change so that coverage is turned off by default for cython. Then in another PR can make it depend on a env variable (how we do everything else), and then only enable in the pandas-ci build. |
|
alternatatively could string out the coverage entirely to another PR and just do the cythonize changes here |
It is off by default as it is. |
|
The remaining thing to do here is to go through the ext_data dict and remove pieces that are now unnecessary. We can do that in one swoop or piece-by-piece in follow-ups. |
|
I've gone through and removed the pxdfiles entries. I think that many of the Related: it tentatively looks like a lot of duplicate build errors, compile time, and possible even .so size are caused by using .pxd files without accompanying .pyx files (util.pxd and khash.pxd mainly). Those don't get their own .so files, so get re-compiled in every file they are used in. (I think something similar happens for lib_depends and tseries_depends) |
jreback
requested changes
Jul 16, 2018
setup.py
Outdated
|
|
||
| # enable coverage by building cython files by setting the environment variable | ||
| # "linetrace" (with a Truthy value) | ||
| linetrace = os.environ.get('linetrace', False) |
There was a problem hiding this comment.
yeah this need to be uppercase and should be like PANDAS_CYTHON_COVERAGE or somesuch
|
don't we have an issue for this? can you change the top to close it |
|
Done. |
|
It looks like |
|
can you rebase again. just want to see this hit the CI after the lastest merges. |
jreback
requested changes
Jul 20, 2018
setup.py
Outdated
| # "PANDAS_CYTHON_COVERAGE" (with a Truthy value) | ||
| linetrace = os.environ.get('PANDAS_CYTHON_COVERAGE', False) | ||
| CYTHON_TRACE = str(int(bool(linetrace))) | ||
| # TODO: Maybe make this a CLI option for build_ext? |
There was a problem hiding this comment.
can you create an issue for this
jreback
approved these changes
Jul 20, 2018
|
@jbrockmendel lgtm. merge on green. |
|
i think this broke the wheel building: pls have a look |
|
No idea why this would cause numpy deprecation warnings. |
Sup3rGeo
pushed a commit
to Sup3rGeo/pandas
that referenced
this pull request
Oct 1, 2018
* enable cython coverage, use cythonize * coverage misreporting workaround * lint cleanups * post-merge cleanup * remove unnecessary pxdfiles * fix signature of dummy cythonize * change env variable * remove comment, making an issue for it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Doesn't close #12624, but merits a mention.
Usage:
(I'll post results from a full test run in a bit)
Apparently there is an issue with the Cython.Coverage plugin that causes cdef function/class definition lines to not get covered. Not sure if that's going to be fixed or if we need to find a workaround.
To make
cythonizework I had to moveutil.pxdtotslibs, then cimport everything into a _libs.util pxd namespace. There may be a way around this that I haven't found. If not, there are parts oftslibs.utilthat are not used intslibs, can be moved to_libs.util.We could remove a whole bunch more from the
ext_datadictionary, but I'm saving that until after the first pass of the review process. (I think doing so will lighten the build, not sure)This will have a merge conflict with #21878, but it'll be easy to resolve when the time comes.