BLD: Use tempita for cython templating #13716

sinhrks · 2016-07-20T11:59:15Z

closes BLD: should we use tempita for cython templating #13399
tests added / passed
passes git diff upstream/master | flake8 --diff

not sure what the setup.py 's best practice be...

jreback · 2016-07-20T12:12:26Z

pandas/src/algos_join_helper.pxi.in

+"""
+Template for each `dtype` helper function for join
+
+WARNING: DO NOT edit .pxi FILE directly, .pxi is generated from .pxi.in


this warning should only be on the .pxi right?

actually I see, maybe make sure this warning is in all of the .pxi.in files.....

jreback · 2016-07-20T12:16:10Z

does this change build time?

you might need to change the manifest (to not include the .pxi files)

codecov-io · 2016-07-20T12:30:10Z

Current coverage is 84.56% (diff: 100%)

Merging #13716 into master will not change coverage

@@             master     #13716   diff @@
==========================================
  Files           141        141          
  Lines         51195      51195          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          43295      43295          
  Misses         7900       7900          
  Partials          0          0

Powered by Codecov. Last update 9f94e6a...09f63b6

sinhrks · 2016-07-20T12:49:02Z

For build time, tempita looks little slower. It may caused by include files are split to 4... (both after setup.py clean):

on current master:

Finished processing dependencies for pandas==0.18.1+220.g4c9ae94

real    5m23.612s
user    4m56.507s
sys 0m14.372s

tempita:

Finished processing dependencies for pandas==0.18.1+221.g9c1d26a

real    5m52.029s
user    5m12.612s
sys 0m13.344s

jreback · 2016-07-20T17:30:21Z

this looks fine to me. @jorisvandenbossche @wesm ?

jorisvandenbossche · 2016-07-20T18:17:43Z

Nice! (@sinhrks you're quite on speed lately! :-))

No further comment from me

gfyoung · 2016-07-20T18:21:27Z

LGTM - nice, @sinhrks !

sinhrks · 2016-07-20T18:33:38Z

you might need to change the manifest (to not include the .pxi files)

added *.pxi on gitignore and manifest, as currently we don't use .pxi elsewhere.

shoyer · 2016-07-20T18:42:19Z

pandas/src/algos_common_helper.pxi.in

+
+    for name, c_type, dtype, can_hold_na, nogil in dtypes:
+
+        nogil = 'with nogil:' if nogil else ''


use a different variable name here (e.g., nogil_str)

done. also, removed unnecessary gil kw not used in templates.

jreback · 2016-07-20T21:09:58Z

can you make sure this is lintable (and add on as well), see here

not a big deal, but if you manually lint pyx files these will be as well

jreback · 2016-07-21T10:41:55Z

@sinhrks ping when ready

sinhrks · 2016-07-21T11:38:33Z

ci/lint.sh

@@ -23,14 +23,26 @@ if [ "$LINT" ]; then
    for path in 'window.pyx'
    do
        echo "linting -> pandas/$path"
-        flake8 pandas/$path --filename '*.pyx' --select=E501,E302,E203,E226,E111,E114,E221,E303,E128,E231,E126,E128
+        flake8 pandas/$path --filename '*.pyx' --select=E501,E302,E203,E226,E111,E114,E221,E303,E128,E231,E126


E128 is duplicated, thus removed.

jorisvandenbossche · 2016-07-21T13:33:40Z

.gitignore

@@ -80,6 +80,7 @@ scikits
 !np_datetime_strings.c
 *.c
 *.cpp
+*.pxi


It's not that important for now, but in general those files should not necessarily be ignored, as we could also have non-generated .pxi files. Eg scipy includes the actual names of the generated file in the gitignore (because the generated files can also be eg .pyx files).
So maybe as good practice already include the actual names?

no, by-definition .pxi are auto-generated, we never want to add them to the repo, while .pxi.in we always do.

AFAIK .pxi are just 'include' files, so they are not auto-generated by definition.

http://docs.cython.org/src/reference/language_basics.html#cython-file-types

but WE are using them as auto-generated files (from .pxi.in)

further we DON't want to include an auto-generated file in git. So yes maybe should specifically exclude them in .gitignore (or rather the auto-gen ones)

https://github.com/cython/cython/wiki/FAQ#what-is-the-difference-between-a-pxd-and-pxi-file-when-should-either-be-used

I think we want to use .pxi for the auto-generated ones. while .pxd is much more like a c include file (e.g. its done by hand). .pyx is also 'hand-generated'

Reconsidered this, and now I think we should include .pxi under git actually. Otherwise, Travis cache can't work because .pxi is always newly generated?

ahh ok - that will still reflect changes so that sounds fine

Will change:

remove pxi from excluding rules

change setup.py to overwrite pxiif required comparing its timestamp.

slightly related question: is there a reason you use pxi/pxi.in files in this case, while we use pyx extension in other cases for included files? (would be nice to do this consistently within pandas)

sinhrks · 2016-07-22T18:06:24Z

@jorisvandenbossche

is there a reason you use pxi/pxi.in files in this case, while we use pyx extension in other cases for included files?

I don't have a strong opinion about it, the name "pxi.in" is derived from @gfyoung 's PR. whichever looks consistent:

use ".pyx" in all Cython files
use ".pxi" in template-generated Cython files

gfyoung · 2016-07-22T18:25:40Z

This link here should help clarify the file extensions.

jreback · 2016-07-25T12:07:06Z

@gfyoung that link doesn't seem to work, can you update.

jorisvandenbossche · 2016-07-25T13:16:43Z

Corrected. So from that link, pxi files are not specific for 'template-generated files', but for 'include' files.

But given that we currenlty use .pyx files for included files as well (eg join.pyx and generated.pyx included in algos.pyx https://github.com/pydata/pandas/blob/master/pandas/algos.pyx#L1337), I would either keep using pyx for the template genereated include files, or rename the other included files to pxi to have it consistent within the pandas source code.
(but to be clear, not that important, this PR is a really nice improvement in the code generation)

jreback · 2016-07-25T21:10:24Z

I actually like that .pxi is template generated, and .pyx is not. sort of a visual diff.

jreback · 2016-07-26T10:52:16Z

thanks @sinhrks

can always think about the .pxi vs .pyx question, but I think ok for now.

sinhrks added Build Library building on various platforms Clean labels Jul 20, 2016

sinhrks added this to the 0.19.0 milestone Jul 20, 2016

jreback reviewed Jul 20, 2016
View reviewed changes

sinhrks force-pushed the tempita branch 2 times, most recently from 77c4d7e to d955b70 Compare July 20, 2016 18:30

shoyer reviewed Jul 20, 2016
View reviewed changes

sinhrks force-pushed the tempita branch from d955b70 to 7eba9af Compare July 21, 2016 11:37

sinhrks reviewed Jul 21, 2016
View reviewed changes

sinhrks force-pushed the tempita branch 3 times, most recently from 6cc47c9 to fa307af Compare July 21, 2016 13:17

jorisvandenbossche reviewed Jul 21, 2016
View reviewed changes

sinhrks mentioned this pull request Jul 22, 2016

PERF: Improve duplicated perf #13751

Closed

4 tasks

BLD: Use tempita for cython templating

09f63b6

sinhrks force-pushed the tempita branch from fa307af to 09f63b6 Compare July 22, 2016 17:53

jreback mentioned this pull request Jul 25, 2016

CLN: template hashtable.pyx #13786

Closed

jreback closed this in 98c5b88 Jul 26, 2016

sinhrks deleted the tempita branch July 26, 2016 11:14


		for name, c_type, dtype, can_hold_na, nogil in dtypes:

		nogil = 'with nogil:' if nogil else ''

Uh oh!

BLD: Use tempita for cython templating #13716

BLD: Use tempita for cython templating #13716

Uh oh!

Conversation

sinhrks commented Jul 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 20, 2016

Uh oh!

codecov-io commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 84.56% (diff: 100%)

Uh oh!

sinhrks commented Jul 20, 2016

Uh oh!

jreback commented Jul 20, 2016

Uh oh!

jorisvandenbossche commented Jul 20, 2016

Uh oh!

gfyoung commented Jul 20, 2016

Uh oh!

sinhrks commented Jul 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback commented Jul 20, 2016

Uh oh!

jreback commented Jul 21, 2016

Uh oh!

sinhrks Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinhrks commented Jul 22, 2016

Uh oh!

gfyoung commented Jul 22, 2016 • edited by jorisvandenbossche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Jul 25, 2016

Uh oh!

jorisvandenbossche commented Jul 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Jul 25, 2016

Uh oh!

jreback commented Jul 26, 2016

Uh oh!

Uh oh!

codecov-io commented Jul 20, 2016 •

edited

Loading

sinhrks Jul 21, 2016 •

edited

Loading

jorisvandenbossche Jul 21, 2016 •

edited

Loading

jorisvandenbossche Jul 21, 2016 •

edited

Loading

jreback Jul 21, 2016 •

edited

Loading

gfyoung commented Jul 22, 2016 •

edited by jorisvandenbossche

Loading

jorisvandenbossche commented Jul 25, 2016 •

edited

Loading