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

Use fastBPE package available from pypi #9

Merged
merged 3 commits into from
Dec 5, 2019

Conversation

chiragjn
Copy link
Contributor

Firstly, Thank you for creating this pure python port.

fastBPE is now available on PyPI and needs no compiling!
https://pypi.org/project/fastBPE/

So I went ahead and did some changes to use that instead of fastBPE for my own usages. Let me know if this is good or can be improved to merge upstream

@yannvgn
Copy link
Owner

yannvgn commented Nov 7, 2019

Thanks for your contribution, @chiragjn!

It's true that fastBPE is now available on PyPI, however I have a few concerns:

  • when installed via pip install fastBPE, fastBPE still needs to be compiled
  • there are no wheels available for common operating systems
  • the installation / compilation of fastBPE is likely to fail unless you have all the needed tools installed

For example, on macOS, running pip install fastBPE fails:

  fastBPE/fastBPE.cpp:603:10: fatal error: 'ios' file not found
  #include "ios"
           ^~~~~
  1 warning and 1 error generated.
  error: command 'gcc' failed with exit status 1
  ----------------------------------------
  ERROR: Failed building wheel for fastBPE

You'll also get an error if you don't have a compiler like gcc installed.

Switching to fastBPE may make laserembeddings technically closer to Facebook's LASER, but from an end-user perspective there would be no differences (besides an additional compilation step during the installation). The main goal of laserembeddings is to be as easy to install as possible, and also as portable as possible. Provided you have a PyTorch installation, pip install laserembeddings should work regardless of the system you're working on.

So I would wait until fastBPE has a more flexible installer / has wheels available for common systems.

Regarding the typos, thanks for spotting them! Either open a new PR, or revert the fastBPE-related changes and rename this PR.

Cheers

@yannvgn
Copy link
Owner

yannvgn commented Dec 5, 2019

I'm merging this PR and I'll revert the fastBPE-related changes in another PR.
Thank you @chiragjn 👍

@yannvgn yannvgn merged commit 3c3f297 into yannvgn:master Dec 5, 2019
@yannvgn yannvgn mentioned this pull request Dec 5, 2019
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.

2 participants