Skip to content

Pypy fixes #15080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Pypy fixes #15080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf458cf
COMPAT: PyPy can only call PyList_GET_SIZE on a PyListObject
mattip Jan 7, 2017
df4cffd
BUG: bring implementation into compliance with cython documentation
mattip Jan 7, 2017
91522c6
BUG: fix improper use of conditional compilation
mattip Jan 8, 2017
b38d7be
BUG: fix improper use of conditional compilation, add env var
mattip Jan 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pandas/lib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ from cpython cimport (PyDict_New, PyDict_GetItem, PyDict_SetItem,
PyBytes_GET_SIZE,
PyUnicode_GET_SIZE)

try:
IF PY_MAJOR_VERSION == 2:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this more typical? (and already defined)

#if PY_VERSION_HEX >= 0x03000000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is more typical for C code, but converting a pyx file to C happens during a cython compiliation, and the only variables available are the ones appearing in cython_compile_time_env AFAICT, which is why I needed to define the variable in setup.py.

It is not surprising that this is not well known, the only reference I could find on it was a stack overflow question. I tried all different ways to rewrite this code to keep the PyString_GET_SIZE on python2 and make it PyUnicode_GET_SIZE on python3, this was the least intrusive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly can use these constants?
https://github.com/cython/cython/blob/master/Cython/Includes/cpython/version.pxd
from
http://stackoverflow.com/a/28316509/3657742

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-b1 that did the trick, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-b1 I could not get that to work since this is needed at compile time, so I needed the upper-case IF <env_var>, not a runtime if <value> :( I reverted to the original fix

Copy link
Contributor

@jreback jreback Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still seems very odd, the why can't you do

#if PY_VERSION_HEX >= 0x03000000
    from cpython cimport PyUnicode_GET_SIZE as PyString_GET_SIZE
#else
    from cpython cimport PyString_GET_SIZE
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that. It does not work for me. Running python3 setup.py cython results in a lib.c with PyString_GET_SIZE in the translated max_len_string_array, where if it worked that would become PyUnicode_GET_SIZE

from cpython cimport PyString_GET_SIZE
except ImportError:
ELSE:
from cpython cimport PyUnicode_GET_SIZE as PyString_GET_SIZE

cdef extern from "Python.h":
Expand Down
6 changes: 5 additions & 1 deletion pandas/src/hashtable_class_helper.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,11 @@ cdef class PyObjectHashTable(HashTable):

def __dealloc__(self):
if self.table is not NULL:
self.destroy()
# cython documentation -
# "don’t call any other methods of the object"
# self.destroy()
kh_destroy_pymap(self.table)
self.table = NULL

def __len__(self):
return self.table.size
Expand Down
4 changes: 4 additions & 0 deletions pandas/src/ujson/python/JSONtoObj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,11 @@ int Object_npyObjectAddKey(void *prv, JSOBJ obj, JSOBJ name, JSOBJ value) {

// only fill label array once, assumes all column labels are the same
// for 2-dimensional arrays.
#ifdef PYPY_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are defining this on external compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is defined when compiling c-extensions with the PyPy headers

if (PyObject_GET_SIZE(npyarr->labels[labelidx]) <= npyarr->elcount) {
#else
if (PyList_GET_SIZE(npyarr->labels[labelidx]) <= npyarr->elcount) {
#endif
PyList_Append(npyarr->labels[labelidx], label);
}

Expand Down
6 changes: 6 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ def build_extensions(self):
with open(outfile, "w") as f:
f.write(pyxcontent)

# used in lib.pyx, assumes old_build_ext
if not self.cython_compile_time_env:
self.cython_compile_time_env = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

major = sys.version_info.major
self.cython_compile_time_env['PY_MAJOR_VERSION'] = major

numpy_incl = pkg_resources.resource_filename('numpy', 'core/include')

for ext in self.extensions:
Expand Down