Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in FastText tests #3059

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

sleepy-owl
Copy link
Contributor

This is in relation to #3050

I found two main problems which were causing the tests to fail intermittently when run in parallel. I found this problem in at least 4 tests during my initial analysis.

  1. get_tmpfile(x) only creates the file name /tmp/x which is not unique, and can be overwritten when tests run in parallel
  2. temporary_file will not create temporary directory, when absolute path is provided (by get_tmpfile)

Hence, when running these tests in parallel, they write to the same file, e.g., /tmp/gensim_fasttext.tst causing them to fail

In this PR, I fix this by

  1. Creating a temporary directory to be used by get_tmpfile
  2. Using only temporary_file instead of temporary_file(get_tmpfile(...))

These changes fix all the flaky failures reported previously and most likely many others. If these changes makes sense, I can extend this fix to test_word2vec.py and test_doc2vec.py which also have the 2nd problem described above.

Let me know what you guys think. Thanks!

@sleepy-owl
Copy link
Contributor Author

@mpenkov @piskvorky

@piskvorky
Copy link
Owner

Oh this is great work. Thanks so much @sleepy-owl, the flakiness has been bugging us for years!

@mpenkov mpenkov changed the title Fixing flaky tests due to file overwriting Fix race condition in FastText tests Mar 3, 2021
@mpenkov
Copy link
Collaborator

mpenkov commented Mar 3, 2021

Merged. Good work @sleepy-owl , and congrats on your first contribution to gensim! Thank you and keep up the good work.

By all means, if you find similar issues in other parts of the code base, please make a PR. Cheers!

@mpenkov mpenkov merged commit 70fc159 into piskvorky:develop Mar 3, 2021
@sleepy-owl
Copy link
Contributor Author

Thanks, folks! Will try to resolve more such issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants