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

Fixed the problem that caused installation to fail. #127

Merged
merged 12 commits into from
Jun 26, 2024

Conversation

decfrr
Copy link
Contributor

@decfrr decfrr commented Jun 24, 2024

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

  • Passed all tests
  • Successful build
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
Copy link
Owner

r9y9 commented Jun 25, 2024

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
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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

@r9y9
Copy link
Owner

r9y9 commented Jun 25, 2024

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
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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

@decfrr
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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
Copy link
Owner

r9y9 commented Jun 25, 2024

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
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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
Copy link
Owner

r9y9 commented Jun 25, 2024

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
Copy link
Contributor Author

decfrr commented Jun 25, 2024

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
Copy link
Owner

r9y9 commented Jun 25, 2024

it works fine with me locally with python 3.8, numpy 1.24.3, OSX. I needed 907162e for tests on OSX but everything else was OK with this PR.

@r9y9
Copy link
Owner

r9y9 commented Jun 25, 2024

Testing other versions/platforms with github actions (#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

…numpy-version

# Conflicts:
#	pyproject.toml
#	setup.py
@decfrr
Copy link
Contributor Author

decfrr commented Jun 26, 2024

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

Copy link
Owner

@r9y9 r9y9 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@r9y9 r9y9 merged commit fe72151 into r9y9:master Jun 26, 2024
4 checks passed
@decfrr decfrr deleted the decfrr/fix-numpy-version branch June 26, 2024 02:00
@decfrr
Copy link
Contributor Author

decfrr commented Jun 26, 2024

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

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.

Cannot install nnmnkwii because of cythonizing error
2 participants