Add megatron-core and torchcodec to requirements.txt#345
Add megatron-core and torchcodec to requirements.txt#345epiccoleman wants to merge 1 commit intoMahmoudAshraf97:mainfrom
Conversation
Summary of ChangesHello @epiccoleman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds megatron-core and torchcodec to requirements.txt to support running on Apple Silicon. My review suggests improving the requirements.txt file by sorting the dependencies, pinning their versions for reproducibility, and using environment markers for platform-specific packages. I've also recommended adding a final newline to the file, which is a common convention.
| nemo_toolkit[asr]>=2.3.0 | ||
| nltk | ||
| faster-whisper>=1.1.0 | ||
| git+https://github.com/MahmoudAshraf97/demucs.git | ||
| git+https://github.com/oliverguhr/deepmultilingualpunctuation.git | ||
| git+https://github.com/MahmoudAshraf97/ctc-forced-aligner.git | ||
| megatron-core | ||
| torchcodec No newline at end of file |
There was a problem hiding this comment.
Thanks for adding the dependencies needed for Apple Silicon. To improve the requirements.txt file, I have a few suggestions:
- Sort dependencies: It's a good practice to sort dependencies alphabetically. This makes it easier to find packages and manage the list as it grows.
- Pin versions: To ensure reproducible builds and prevent unexpected breakages from upstream changes, it's best to pin the versions of all dependencies (e.g.,
package==1.2.3). The newly added packages are not pinned. You can find the exact versions you installed by runningpip freeze. - Platform-specific dependencies: Since
megatron-coreandtorchcodecare specifically for Apple Silicon, we can use environment markers to ensure they are only installed on that platform (sys_platform == 'darwin'). This avoids installing unnecessary packages for other users. - Add a final newline: It's a convention to end files with a newline character.
I've provided a suggestion to sort the file and add a newline. Please also consider pinning the versions and adding environment markers where appropriate.
faster-whisper>=1.1.0
git+https://github.com/MahmoudAshraf97/ctc-forced-aligner.git
git+https://github.com/MahmoudAshraf97/demucs.git
git+https://github.com/oliverguhr/deepmultilingualpunctuation.git
megatron-core
nemo_toolkit[asr]>=2.3.0
nltk
torchcodec
|
Hello and thanks for the contribution, however, these two packages are not used in this project, so they are probably listed as requirements for one of the packages used in the project and I strongly suspect it's nvidia nemo, I will have to close this PR but can you share the error traceback so I can point you to the correct place to report the issue? |
When running this on my Apple Silicon Mac, I needed to install
megatron-coreandtorchcodecto get everything to work.Feel free to close this PR if there's some reason not to add these. Maybe there's a reason for these not to be included, but I figured I'd PR what i had, in case it was helpful to anyone.
Thanks for the repo, worked great for my use case.