Conversation
|
|
|
Opened a PR though we have one already opened: #51309. |
|
Warnings gone: vs: from the same @jorisvandenbossche if you have time for review. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Warning failures can leak allocated DLPack tensors in both export paths.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR updates tests and DLPack handling to address NumPy/Pandas deprecation warnings while preserving legacy behavior coverage.
Changes:
- Updated deprecated NumPy and pandas test usage.
- Filtered intentional deprecation warnings.
- Adjusted legacy DLPack export warning handling and tests.
| File | Summary |
|---|---|
python/pyarrow/tests/test_pandas.py |
Uses explicit timedelta units. |
python/pyarrow/tests/test_dlpack.py |
Updates warning filters and versioned DLPack coverage. |
python/pyarrow/tests/test_compute.py |
Updates timestamp construction and warning filters. |
python/pyarrow/tests/test_array.py |
Handles NumPy generic-unit deprecations. |
python/pyarrow/tensor.pxi |
Adjusts legacy DLPack export warning handling. |
python/pyarrow/array.pxi |
Adjusts legacy DLPack export warning handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Converting to draft as I need to think about Copilot's comment, looks right from a quick read. |
b51c0a4 to
84a8518
Compare
|
I decided to remove changes connected to the depr warning in Currently the CI is not showing any deprecation warnings in out tests: https://github.com/apache/arrow/actions/runs/35728469810/job/106747835234?pr=51404#step:6:4240 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved NumPy compatibility issues affect DLPack tests and nightly generic-unit warning coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (2)
| with pytest.raises(TypeError, match="Can only use DLPack " | ||
| "on arrays with no nulls."): | ||
| np.from_dlpack(arr) | ||
| np.from_dlpack(DLPackForwarder(arr, max_version=(1, 0))) |
|
@github-actions crossbow submit test-conda-python-3.14-pandas-nightly-numpy-nightly |
|
Revision: 84a8518 Submitted crossbow builds: ursacomputing/crossbow @ actions-0a1f3fcf86
|
| assert np.dtype(np.timedelta64) == expected | ||
|
|
||
| df = pd.DataFrame({"a": [np.timedelta64()]}) | ||
| df = pd.DataFrame({"a": [np.timedelta64(0, "s")]}) |
There was a problem hiding this comment.
While this makes the test run without the warning, I am not sure if then the surrounding asserts still make sense.
One thing to do is also to change expected = np.dtype('m8') to use "m8[s]", but no idea if that would then still have produced the initial bug (the reason this test was added, #13553)
| with pytest.raises(TypeError, match="Can only use DLPack " | ||
| "on arrays with no nulls."): | ||
| np.from_dlpack(arr) | ||
| np.from_dlpack(DLPackForwarder(arr, max_version=(1, 0))) |
There was a problem hiding this comment.
If leaving the DLPack depr question for a separate issue, I would maybe also leave out any of the dlpack-related changes here.
|
@AlenkaF thanks for the PR! I triggered a nightly pandas/numpy crossbow build, because it is there that I checked for the warnings (it might be that some of those were also showing up in the main CI builds, though) |
Rationale for this change
Test warnings accumulated in our CI.
What changes are included in this PR?
The warnings are fixed or filtered if the deprecated functionality is still being tested.
Are these changes tested?
Yes.
Are there any user-facing changes?
No. Fixing test warnings in our CI.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: