-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Detect when a fasttext executable is available in PATH #3264
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3264 +/- ##
========================================
Coverage 79.53% 79.53%
========================================
Files 68 68
Lines 11781 11781
========================================
Hits 9370 9370
Misses 2411 2411 Continue to review full report at Codecov.
|
Also check that fasttext exists in FT_HOME and is executable. This is useful when using a distro like Debian that has a package of fasttext available to install.
1dd8f7e
to
0b59ffe
Compare
Can someone re-run the checks? Seems like they timed out. |
@piskvorky I'm going to merge this as-is because we use the facebook executable in multiple places during the tests. It's easier to use the executable than to store half a dozen models as test data. |
@pabs3 Thank you for your work! |
@@ -44,7 +45,8 @@ | |||
BUCKET = 10000 | |||
|
|||
FT_HOME = os.environ.get("FT_HOME") | |||
FT_CMD = os.path.join(FT_HOME, "fasttext") if FT_HOME else None | |||
FT_CMD = shutil.which("fasttext", path=FT_HOME) or \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single line please.
Requested-in: piskvorky#3264 (review)
Requested-in: #3264 (review) Co-authored-by: Michael Penkov <m@penkov.dev>
Also check that fasttext exists in FT_HOME and is executable.
This is useful when using a distro like Debian that
has a package of fasttext available to install.