r9y9 / nnmnkwii

Library to build speech synthesis systems designed for easy and fast prototyping.
https://r9y9.github.io/nnmnkwii/latest/
Other
393 stars 73 forks source link

Fixed the problem that caused installation to fail. #127

Closed decfrr closed 5 months ago

decfrr commented 5 months ago

What

Fix #126

Why

The problem was due to an unset version limit for numpy. With the release of version 2.0.0 on the 16th of this month, which contained some destructive changes, the build of this package was failing. By setting the upper version limit, the build will succeed.

Tasks

Successfully built nnmnkwii pysptk fastdtw
Installing collected packages: tqdm, threadpoolctl, scipy, joblib, fastdtw, decorator, cython, scikit-learn, pysptk, nnmnkwii
Successfully installed cython-3.0.10 decorator-5.1.1 fastdtw-0.3.4 joblib-1.4.2 nnmnkwii-0.1.2+67ff5ff pysptk-0.2.2 scikit-learn-1.5.0 scipy-1.13.1 threadpoolctl-3.5.0 tqdm-4.66.4
r9y9 commented 5 months ago

Looks good to me. Can you apply the changes in #128 to fix CI failures?

Run actions/setup-python@v1
Error: Version 3.9 with arch x64 not found
decfrr commented 5 months ago

Thanks for the confirmation. I have merged and imported the contents of the patch. Please confirm.

r9y9 commented 5 months ago

umm, looks like numpy 2.0.0 is installed on CI 🤔

Using cached numpy-2.0.0-cp39-cp39-macosx_14_0_arm64.whl (5.2 MB)
decfrr commented 5 months ago

I guess when we try to build this package using non version 2.0 numpy, we need to remove the cache in advance.

decfrr commented 5 months ago

Also, rather than specifying less than a specific version, I thought I should specify a version less than 2.0. I don't think this is a point to be particular about since it is a primary response, but I think it is better this way, so I changed it.

decfrr commented 5 months ago

I guess when we try to build this package using non version 2.0 numpy, we need to remove the cache in advance.

I too had a problem with ci downloading numpy 2.0. I will investigate.

It seems not a package requirements problem, because if we try to install nnmnkwii from the git, like pip install git+https://github.com... pip use numpy under version 2.0 and build succsessfully. Umm...

r9y9 commented 5 months ago

I guess I found the reason: setup_requires is deprecated

https://setuptools.pypa.io/en/latest/userguide/dependency_management.html

In previous versions of setuptools, this used to be accomplished with the setup_requires keyword but is now considered deprecated in favor of the PEP 517 style described above. To peek into how this legacy keyword is used, consult our guide on deprecated practice (WIP).

so, what about removing setup_requires and instead add the numpy requirement in install_requires?

decfrr commented 5 months ago

By using install_requires instead of setup_requires, the problem of numpy version had been solved.

Also, some test problems had occured, I fixed them.

  1. np.int type has been deprecated https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
  2. change error type RuntimeError -> ValueError of wrong number test
  3. Clarified librosa.effects.time_stretch argument

And I found a error of deprecated field np.object, I fixed it.

r9y9 commented 5 months ago

Thank you for the fixes! CI still fails on python 3.7 but I think it's OK to drop python 3.7 from CI. Shall we check all tests are passing except for python 3.7?

decfrr commented 5 months ago

I see that the test fails in 3.7. I'd like you to run the test on other versions just to be sure.

r9y9 commented 5 months ago

it works fine with me locally with python 3.8, numpy 1.24.3, OSX. I needed https://github.com/r9y9/nnmnkwii/pull/128/commits/907162ed95aeaafeab2422d0cc9bd290c1ea9505 for tests on OSX but everything else was OK with this PR.

r9y9 commented 5 months ago

Testing other versions/platforms with github actions (https://github.com/r9y9/nnmnkwii/pull/128) and it is working OK too. I'll merge this PR after all CI gets green by either

  1. dropping python 3.7 (easiest; I'm OK. It's already EOL https://devguide.python.org/versions/)
  2. another idea if you prefer
decfrr commented 5 months ago

I have merged the changes of including dropping python 3.7 from ci. It seems that all CI will complete jobs successfully.

decfrr commented 5 months ago

Thank you for valuable opportunity. I appereciate your ongoing maintenance.