-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add compatibility for NumPy 2.0 #21085
Conversation
@yufenglee, there is a test error in test_quant_util.py. I think it is caused by precision. Maybe we should just relax the tolerance. Can you help take a look? |
ONNX Runtime Python Test Pipeline (Packages_Somking_Test Test_MAC_Wheels Python310) seem to test nightly packages so the failure would be expected then? |
2 errors on ARM build. x64 is ok. =====================================================================
|
+@jambayk who added test_quantizeblockwise_bnb4.py |
Among the changes which could introduce some numerical differences (see [https://numpy.org/devdocs/numpy_2_0_migration_guide.html#changes-to-numpy-data-type-promotion](Changes to NumPy data type promotion)).
About the failing test, the tested function I think we should also add a warning in build.py to let the users know if numpy 1.x is installed when building the python binding and tell them onnxruntime is not compatible with numpy 2.0 in that case. This warning could becone an error if this condition is met during one of the pipeline building the release. |
That discrepancy looks ok. Wrapping the expected value in |
@adrianlizarraga, could you please add a commit to fix both numeric discrepancy issues? |
Talked to @yufenglee offline. We need to change the assertEqual function calls to numpy.testing.assert_allclose in test_quant_util.py |
@snn @yufenglee I pushed an update that fixes both numerical accuracies |
Now there's a different error with
|
It was my code. I will fix it. |
/azp run Windows CPU CI Pipeline |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run orttraining-amd-gpu-ci-pipeline , Linux MIGraphX CI Pipeline |
Azure Pipelines successfully started running 2 pipeline(s). |
do we need some kind of test for testing 1.x and 2.x compatibility with these numpy 2.0 builds? |
Obviously, the answer is yes. I manually tested it. But there is not coverage in our pipeline. In theory we have a lot of things to test. For example, we build our onnxruntime on Windows Server 2022 and we claim the binaries are compatible with Windows 10, but we do not have automated tests to coverage that. Similarly, we only test our python package with the latest ONNX version, while we should test it with all history ONNX version from 1.2. Due to limited resources we have, we cannot do most of the things. |
ok yeah, understood. |
I think either is fine. The difference is: if we pin it, we won't get a surprise when numpy published a new version that is incompatible with 1.21.6(now it supports as low as 1.19). The downside is: we need to manually update the pinned version number when numpy updates, which means more work. Now in the requirements.txt files, some packages are pinned, some packages are not pinned. What do you think is better? |
Let me add a commit to pin them. |
i don't think we will need to update numpy that soon after pinning to 2.0.0 |
2278aa4
to
35d81af
Compare
This reverts commit ee48d03.
@jywu-msft, I updated the requirements.txt files. Would mind reviewing them again? |
Description
As suggested by SciPy's doc, we will
Build against NumPy 2.0.0, then it will work for all NumPy versions with the same major version number (NumPy does maintain backwards ABI compatibility), and as far back as NumPy 1.19 series at the time of writing
I think it works because in numpyconfig.h#L64 there is a macro NPY_FEATURE_VERSION. By default it is set to NPY_1_19_API_VERSION. And the NPY_FEATURE_VERSION macro controls ABI.
This PR only upgrade the build time dependency; When a user installs ONNX Runtime, they still can use numpy 1.x.
Motivation and Context