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

loop efficiency w/ cupy #155

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

prasad-aditya
Copy link
Contributor

@prasad-aditya prasad-aditya commented Nov 25, 2021

Related Issue

Fixes N/A

Summary

  • I have read CONTRIBUTING.md to understand how to contribute to this repository :)

<Please summarize what you are trying to achieve, what changes you made, and how they acheive the desired result.>
Added cupy as a dependency, rewrote loop accordingly.

Previous runtime: 0.14577968645095826s
New runtime: 0.006239746809005737s

ISSUES

Local machine does not have CUDA installed, so cannot rely on it to pass unit testing or install cupy.

Unit Tests

If your changes touch the audio module, please run all of the audio tests and paste the output here. Likewise for image, text, & video. If your changes could affect behavior in multiple modules, please run the tests for all potentially affected modules. If you are unsure of which modules might be affected by your changes, please just run all the unit tests.

Audio

python -m unittest discover -s augly/tests/audio_tests/ -p "*"

Image

python -m unittest discover -s augly/tests/image_tests/ -p "*_test.py"
# Or `python -m unittest discover -s augly/tests/image_tests/ -p "*.py"` to run pytorch test too (must install `torchvision` to run)

Text

python -m unittest discover -s augly/tests/text_tests/ -p "*"

Video

python -m unittest discover -s augly/tests/video_tests/ -p "*"

All

python -m unittest discover -s augly/tests/ -p "*"

Other testing

If applicable, test your changes and paste the output here. For example, if your changes affect the requirements/installation, then test installing augly in a fresh conda env, then make sure you are able to import augly & run the unit test

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 25, 2021
@prasad-aditya prasad-aditya marked this pull request as draft November 25, 2021 01:26
@prasad-aditya
Copy link
Contributor Author

image

test_python check not passing due to absence of CUDA.

@prasad-aditya
Copy link
Contributor Author

prasad-aditya commented Nov 25, 2021

Can possibly use this action in workflow? @zpapakipos @jbitton

augly/audio/functional.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@prasad-aditya prasad-aditya marked this pull request as ready for review November 25, 2021 18:40
Copy link
Contributor

@zpapakipos zpapakipos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great! Since you're adding a new optional dependency, we should document that so users know how to add gpu support when installing augly. Can you add a note about this in the audio README "Installation" section? For example you could add after what's already in that section, "If you have CUDA in your environment & want to use GPU-acceleration to speed up the audio augmentations, you can install augly using the following command: pip install augly[av,gpu]."

.github/workflows/test_cupy.yml Outdated Show resolved Hide resolved
.github/workflows/test_cupy.yml Outdated Show resolved Hide resolved
.github/workflows/test_cupy.yml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants