search

pytries / datrie

Fast, efficiently stored Trie for Python. Uses libdatrie.
http://pypi.python.org/pypi/datrie/
GNU Lesser General Public License v2.1
530 stars 88 forks source link

Python3.8 + cythonize #78

Closed ashwinvis closed 4 years ago

ashwinvis commented 4 years ago

Merges #76 and #69

johanneskoester commented 4 years ago

@kmike @superbobry any chance that this will be merged and released? We urgently need this fix for Snakemake. We are happy to help in any way if there are further showstoppers.

sarnold commented 4 years ago

Apparently I'm (half)blind and didn't notice this PR until after I updated the bindings for 3.8+ (and dropped python2 support) in my fork. My changes are simpler(?) but instead of messing with an old submodule commit I made it depend on libdatrie instead (currently at 0.2.12). My plan was to make a new tag (in my fork) and push new packages to a PPA and Gentoo overlay. Does that help at all?

ashwinvis commented 4 years ago

@sarnold Sure go ahead. I am on Arch and I did something similar which has libdatrie 0.2.12-1. I simply added a symlink, instead of relying on the submodule.

❯ ls -l datrie/libdatrie
total 4
lrwxrwxrwx 1 avmo avmo 19 14.01.2020 13:28 datrie -> /usr/include/datrie/

However, I wonder how one can support Windows by this approach? Maybe you have a better idea.

sarnold commented 4 years ago

I tested your branch yesterday, and also made a (hopefully) one-off github release for python3 only:

https://github.com/freepn/datrie/releases/tag/0.8.1

Still I didn't manage to fix the build warnings; I had the pointer type warning fixed yesterday but didn't get it in yet, and I'm not quite sure what the right/pythonic answer is for the collections import (since py27 is special there). Got any ideas for the handling the import issue?

Also I have PPA pkgs for xenial, helpful for travis builds, er, here even:

https://launchpad.net/~nerdboy/+archive/ubuntu/embedded

johanneskoester commented 4 years ago

@sarnold @ashwinvis what do you think about a pypi release. Something like datrie-<areasonablesuffix>? This would make it very easy to get it into bioconda or conda-forge, where I need it as a snakemake dependency.

ashwinvis commented 4 years ago

Apart from Python + Cython + packaging knowledge, I have neither the expertise on tries nor the long term motivation to be a package maintainer for datrie. I do like to see this happen, but it would be mistake if I take up this responsibility now, only to abdicate it in the future.

ashwinvis commented 4 years ago

@sarnold I compared your branch, and as suspected you have many instance of /usr/include/libdatrie in the cythonized sources of your branch instead of ../libdatrie. This almost certainly will break in non-Linux OSs.

johanneskoester commented 4 years ago

Maybe I should move it under the hood of snakemake.

johanneskoester commented 4 years ago

@ashwinvis is it ok if I fork your repo, merge in your fix and release this as datrie-snakemake on pypi?

johanneskoester commented 4 years ago

Or, probably cleaner, I fork this original repo, and you add your PR against my fork?

ashwinvis commented 4 years ago

Sounds good @johanneskoester!

tacaswell commented 4 years ago

If you are going to require cython as a build time dependency you can drop the bundled c-files all together which will prevent a similar "the c-files are broken" issues in the future.

tacaswell commented 4 years ago

@ashwinvis is it ok if I fork your repo, merge in your fix and release this as datrie-snakemake on pypi?

Between that and the 0.8.1 tag this package is getting pretty forked. Has anyone tried reaching out to @kmike or @superbobry not through github?

johanneskoester commented 4 years ago

I wasn't able to reach them via mail. From one, I did not find an email, from the other, the published mail address is not existent any more.

tacaswell commented 4 years ago

ok.

Very short term, I think just the "do the cythonization" patch to setup.py will un-block conda-forge so we can get that moving again.

johanneskoester commented 4 years ago

@ashwinvis I am done with the setup of the fork and github actions for testing. Awaiting your PR. Thanks a lot!

tacaswell commented 4 years ago

https://github.com/conda-forge/datrie-feedstock/pull/5 <- CF pr

tacaswell commented 4 years ago

@ashwinvis @johanneskoester @sarnold I now have commit rights on this repo and should have pypi permissions soon. Should be able to get this done and release in the next ~24 hours. Going to make another issue to make clear what my plan is and track the work.

johanneskoester commented 4 years ago

@tacaswell datrie-snakemake is now available on pypi, with the fixes of @ashwinvis. I can create a new conda-forge recipe for it, or we keep the old and refer to datrie-snakemake in it. What do you think?

johanneskoester commented 4 years ago

Wait, I have just seen your release plan. Are you one of the maintainers? In any case, I am happy to remove my fork again.

tacaswell commented 4 years ago

@johanneskoester Yes, @kmike gave me commit rights this morning and I just confirmed I have access to pypi as well.

As for conda, snakemake now at least installs on py38 (see log in https://discourse.covid-oss-help.org/t/helping-nextstrain/284/20) from the bioconda + conda-forge channels.

tacaswell commented 4 years ago

Commits merged in #80. Thank you @ashwinvis you did most of the hard work here!

johanneskoester commented 4 years ago

Awesome @tacaswell! I will remove my fork then!

johanneskoester commented 4 years ago

Done.

sarnold commented 4 years ago

Sorry, got distracted with stuff... Feel free to use/fork whatever is useful; I just made a (local) tag so I could have something to package/deploy for now.