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

Move from SWIG to Cython and support Py2 and Py3 #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Move from SWIG to Cython and support Py2 and Py3 #8

wants to merge 1 commit into from

Conversation

laserson
Copy link

@laserson laserson commented Jun 2, 2015

The interface is in the .pyx file. There may be some subtle unicode issues, as the functions always return bytestrings. This is fine in Python 2, but might require people to manually decode into UTF8 unicode strings if using Python 3. I did a bit of cleanup as well.

@laserson
Copy link
Author

laserson commented Jun 2, 2015

I also eliminated the C++ Client class, as it is redundant. There is now just the ClientImpl class, for which I exposed the relevant interface, and Cython will generate the necessary Client wrapper class.

@laserson
Copy link
Author

laserson commented Jun 5, 2015

@toddlipcon poke. This is good to go for me. Successfully worked against HiveServer2 from Python 2 and 3 in impyla.

@laserson
Copy link
Author

laserson commented Jul 9, 2015

@toddlipcon poke. This is also for a customer.

@toddlipcon
Copy link
Owner

TBH, I'm a little worried that this is a pretty large rewrite to switch from swig to cython. I don't know much about Cython so I don't feel particularly qualified to review it, and the potential for leaks, etc, is somewhat high in this sort of code.

What kind of stress/integration testing did you do? I'm guessing that projects that depend on this aren't pinned to a particular version (are they?), so I'm a little afraid to commit a large change without being extra conservative (especially when it's for a niche feature). Have you run this change by the Hue team, for example? Any checks you can do that verify that there aren't memory leaks?

@laserson
Copy link
Author

laserson commented Jul 9, 2015

Not very much testing. Wanted to hear what you thought would be convincing first.

cc @wesm, when you have time, could you check out this change?

I'll send a message to the Hue team. Do you anticipate that other parts of CDH would be touched by this?

@toddlipcon
Copy link
Owner

Looks like just impala-shell and Hue based on my grepping of CDH.git

@toddlipcon
Copy link
Owner

Actually looks like Hue uses its own 'ext-py/' directory with a local checkout rather than pulling from PyPI, but would probably be good to get them to confirm.

In general, would be nice to transition ownership of this module over to someone on one of the teams that uses it and can take better care of maintaining it :)

@laserson
Copy link
Author

laserson commented Jul 9, 2015

agreed

@romainr
Copy link

romainr commented Jul 9, 2015

Yes, Hue ships with a checked-in version of the lib, so is independent of the push of this PR until a potential rebase.

Main worry is indeed that this would need a careful regression testing :)

@wesm
Copy link

wesm commented Nov 12, 2015

I can review this PR if we can prioritize merging? I believe it is impacting users

@toddlipcon
Copy link
Owner

Sure, I basically don't have time to act as a responsible maintainer. If you want to take over being the maintainer, we can move the repository to your github.

@wesm
Copy link

wesm commented Nov 12, 2015

Roger. Let's move the repo to the cloudera org and I can take over there. The only comment I have on the diff is that we should not be checking in compiled Cython code to git, but we can include compiled sources in the tarball similar to pandas and other projects with substantial Cython components (where user systems may not have Cython installed)

@toddlipcon
Copy link
Owner

Sounds good. Let me know if you need assistance with migrating to that repo.

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.

None yet

4 participants