Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow datrie with recent Python / Drop datrie? #442
Comments
Please see PR #446 to require datrie 0.8+ to support Python 3.7. But datrie doesn't yet support Python 3.8: pytries/datrie#73. |
Ergh, yeah, okay, should just take a look at the perf impact of just dropping support. |
In a bunch of more ordinary cases the perf impact seems to be approximately nothing. I suggest we just drop support for datrie? I doubt even in the worst case it's going to be significant? |
If keeping it only offers minimal or zero performance benefit, sounds like a good idea to drop, it'll reduce the maintenance burden, as it seems this will happen for every new (now annual) Python release. And we're 4 months since 3.8 came out, and Datrie support is still pending. |
datries 0.8.2 should support py37 and py38. |
On the two tests I have that are pretty heavy on entities, we get: Mean +- std dev: [datrie_without] 293 ms +- 6 ms -> [datrie_with] 291 ms +- 6 ms: 1.01x faster (-1%) On the whole, despite both tests showing statistically significant perf gains, I think they're small enough they're not worth it? It's probably worthwhile noting the pure-Python entity lookup with was much slower when datrie was added as an optional dependency, and as such the perf gain from it was much larger. |
#455 dropped datrie. |
We limit the datrie optional dependency to Python < 3.7, because prior to the 0.8 release it didn't support Python 3.7. Probably, we should either: