-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Adjust setup.py. Map scripts to binary names. Adjust Readme docs. #23
Conversation
requirements.txt
Outdated
@@ -3,3 +3,4 @@ datasets | |||
torchaudio | |||
soundfile | |||
librosa | |||
fairseq2 |
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.
As it's early stage of fairseq2 shall we add version to avoid compatibility issues?
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.
Sg, fixed
README.md
Outdated
``` | ||
conda install -y -c conda-forge libsndfile | ||
``` | ||
At this point fairseq2 has a confirmed support only for Linux and macOS. |
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.
Shall we make it explicit that macOS support ✅ but pre-built package ❌, as Can mentioned in #19
right now we don't have pre-built packages available for macOS. For the time-being I suggest using a Linux box. We plan to have macOS wheels available pretty soon.
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.
Added
setup.py
Outdated
package_dir={"": "src"}, | ||
package_data={"": ["assets/cards/*.yaml"]}, | ||
version="1.0.0", | ||
packages=find_packages(where="src") + ['m4t_scripts.finetune', 'm4t_scripts.predict'], |
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.
Do we expect heavy code (that's not main entry) in finetune / predict or something importable? I was thinking we should move things that need to be imported to src/ all. But open to this change here.
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.
This is to expose CLI scripts as executables.
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.
Could you rename python scripts/m4t/predict/predict.py
to m4t_predict
here too? https://github.com/facebookresearch/seamless_communication/blob/main/scripts/m4t/predict/README.md
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.
We create new directories seamless_communication
and m4t_scripts
, do we want that? cc @cndn
class cmd_for_editable_mode(develop): | ||
def run(self): | ||
# add symlinks for modules if install in editable mode | ||
_add_symlinks() |
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.
Maybe I am missing something, but are these symlinks necessary? Normally pip install -e .
adds the relative "src" directory to Python's sys.path
, so Python should be able to resolve.
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.
Editable mode of Python creates a symlink in site-packages to the module's parent folder. This logic does not work if you have more than one module to expose and they don't share their parent folder.
# symlinks | ||
seamless_communication | ||
m4t_scripts |
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.
What Ruslan means by "more than one module to expose" @cbalioglu
Adjust setup.py:
Adjust Readme docs:
python .. path to script.py
Adjustments in the script files:
E2E evaluation:
Same if installing in editable mode (also testing python 3.10):