[DNM] Fix FastText hash function incompatibility #2059

[aneesh-joshi]

Note: this is WIP. More thought needs to go into everything.
From a preliminary glance, this looks like it might work. But it's failing some unittests.

Changes will be made as more feedback comes in.

Addresses: #2059

[mpenkov]

Hi, just thought I'd check on how this is going. Are you still actively working on this?

[aneesh-joshi]

@mpenkov
I have locally tested my changes. I believe it's working.
I will upload my tests soon.

[mpenkov]

I've discussed this with @menshikh-iv, and there is a backwards compatibility issue that we need to handle. If we just change the hash function, then we will be unable to load older models into gensim. What we need to do is:

• Introduce a use_new_hash attribute to the model
• This attribute determines which hash function to use (the old buggy one, or your fixed one)
• When creating new models, it should be true by default
• When loading models, we should check for this attribute. If it is missing, assume that we're dealing with an old model

The above mechanism should ensure that old models keep working with newer versions of gensim. Does that make sense?

[aneesh-joshi]

@mpenkov cc: @menshikh-iv
Yes, makes sense.
I'll get started with it.

[menshikh-iv]

@aneesh-joshi don't worry, @mpenkov will do that.

From you @aneesh-joshi, I'm waiting for tests (sanity-hecks: comparing with FB FT output) & describe in details how you prepared this test (what's FB FT version you used, how you run that, etc), thanks for your help 👍

[aneesh-joshi]

@menshikh-iv @mpenkov

Tests are pushed.

Summary of test:

I have added three txt files in gensim/test/test_data/ft_test_data

• no_unicode.txt : contains no unicode words
• only_unicode.txt : contains only unicode words (I think. May contains some non unicode characters)
• unicode_non_unicode_mix.txt : contains a mix of unicode and non unicode

The script test_ft.sh :

• clones the facebook github repo
• runs make
• builds 3 models for the three txt files mentioned above

Once the models are built, you should run test_models.sh

This runs a grep on the .vec file created by FB and then builds a gensim model from the .bin. These two values should match.

If you use the current develop, you will get different vectors for the same words from ft and gensim's reconstruction.
If you use this PR's code, you will get the same results.

My changes:

Since gensim defaults to the Cython version and I didn't want to touch the Cython code just yet, I have intentionally raise a Import Error so that it defaults to the Python version. I know, bad practice! But this is just for testing. Most of the code here won't be merged.

Further, I have changed the hash function as suggested in the corresponding issue.

I have locally run test_fasttext.py and it passes all cases:

Ran 40 tests in 165.098s

OK

However, I don't know if me raising an ImportError affects this.

[menshikh-iv]

thank you @aneesh-joshi 👍

[aneesh-joshi]

@mpenkov @menshikh-iv
I'd be glad to add the backward compatibility check. I've done it once before for w2v. Unless @mpenkov has already finished or started it?

What other work is pending for this PR?
(I know Cython is but I'm holding that off till the current changes are approved.)

[menshikh-iv]

@aneesh-joshi
@mpenkov work on massive fixing FT issues, your PR will help them much with #2059 (he already work on it).

Nothing else needed from your side (at least now), thanks for you help 🔥

[aneesh-joshi]

@menshikh-iv
Ok, I would just like to point out to @mpenkov that my change to fasttext.py seems to be resolving the issue superficially at least.

[menshikh-iv]

thanks @aneesh-joshi, you helped us much 👍 , overlapped by #2313