Skip to content
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.

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

Moving pandasjson back into mainline pandas #3583

Closed
wants to merge 3 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented May 12, 2013

This extension works perfectly fine on Python 2.x and all platforms to my knowledge except win32 on MinGW (it's fine with VC2008+ AFAIK). It might have problems on some of @yarikoptic's esoteric Debian platforms, we should identify which ones those are and disable the extension in the setup.py until we have a chance to sort out what's wrong in the C code. I don't think that's a bad compromise for now in the interest of having a usable extension for a lot of folks.

@wesm
Copy link
Member Author

wesm commented May 12, 2013

also cc @Komnomnomnom

@Komnomnomnom
Copy link
Contributor

Ok, so iirc the problems on the esoteric platforms were solved, at least on SPARC 32+64, if memory serves there was some memory alignment and buffer overflow problems and the fixes were pushed upstream to ujson.

As for windows, that was indeed a bit of a nightmare, the problems that occurred were intermittent segmentation faults which were hard to reproduce and pin down, and I neither had the windows tools or experience to debug it thoroughly. Here's what I concluded at the time (EPD = Enthought Python Distribution):

With an independent version of mingw -> no segmentation faults. Compiled a python debug version on win32 and compiled numpy and pandas using EPD's mingw -> no segmentation faults. From the little bit of a stack trace I've been able to get it appears that the problem occurs when ujson calls numpy C-API functions (take that with a healthy dose of salt however as the stack trace also looked pretty corrupted.)

I think the problem occurred when mixing VS compiled ujson and mingw compiled numpy, but it will take more investigation to know for sure. Ideally it would be great if someone with a proper windows setup could take a look, but maybe having it in pandas but disabled might motivate some brave soul...

On another note I think there have been some minor fixes and enhancements to ujson since it was last merged with pandasjson. Once / if pandasjson is re-included in mainline pandas I'll see about merging them in and send a pull request through?

BTW the extension should work fine in Python 3.x too, or are there some issues I'm not aware of?

@wesm
Copy link
Member Author

wesm commented May 12, 2013

Yes I think it should be working with python 3, I need to run the CI tests (I didn't have travis enabled when I opened the PR). If you could merge in the upstream changes sometime that would be great

@hayd
Copy link
Contributor

hayd commented May 17, 2013

@wesm travis is broken :(

@jreback
Copy link
Contributor

jreback commented Jun 7, 2013

@wesm @Komnomnomnom @hayd want to give #3804 a test run?

@jreback
Copy link
Contributor

jreback commented Jun 11, 2013

closed via #3804

@jreback jreback closed this Jun 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants