neuromorphs / tonic

Publicly available event datasets and transforms.
https://tonic.readthedocs.io
GNU General Public License v3.0
208 stars 47 forks source link

Adding some audio transforms and augmentations to tonic #273

Closed MinaKh closed 5 months ago

MinaKh commented 11 months ago

This branch includes following updates in tonic/develop:

fabrizio-ottati commented 11 months ago

@MinaKh I think you should add torchaudio to the test requirements.txt, 'cause it fails as dependency in the CI tests.

MinaKh commented 11 months ago

@MinaKh I think you should add torchaudio to the test requirements.txt, 'cause it fails as dependency in the CI tests.

Thanks, just did!

fabrizio-ottati commented 11 months ago

It seems that the tests now are running using CUDA. Maybe it is better to run them with everything on CPU? Or we could modify the requirements.txt to install the GPU version of PyTorch (both vision and audio).

MinaKh commented 11 months ago

It seems that the tests now are running using CUDA. Maybe it is better to run them with everything on CPU? Or we could modify the requirements.txt to install the GPU version of PyTorch (both vision and audio).

@fabrizio-ottati No, I can set them to run on cpu. Also there are other tests that I passed on my machine but fail on CI. I need to fix without need to install extra packages. Thanks!

biphasic commented 11 months ago

The Github Actions test suite doens't have a GPU installed, therefore there's no point in installing any CUDA dependencies. Tests should always run on CPU please. Thank you

MinaKh commented 11 months ago

The Github Actions test suite doens't have a GPU installed, therefore there's no point in installing any CUDA dependencies. Tests should always run on CPU please. Thank you

Thanks @biphasic, As far as I checked my tests are not running on GPU. The error might be caused by general imports of torch and torchaudio, or by the difference in my local version and the installed one on the server. So I included the versions in the requirements. At this point I need to be able to run tests on GitHub to understand the issue better and fix it. Currently tests are not running automatically after my pushes (perhaps needs to be authorized every time by you and other admins?).

biphasic commented 11 months ago

@MinaKh are the tests passing on your local machine?

biphasic commented 11 months ago

Also I just relaxed the Github actions approval to the minimum level possible, I hope it now works without my manual approval!

MinaKh commented 11 months ago

Hi @biphasic, Thanks for facilitating this! It seems that we need to explicitly install cpu version of torchaudio in ci-pipeline.yml. Lines 24, 45 and 66 need to be updated to following: pip install torch torchaudio torchvision --index-url https://download.pytorch.org/whl/cpu I tried to do that but apparently I don't have permission to update the workflow:

(refusing to allow a Personal Access Token to create or update workflow.github/workflows/ci-pipeline.ymlwithoutworkflowscope)

How shall we proceed?

biphasic commented 11 months ago

where do you see that error? I see a different error, again related to CUDA https://github.com/neuromorphs/tonic/actions/runs/7089430652/job/19294156537?pr=273

MinaKh commented 11 months ago

where do you see that error? I see a different error, again related to CUDA https://github.com/neuromorphs/tonic/actions/runs/7089430652/job/19294156537?pr=273

MinaKh commented 11 months ago

where do you see that error? I see a different error, again related to CUDA https://github.com/neuromorphs/tonic/actions/runs/7089430652/job/19294156537?pr=273

@biphasic Yes I see this error and I think its because we don't specify the cpu version while installing torchaudio. To fix that I proposed that update in the ci-pipeline.yml And the error that I included in the last message is what I see in my pc, not being able to push to ci-pipeline.yml . I am not authorized to push to workflow! sorry I accidentally closed the previous comment and don't know how to undo that!

fabrizio-ottati commented 11 months ago

Will get on it! On 5 Dec 2023, at 10:54, Mina Khoei @.***> wrote:

where do you see that error? I see a different error, again related to CUDA https://github.com/neuromorphs/tonic/actions/runs/7089430652/job/19294156537?pr=273

@biphasic Yes I see this error and I think what causes that is we dont specify the cpu version while installing torchaudio. Tofix that I proposed that update in the ci-pipeline.yml And the error that I included in the last message is what I see in my pc. not being able to push to ci-pipeline.yml . I am not authorized to push to workflow

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

MinaKh commented 11 months ago

@fabrizio-ottati Thanks Fabrizio, sorry I just accidentally closed this PR :D Can you undo that?

MinaKh commented 11 months ago

Ok, reopened! sorry for the inconvenience!

fabrizio-ottati commented 11 months ago

@MinaKh I need to sit and do it properly. I will update you this afternoon

fabrizio-ottati commented 11 months ago
image

I don't know why but it keeps installing the CUDA version even if I have specificied to use the CPU wheel of PyTorch.

fabrizio-ottati commented 11 months ago

Okay, it seems I convinced it @MinaKh :)

codecov-commenter commented 11 months ago

Codecov Report

Attention: Patch coverage is 97.19626% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 77.72%. Comparing base (e5bd291) to head (0af124a). Report is 22 commits behind head on develop.

Files Patch % Lines
tonic/audio_transforms.py 92.00% 2 Missing :warning:
tonic/audio_augmentations.py 98.78% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #273 +/- ## =========================================== + Coverage 76.84% 77.72% +0.88% =========================================== Files 53 55 +2 Lines 3001 3174 +173 =========================================== + Hits 2306 2467 +161 - Misses 695 707 +12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MinaKh commented 11 months ago

Okay, it seems I convinced it @MinaKh :)

Okay, it seems I convinced it @MinaKh :)

Thanks @fabrizio-ottati

fabrizio-ottati commented 11 months ago

@biphasic all the tests are passing now. I have created a separate test/torch_requirements.txt so that I can pull the CPU wheel of PyTorch. Moreover, following torchaudio documentation, specific combinations of torch and torchaudio versions need to be used to ensure safe installation.

Now the code is ready to be reviewed and CI should be safe.

MinaKh commented 11 months ago

Most transforms are encapsulated enough so that I can add them, but some stuff that uses QUTNoise things I won't be able to merge like that, unless it is made a bit more general. For example, maybe Tonic has a AddNoise class, but then in your user code at SynSense you call it with AddNoise(QUTNoise), after the principle of dependency injection

@biphasic I removed those classes (noise augmentations) from this branch. currently it is very specific and I will prepare another PR later for that.

fabrizio-ottati commented 9 months ago

What's the status on this PR? :)

MinaKh commented 9 months ago

What's the status on this PR? :)

It is ready for final review...