Fix FastText model reading of unsupported modes from Facebook's FastText #3222

Tewatia5355 · 2021-08-30T16:28:35Z

Fixes #3179: Added error handling.

SagarDollin · 2021-08-31T09:31:17Z

Hey @Tewatia5355 @karan121bhukar ,
Similar to your PR , even my PR #3220 passes all the checks except for the build wheels. I have cloned the project on a windows system. Any idea how to fix this?

Tewatia5355 · 2021-08-31T10:46:19Z

Hey @Tewatia5355 @karan121bhukar ,
Similar to your PR , even my PR passes all the checks except for the build wheels. I have cloned the project on a windows system. Any idea how to fix this?

Hey there, I'm still figuring out why it didn't worked!! Will tell you if I get some leads :)

SagarDollin · 2021-08-31T10:57:02Z

Hey there, I'm still figuring out why it didn't worked!! Will tell you if I get some leads :)

I think this is because the file build-wheels . A change was made 13 days ago where they added username. The error we are getting is also related to username:

tagging @mpenkov : Should we create an issue for this. I observed that in the previous pull requests this issue wasn't seen. Infact the newer PR seems to be having more checks than previous one's.

mpenkov · 2021-08-31T12:22:20Z

Thank you for letting me know. I'll disable wheel building for PRs.

SagarDollin · 2021-08-31T12:24:02Z

Thank you for letting me know. I'll disable wheel building for PRs.

Thank you, does this mean we have to create another pull request to pass the checks?

mpenkov · 2021-09-01T00:04:26Z

No, there's no need for that.

piskvorky · 2022-02-19T15:31:03Z

gensim/models/fasttext.py

@@ -807,6 +807,13 @@ def _load_fasttext_format(model_file, encoding='utf-8', full_model=True):
    with utils.open(model_file, 'rb') as fin:
        m = gensim.models._fasttext_bin.load(fin, encoding=encoding, full_model=full_model)

+    # Here we are checking for unsupported FB FT Modes
+    if m.loss != 1 and m.loss != 2:


Suggested change

if m.loss != 1 and m.loss != 2:

if m.loss not in (1, 2):

According to #3223, negative sampling is loss=0, not loss=2. Can you please double check? What's the correct value?

Hi @piskvorky,
I checked it. In the parameters docstring in the models/_fasttext_bin, that I have copied here:

"""Holds data loaded from the Facebook binary. Parameters . . neg : int If non-zero, indicates that the model uses negative sampling. loss : int If equal to 1, indicates that the model uses hierarchical sampling. model : int If equal to 2, indicates that the model uses skip-grams. """

This suggests that m.neg==0 means no negative sampling, and m.neg == n (where n is a positive integer) means negative sampling is being used.
So I think if we are checking for negative sampling, the condition should be if m.neg == 0 or if m.neg != 0 based on what we want to do. From what I have read from the #3223 I gather that negative sampling and hierarchical softmax are supported in loss. so we shouldn't really need to check for negative sampling i.e, if m.neg != 0 (not really required)

I have made a list of what's supported and what's not based on #3223 . Please let me know if they are right, ill make changes as per that:

Supported:
a. Loss

1. Negative Sampling 2. Hierarchical Softmax

b. Model

1. CBOW 2. Skip-gram

Unsupported:
a. Loss:

OneVsAll_Loss

b. Model:

Supervised
Let me know if there are any inconsistencies in my understanding. I too want to help you complete this asap.

Sorry, I don't really understand that list. If the check is == 0 or != 0, why would we check for == 1 and == 2?

I don't know the fasttext parameters, so don't ask me :) Can you please check the fasttext docs or code (not gensim) for what the parameters actually are? And then make changes here to match that. Thanks.

Sorry, I don't really understand that list. If the check is == 0 or != 0, why would we check for == 1 and == 2?

Hi yes you are right. We shouldn't be checking ==1 or ==2 in this case, also if we are checking for negative sampling we must not, use m.loss at all we must check for m.neg. (Just to clarify the above ==1 and ==2 was not done by me, this PR was created by @Tewatia5355, Im just trying to figure out what are the losses and models not supported in fasttext. )

I don't know the fasttext parameters, so don't ask me :) Can you please check the fasttext docs or code (not gensim) for what the parameters actually are? And then make changes here to match that. Thanks.

Hi, yes sure I can investigate that and ill create a PR accordingly. Also if you can provide the link for fasttext docs that would be great. (just to be sure that I'm referring the right docs). Thanks!

OK, please do. This PR seems incorrect so I'm removing the "Next release" tag – we need to fix it first, make the code correct.

Fasttext code is at https://github.com/facebookresearch/fastText.

Thanks!

piskvorky · 2022-02-19T15:31:19Z

gensim/models/fasttext.py

+    # Here we are checking for unsupported FB FT Modes
+    if m.loss != 1 and m.loss != 2:
+        raise ValueError("Loss paramter value can be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")
+    elif m.model != 1 and m.model != 2:


Suggested change

elif m.model != 1 and m.model != 2:

elif m.model not in (1, 2):

piskvorky · 2022-02-19T15:33:40Z

gensim/models/fasttext.py

@@ -807,6 +807,13 @@ def _load_fasttext_format(model_file, encoding='utf-8', full_model=True):
    with utils.open(model_file, 'rb') as fin:
        m = gensim.models._fasttext_bin.load(fin, encoding=encoding, full_model=full_model)

+    # Here we are checking for unsupported FB FT Modes
+    if m.loss != 1 and m.loss != 2:
+        raise ValueError("Loss paramter value can be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")


Suggested change

raise ValueError("Loss paramter value can be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")

raise ValueError("The fasttext `loss` parameter must be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")

piskvorky · 2022-02-19T15:34:56Z

gensim/models/fasttext.py

+        raise ValueError("Loss paramter value can be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")
+    elif m.model != 1 and m.model != 2:
+        raise ValueError(
+            "Model parameter value can be either 1 (for Continous Bag of Words model) or 2 (for Skip-gram model)")


Suggested change

"Model parameter value can be either 1 (for Continous Bag of Words model) or 2 (for Skip-gram model)")

"The fasttext `model` parameter must be either 1 (for Continuous Bag of Words model) or 2 (for Skip-gram model)")

piskvorky · 2022-02-19T15:35:30Z

gensim/similarities/fastss.c

@@ -0,0 +1,8416 @@
+/* Generated by Cython 0.29.23 */


Why is this here? Please remove from the PR.

piskvorky · 2022-02-19T15:38:52Z

gensim/models/fasttext.py

@@ -807,6 +807,13 @@ def _load_fasttext_format(model_file, encoding='utf-8', full_model=True):
    with utils.open(model_file, 'rb') as fin:
        m = gensim.models._fasttext_bin.load(fin, encoding=encoding, full_model=full_model)

+    # Here we are checking for unsupported FB FT Modes


Suggested change

# Here we are checking for unsupported FB FT Modes

# Check for unsupported Facebook FastText modes.

piskvorky · 2022-02-25T12:48:31Z

@SagarDollin can you address the above please? We're planning a new release soon, it'd be great to include your fix.

SagarDollin · 2022-02-25T13:04:02Z

@SagarDollin can you address the above please? We're planning a new release soon, it'd be great to include your fix.

Hi. I have approved the changes you requested. I'll also give some time for the comments you mentioned. However Im currently in between some other work and I'll get back to this within 2-3 days.

By addressing this did you mean only approving the requested changes or to address all the comments?

Thanks,
Sagar B Dollin

piskvorky · 2022-02-25T13:12:58Z

By addressing this did you mean only approving the requested changes or to address all the comments?

"All the comments" – although I only see one: loss=0 vs loss=2. Is there anything else unresolved?

The rest are cosmetic language changes, to be simply accepted.

SagarDollin · 2022-02-25T13:28:49Z

By addressing this did you mean only approving the requested changes or to address all the comments?

"All the comments" – although I only see one: loss=0 vs loss=2. Is there anything else unresolved?

The rest are cosmetic language changes, to be simply accepted.

By all the other comments I meant these comments on PR #3220 :
#3220 (review)

piskvorky · 2022-02-25T13:41:00Z

Hm, is that necessary? #3220 is a separate PR. Is it blocking this PR in some way?

SagarDollin · 2022-02-25T13:50:02Z

Hm, is that necessary? #3220 is a separate PR. Is it blocking this PR in some way?

No not at all. I'll take a look at this one first.

Thanks..

…eling-issue-3179

piskvorky · 2022-04-15T12:09:59Z

@SagarDollin can you finish this please? We'd like to release ASAP. Thanks!

piskvorky · 2022-04-25T08:12:23Z

