Dev.ej/compact lexicon

[joanise]

PR Goal?

A bit of a crazy idea to reduce the amount of memory used by the g2p library: the English lexicon takes a lot of space in memory because each word is stored in a string, and each string object in Python is quite large, well beyond the size of the actual character sequence.

So, I thought, let's see what happens if I compact them by joining them by blocks. And a) it save 15MB in RAM from loading langs, b) with no measurable speed cost. The real cost is the increased complexity of the find_alignment() function in g2p.mappings.utils, but it goes from six lines to 15 lines, so, really, not that bad.

I've tested carefully to make sure the results are identical, and speed is not adversely affected.

The langs.json.gz file is a tad larger, but the memory footprint from loading it is significantly smaller.

Fixes?

Alternative solution to the problem raised in #395

Feedback sought?

Careful review.

And, whether we can reasonably expect a memory mapping solution soon, which would make this patch irrelevant.

Priority?

low

Tests added?

I better do that... coverage was not as good as I expected, and I will fix that to make sure I've tested all the corner cases.

Yes

How to test?

• g2p convert "some test" eng eng-ipa still works as expected
• unit tests & CI pass

Confidence?

high

Version change?

nope

[github-actions]

CLI load time: 0:00.05
Pull Request HEAD: 291708d1aef2a87b9480ea116f89450bec6b01b7
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.89%. Comparing base (b315a6c) to head (291708d).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
+ Coverage   93.82%   93.89%   +0.06%     
==========================================
  Files          18       18              
  Lines        2575     2587      +12     
  Branches      577      580       +3     
==========================================
+ Hits         2416     2429      +13     
  Misses         91       91              
+ Partials       68       67       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dhdaines]

I think this actually is the memory mapping solution we're looking for, because if we put all the data together in a single (or multiple, whatever) block, then... we can easily memory map that block, without needing any external dependencies.

But also, if we support some kind of compact trainable G2P rules, we don't need to store the lexicon entries that are predictable by the rules, so that's something to think about (I could resume my debogosifying efforts on Phonetisaurus for instance)

[joanise]

Before we use this as memory mapping, I'd want to convert the solution from splitting the compacted blocks into memory into reading the entries in place. But yeah, if we add the ability to access individual entries via string slices, we could turn the whole list into a single string, i.e., just one compact block.
For bisect to work on this, I guess we say new_pos = (right + left)//2, then scan back until we find \x01, from there scan forward to \x01, and that's the entry to examine.
I don't know if bisect supports anything but integral indexing -- probably not -- but it's a pretty darn simple algorithm to write ourselves -- that's one I've done a number of times -- so might be the solution indeed.

Question @dhdaines do we merge this anyway, take the 15mb saving, and plan a future update with a single block as described above, or do we hold off on this PR?
I vote for merging this PR in, because

• it would already let us turn the ReadAlong-Studio Heroku server back down to a 1x from the current 2x.
• the one-block solution will take a bit longer to write and I don't have time right now for it.

[dhdaines]

Question @dhdaines do we merge this anyway, take the 15mb saving, and plan a future update with a single block as described above, or do we hold off on this PR?

Yes, merge it now, because the internal representation of our rules/lexicons is not a public API/ABI so it really doesn't matter if we change it.

[joanise]

Yes, merge it now, because the internal representation of our rules/lexicons is not a public API/ABI so it really doesn't matter if we change it.

Sounds good.

😁 can you approve it then?

[dhdaines]

Approved, though you could add the assertions / comments mentioned if you want to make sure future generations understand what's going on :)

[joanise]

Yes, adding that additional documentation (and possibly assertions) would be helpful, this is definitely not obvious code. Which is part of why I was as extensive as I was for unit testing, making unit testing confirm to me I didn't miss any corner cases.

[joanise]

Thanks for the careful review and these comments.

[joanise]

19be394 documents the algorithm much more clearly now.