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

Fix Deprecation/Future Warnings in Notebook 211-Speech-to-Text #594

Merged
merged 8 commits into from Jul 29, 2022

Conversation

YDX-2147483647
Copy link
Contributor

In the committed version, Imports are at the top of the notebook.

  • librosa.filters.mel in audio_to_mel

    FutureWarning: Pass sr=16000, n_fft=512 as keyword args. From version 0.10 passing these as positional arguments will result in an error.

Question

The original pull requeset did NOT add librosa (the audio analysis package used here) into requirements.txt or .docker/Pipfile. Is it on purpose? Should I tell how to install it in the notebook?

- [`waveplot` is deprecated in favor of `waveshow` since librosa v0.9.](https://librosa.org/doc/latest/changelog.html?highlight=waveplot#v0-8-1)

  Note that `max_sr` in [`waveplot`](http://librosa.org/doc/0.8.1/generated/librosa.display.waveplot.html?highlight=max_sr#librosa.display.waveplot) is removed. `max_sr` controls how signals are down-sampled if they’re too long. (`waveplot` uses “a downsampled amplitude envelope” instead)

- `librosa.filters.mel` in `audio_to_mel`

  FutureWarning: Pass `sr=16000, n_fft=512` as keyword args. From version 0.10 passing these as positional arguments will result in an error.
`max_points` should be an `int`, not `float`.
@YDX-2147483647
Copy link
Contributor Author

Sorry, it's mine. max_points in waveshow should be an int, not float.

(I checked it on a private repo and manually copy the changes to this repo - the screenshot was right.)

@Debskij
Copy link

Debskij commented Jul 25, 2022

Hi @YDX-2147483647, thanks for your contribution. It would be great if you can add librosa to both requirements.txt and Pipfile (and lock it afterward).

@YDX-2147483647
Copy link
Contributor Author

YDX-2147483647 commented Jul 26, 2022

New discovery: librosa v0.8.1 exists in Pipfile.lock.

I pip install again and found it required by ppgan v2.1.0.

> pip show librosa
Name: librosa
Version: 0.8.1
……
Requires: audioread, decorator, joblib, numba, numpy, packag
ing, pooch, resampy, scikit-learn, scipy, soundfile
Required-by: ppgan

ppgan's requirements.txt:

librosa==0.8.1

It seems that ppgan only uses librosa to read/write wav. Therefore I'll turn to ppgan and mark this PR as WIP.

@YDX-2147483647 YDX-2147483647 changed the title Fix Deprecation/Future Warnings in Notebook 211-Speech-to-Text [WIP] Fix Deprecation/Future Warnings in Notebook 211-Speech-to-Text Jul 26, 2022
@YDX-2147483647 YDX-2147483647 marked this pull request as draft July 26, 2022 15:12
@adrianboguszewski
Copy link
Contributor

@YDX-2147483647 please add it to requirements and Pipfile anyway

@YDX-2147483647
Copy link
Contributor Author

✓ OK, but maybe tomorrow.

@YDX-2147483647 YDX-2147483647 marked this pull request as ready for review July 28, 2022 03:52
@YDX-2147483647 YDX-2147483647 changed the title [WIP] Fix Deprecation/Future Warnings in Notebook 211-Speech-to-Text Fix Deprecation/Future Warnings in Notebook 211-Speech-to-Text Jul 28, 2022
@Debskij
Copy link

Debskij commented Jul 28, 2022

Hi @YDX-2147483647. Thanks for updating requirements.txt and Pipfile, still I can see that you haven't updated the Pipfile.lock file. To do that you have to (on your own machine) get into the directory openvino_notebooks/.docker and run command pipenv lock. Please add the updated file to the repository when it is ready.

@YDX-2147483647
Copy link
Contributor Author

Uhh… Thanks for helping me. In fact I had noticed “(and lock it afterward)”, but got stuck at pipenv install for hours (even on main). I am working on it.

@YDX-2147483647
Copy link
Contributor Author

Could I leave Pipfile.lock as it is?

Reason: As mentioned above, there’s no new requirement at all — we only elevate librosa from ppgan's sub-dependency to a direct dependency. Current Pipfile.lock does contain librosa (v0.8.1), making it a valid environment.

My difficulty: I can’t pipenv install even on main because of PyTorch.

My tries

(Failures omitted)

  • HTTP Errors.

    I use a mirror source instead, then manually change Pipfiles back before I commit.

  • PyPI can't recognize +cpu for torch and torchvision.

    With everything else settled, I pipenv run pip install from PyTorch's site.

  • pipenv lock failed. (pipenv lock without PyTorch succeeded.)

    Use advanced syntax in Pipfile.

  • No space left on device.

    Clean pip cache. Used up again. Clean. Again. …

    (Maybe a pipenv issue: Pipenv downloads PyTorch for all versions of Python, grabbing 16GB of data instead of just 1.7GB)

It sounds ridiculous that I am fighting against PyTorch environment days and night without using it… But it's truth. ╮(╯_╰)╭

.docker/Pipfile Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@adrianboguszewski
Copy link
Contributor

@YDX-2147483647 ok, we will add librosa in another PR, let's merge your changes in 211

@YDX-2147483647
Copy link
Contributor Author

Thanks!

@adrianboguszewski adrianboguszewski merged commit 6ce4752 into openvinotoolkit:main Jul 29, 2022
OpenVINO-dev-contest pushed a commit to OpenVINO-dev-contest/openvino_notebooks that referenced this pull request Dec 6, 2022
…inotoolkit#594)

* Fix deprecation/future warnings in notebook 211

- [`waveplot` is deprecated in favor of `waveshow` since librosa v0.9.](https://librosa.org/doc/latest/changelog.html?highlight=waveplot#v0-8-1)

  Note that `max_sr` in [`waveplot`](http://librosa.org/doc/0.8.1/generated/librosa.display.waveplot.html?highlight=max_sr#librosa.display.waveplot) is removed. `max_sr` controls how signals are down-sampled if they’re too long. (`waveplot` uses “a downsampled amplitude envelope” instead)

- `librosa.filters.mel` in `audio_to_mel`

  FutureWarning: Pass `sr=16000, n_fft=512` as keyword args. From version 0.10 passing these as positional arguments will result in an error.

* Fix `waveshow` Speech to text notebook 211

`max_points` should be an `int`, not `float`.

* add librosa to requirements

* Update .docker/Pipfile

* Update requirements.txt

Co-authored-by: Adrian Boguszewski <adekboguszewski@gmail.com>
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.

None yet

3 participants