The code in this PR seems incorrect (see #3222 (comment)) and its author went MIA, so closing here. But fixing the issue is still desirable.

added error handling for issue 3179

7a15d82

Tewatia5355 mentioned this pull request Aug 30, 2021

Gensim's FastText model reads in unsupported modes from Facebook's FastText #3179

Open

SagarDollin mentioned this pull request Aug 31, 2021

Updated RandomState (deprecated from numpy) to default_rng (Generator) #3220

Open

piskvorky changed the title ~~Fixes #3179~~ Fix FastText model reading of unsupported modes from Facebook's FastText Feb 18, 2022

piskvorky added this to the Next release milestone Feb 18, 2022

piskvorky mentioned this pull request Feb 19, 2022

Added warning for unsupported Facebook FastText modes #3223

Closed

piskvorky requested changes Feb 19, 2022

View reviewed changes

SagarDollin approved these changes Feb 20, 2022

View reviewed changes

piskvorky self-assigned this Feb 25, 2022

SagarDollin approved these changes Feb 25, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into added-error-hand…

fdc2ed7

…eling-issue-3179

piskvorky added the awaiting reply label Apr 15, 2022

piskvorky closed this Apr 25, 2022

piskvorky removed this from the Next release milestone Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FastText model reading of unsupported modes from Facebook's FastText #3222

Fix FastText model reading of unsupported modes from Facebook's FastText #3222

Tewatia5355 commented Aug 30, 2021 •

edited by piskvorky

SagarDollin commented Aug 31, 2021 •

edited

Tewatia5355 commented Aug 31, 2021

SagarDollin commented Aug 31, 2021

mpenkov commented Aug 31, 2021

SagarDollin commented Aug 31, 2021

mpenkov commented Sep 1, 2021

piskvorky Feb 19, 2022

piskvorky Feb 19, 2022

SagarDollin Apr 16, 2022 •

edited

piskvorky Apr 16, 2022 •

edited

SagarDollin Apr 16, 2022 •

edited

piskvorky Apr 25, 2022 •

edited

piskvorky Feb 19, 2022

piskvorky Feb 19, 2022

piskvorky Feb 19, 2022

piskvorky Feb 19, 2022

piskvorky Feb 19, 2022

piskvorky commented Feb 25, 2022

SagarDollin commented Feb 25, 2022

piskvorky commented Feb 25, 2022

SagarDollin commented Feb 25, 2022

piskvorky commented Feb 25, 2022 •

edited

SagarDollin commented Feb 25, 2022

piskvorky commented Apr 15, 2022

piskvorky commented Apr 25, 2022

	elif m.model != 1 and m.model != 2:
	elif m.model not in (1, 2):

	raise ValueError("Loss paramter value can be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")
	raise ValueError("The fasttext `loss` parameter must be either 1 (for Hierarchical Softmax) or 2 (for Negative Sampling)")

	"Model parameter value can be either 1 (for Continous Bag of Words model) or 2 (for Skip-gram model)")
	"The fasttext `model` parameter must be either 1 (for Continuous Bag of Words model) or 2 (for Skip-gram model)")

	# Here we are checking for unsupported FB FT Modes
	# Check for unsupported Facebook FastText modes.

Fix FastText model reading of unsupported modes from Facebook's FastText #3222

Fix FastText model reading of unsupported modes from Facebook's FastText #3222

Conversation

Tewatia5355 commented Aug 30, 2021 • edited by piskvorky

SagarDollin commented Aug 31, 2021 • edited

Tewatia5355 commented Aug 31, 2021

SagarDollin commented Aug 31, 2021

mpenkov commented Aug 31, 2021

SagarDollin commented Aug 31, 2021

mpenkov commented Sep 1, 2021

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

SagarDollin Apr 16, 2022 • edited

Choose a reason for hiding this comment

piskvorky Apr 16, 2022 • edited

Choose a reason for hiding this comment

SagarDollin Apr 16, 2022 • edited

Choose a reason for hiding this comment

piskvorky Apr 25, 2022 • edited

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky Feb 19, 2022

Choose a reason for hiding this comment

piskvorky commented Feb 25, 2022

SagarDollin commented Feb 25, 2022

piskvorky commented Feb 25, 2022

SagarDollin commented Feb 25, 2022

piskvorky commented Feb 25, 2022 • edited

SagarDollin commented Feb 25, 2022

piskvorky commented Apr 15, 2022

piskvorky commented Apr 25, 2022

Tewatia5355 commented Aug 30, 2021 •

edited by piskvorky

SagarDollin commented Aug 31, 2021 •

edited

SagarDollin Apr 16, 2022 •

edited

piskvorky Apr 16, 2022 •

edited

SagarDollin Apr 16, 2022 •

edited

piskvorky Apr 25, 2022 •

edited

piskvorky commented Feb 25, 2022 •

edited