-
Notifications
You must be signed in to change notification settings - Fork 80
Enable compatibility with Numpy 2.0 #230
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
Conversation
Technically np.core.sctypes is private, but this is the simplest one-line change.
Remove usage of `np.core.sctypes.values()` which is private.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
=====================================
Coverage ? 77%
=====================================
Files ? 33
Lines ? 4696
Branches ? 0
=====================================
Hits ? 3622
Misses ? 1074
Partials ? 0 |
|
Getting this error on Windows: Seems like |
|
Hey @weiji14. Thanks for your contribution ;) Yes, it seems numpy has a different behaviour on Windows. I will look into it. BTW, I can also see your are contributor to https://zen3geo.readthedocs.io/en/latest/. This is such a beautiful library ;) Now I am curious, what's your interest in LitData ? Best, |
|
Cool, I don't have access to a Windows computer, so this will be tricky to debug on my end. Best I can do is to do lots of pushes and figure things out on GitHub Actions 🙂 And thanks for the compliment on zen3geo 😄 I've been meaning to update it, and have ideas of expanding some features, but am getting sucked into more low-level data pipeline stuff in Rust recently. My interest with litdata was when my teammates were looking into it at Clay-foundation/model#169 because we're fans of Lightning, so I packaged it on conda-forge, and now I'm stuck maintaining the package at https://github.com/conda-forge/litdata-feedstock (same with a few other Lightning packages) 😆 The NumPy 2.0 compat is just something I'd like to get in to clear the backlog of updates at https://github.com/conda-forge/litdata-feedstock/pulls (not a big fan of upper pins). |
|
Thanks for your contribution @weiji14 |
Adding a regression test from Lightning-AI/litData#230. Should fail with `AttributeError: `np.sctypes` was removed in the NumPy 2.0 release` until patched.
Patch from Lightning-AI/litData#230 on the requirements.txt and src/litdata/constants.py files.
* updated v0.2.16 * MNT: Re-rendered with conda-build 24.5.1, conda-smithy 3.36.2, and conda-forge-pinning 2024.07.11.08.43.30 * Remove numpy <2.0.0 upper pin and add regression test Adding a regression test from Lightning-AI/litData#230. Should fail with `AttributeError: `np.sctypes` was removed in the NumPy 2.0 release` until patched. * MNT: Re-rendered with conda-build 24.5.1, conda-smithy 3.37.1, and conda-forge-pinning 2024.07.15.01.22.34 * Add patch for NumPy 2.0 compatibility Patch from Lightning-AI/litData#230 on the requirements.txt and src/litdata/constants.py files. --------- Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Included in litdata=0.2.17, xref Lightning-AI/litData#230
* updated v0.2.17 * Remove NumPy 2.0 compatibility patch Included in litdata=0.2.17, xref Lightning-AI/litData#230 --------- Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Before submitting
What does this PR do?
Make litdata work with NumPy 2.0 by changing
np.sctypes.values()to a list of dtypes obtained fromnp.core.sctypes.values(). Also added NPY201 ruff lint rule and remove uppernumpy<2.0pin in requirements.txt.Fixes #175, specifically this error:
Also closes #201
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