Make khash its own extension #18472
Make khash its own extension #18472
Conversation
|
this is a header only, I don't think this is necessary. I was referring to skiplist becoming its own module, khash is fine as is. |
As discussed, it is necessary for cythonize. In attempts to date, cythonize chokes on khash, skiplist, datetime, and util. |
|
BTW skiplist push is coming before long, but I'm testing locally before pushing. |
|
well, then you should edit everything to remove the direct imports from khash.h and put the declarations in .pxd. |
OK. I'm not clear on what you're asking for. The only things that have "direct imports from khash.h" (based on a quick grep) are "src/klib/khash_python.h" and "src/parser/tokenizer.h". The only place that has directly imports from khash_python.h is khash.pxd. |
Codecov Report
@@ Coverage Diff @@
## master #18472 +/- ##
==========================================
- Coverage 91.36% 91.32% -0.04%
==========================================
Files 163 163
Lines 49700 49717 +17
==========================================
- Hits 45407 45404 -3
- Misses 4293 4313 +20
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18472 +/- ##
==========================================
- Coverage 91.36% 91.33% -0.03%
==========================================
Files 163 164 +1
Lines 49700 49819 +119
==========================================
+ Hits 45407 45503 +96
- Misses 4293 4316 +23
Continue to review full report at Codecov.
|
setup.py
Outdated
| @@ -335,6 +335,7 @@ class CheckSDist(sdist_class): | |||
| 'pandas/_libs/index.pyx', | |||
| 'pandas/_libs/algos.pyx', | |||
| 'pandas/_libs/join.pyx', | |||
| 'pandas/_libs/khash.pyx', | |||
| 'pandas/_libs/indexing.pyx', | |||
| 'pandas/_libs/interval.pyx', | |||
There was a problem hiding this comment.
hashtable is still coupled to khash_python.h
There was a problem hiding this comment.
I guess I'm still unclear on the transitive-dependencies policy. A bunch of other things are coupled to hashtable, so are they also coupled to khash_python.h?
There was a problem hiding this comment.
well this is exactly the issue
you now have an actual module rather than header only so you shouldn’t import from the c files and instead put the defa inside the .pxd
you may want to find an example how to do this
i don’t like the idea of a dummy module
There was a problem hiding this comment.
Would getting rid of the .pyx file solve this?
|
One down, thee to go. Let’s knock this out this week. |
|
i don't like the dummy file here. |
|
does this help with cythonize? |
Good question. With the pyx file it was a "yes". I'll have to double-check without. |
|
Yes, this helps with cythonize. i.e. the setup that I got working with cythonize also works without the dummy .pyx file. FYI, the steps to get cythonize working (not necessarily the only way, just the way that I found):
|
| @@ -10,7 +10,7 @@ np.import_array() | |||
|
|
|||
| from util cimport is_string_object, get_nat | |||
|
|
|||
| from khash cimport ( | |||
| from pandas._libs.khash cimport ( | |||
There was a problem hiding this comment.
I think you can do relative imports here?
from ..khash cimport ? not sure. but this is ok for now.
The path towards cythonize continues.