FIX use pyarrow types in pyarrow.filter() for older pyarrow versions #31605

StefanieSenger · 2025-06-20T10:21:26Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This fixes a bug that appears for older pyarrow versions, when the filter method is used with other indexes than pyarrow types.

See the issue description for more details.

github-actions · 2025-06-20T10:22:15Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 6202316. Link to the linter CI: here}

StefanieSenger · 2025-06-22T02:53:04Z

I added pyarrow to pymin_conda_forge_openblas_min_dependencies, which I had hoped would fix the codecov errors (it didn't), but it is the right thing to do anyways.

The Windows pymin_conda_forge_mkl timeout error comes seems unrelated and also appeared in other PRs.

StefanieSenger · 2025-06-22T08:14:10Z

So it seems that not all the CIs get into the coverage report for codecov. I have found in the azure-pipelines.yml that there can be a Coverage: 'false' setting, that some CIs have, but ours, the pymin_conda_forge_openblas_min_dependencies doesn't seem to possess this setting. I would think that means that it is not reporting coverage? Do we want this to be like this?

Edit: I found the coverage is set to true in posix.yml, which applies to our job as well. So that means that it should make a coverage report. That is strange, because locally, with pyarrow 14.0.0, 15.0.0 and 16.0.0 I do reach into the new branches. I didn't however manage to install pyarrow==12.0.0 or pyarrow==13.0.0 into the same environment as pymin_conda_forge_openblas_min_dependencies because there were conflicting version definitions between python_abi and tornado. I now think that might happen to the CI invironment as well (but then I wonder why it keeps running ...)

StefanieSenger · 2025-06-22T09:57:03Z

To sum up: it seems that the codecov errors come from pymin_conda_forge_openblas_min_dependencies not installing pyarrow==12.0.0, because of a version mismatch.

Proposing solution: change min version of pyarrow to 14.0.0.

Edit: I tried that (not pushed) but I am not sure anymore, since the changes I did in pyproject.toml, sklearn/min_dependencies.py and sklearn/tests/test_base.py don't affect the build at all. There is no change in the newly generated lock file and now I see that pyarrow is not mentioned in the current one. That means that adding "pyarrow": "min" did not have the effect of adding pyarrow's min version to the job?

Edit: 🤦 Now I got it! I should have added pyarrow to the conda_dependencies part too, not just to the version restrictions.

StefanieSenger · 2025-06-22T14:05:45Z

All passes except for the Windows CI failing which this is unrelated and coming from main.

lorentzenchr

Thanks for the fix.

sklearn/utils/fixes.py

lorentzenchr · 2025-06-24T08:05:46Z

sklearn/utils/fixes.py

+    import pyarrow
+
+    pyarrow_version = parse_version(pyarrow.__version__)
+    if pyarrow_version < parse_version("17.0.0"):


Out of curiosity, what happens if pyarrow < 12 is installed?

I haven't tried this.

sklearn/utils/_indexing.py

lorentzenchr · 2025-06-24T10:39:22Z

sklearn/utils/_indexing.py

+            import pyarrow
+
+            if not isinstance(key, pyarrow.BooleanArray):
+                key = pyarrow.array(key, type=pyarrow.bool_())


Suggested change

import pyarrow

if not isinstance(key, pyarrow.BooleanArray):

key = pyarrow.array(key, type=pyarrow.bool_())

try:

pa = sys.modules["pyarrow"]

except KeyError: # pragma: no cover

raise ImportError("pyarrow must be installed to use pyarrow data objects")

if not isinstance(key, pa.BooleanArray):

key = pa.array(key, type=pyarrow.bool_())

Question for the 2. reviewer: Does it actually make a big enough difference (compared to import pyarrow)?

I'm not a second reviewer, but I can reason you through what I did and why:

When we are in line import pyarrow (currently 140) we already know that the user has pyarrow installed.

This is because this line can only be executed conditioned on that PYARROW_VERSION_BELOW_17 returns True. The condition check one line above makes fixes.py to be run and in it, we already have a try-except block evaluating import pyarrow (and the version, which doesn't matter here).

In that case you can just do pa = sys.modules["pyarrow"] without the try except.

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

StefanieSenger

Thanks for reviewing, @lorentzenchr!

I've commited your changes with the clearer TODO commands.

StefanieSenger · 2025-06-24T10:55:44Z

sklearn/utils/fixes.py

+    import pyarrow
+
+    pyarrow_version = parse_version(pyarrow.__version__)
+    if pyarrow_version < parse_version("17.0.0"):


I haven't tried this.

StefanieSenger · 2025-06-24T11:05:15Z

sklearn/utils/_indexing.py

+            import pyarrow
+
+            if not isinstance(key, pyarrow.BooleanArray):
+                key = pyarrow.array(key, type=pyarrow.bool_())


I'm not a second reviewer, but I can reason you through what I did and why:

When we are in line import pyarrow (currently 140) we already know that the user has pyarrow installed.

This is because this line can only be executed conditioned on that PYARROW_VERSION_BELOW_17 returns True. The condition check one line above makes fixes.py to be run and in it, we already have a try-except block evaluating import pyarrow (and the version, which doesn't matter here).

MNT fix bug with pyarrow filter function

a6babf5

github-actions bot added the module:utils label Jun 20, 2025

StefanieSenger added 2 commits June 20, 2025 12:23

little refactor

729ca81

fix ModuleNotFoundError in CI

ef17c1e

StefanieSenger changed the title ~~MNT fix bug with pyarrow filter function~~ FIX convert input to pyarrow filter() to pyaor older pyarrow versionsrrow type f Jun 20, 2025

StefanieSenger changed the title ~~FIX convert input to pyarrow filter() to pyaor older pyarrow versionsrrow type f~~ FIX convert indices passed into to pyarrow filter() to pyarrow type for older pyarrow versions Jun 20, 2025

StefanieSenger and others added 4 commits June 20, 2025 13:31

add fix for pyarrow import and version checking

0819b94

also fix versions below 14

6169bbb

add pyarrow min version to pymin CI

2dfedda

Merge branch 'main' into fix_safe_indexing_pyarrow

f2b2d38

StefanieSenger changed the title ~~FIX convert indices passed into to pyarrow filter() to pyarrow type for older pyarrow versions~~ FIX use pyarrow types in pyarrow.filter() for older pyarrow versions Jun 22, 2025

add pyarrow to the conda_dependencies

6c81cef

lorentzenchr approved these changes Jun 24, 2025

View reviewed changes

Apply suggestions from code review

30c3511

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

StefanieSenger commented Jun 24, 2025

View reviewed changes

StefanieSenger and others added 2 commits June 24, 2025 13:10

Merge branch 'main' into fix_safe_indexing_pyarrow

eec3d5f

fix linting

09d4d85

StefanieSenger added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jun 24, 2025

adrinjalali approved these changes Jun 24, 2025

View reviewed changes

adrinjalali enabled auto-merge (squash) June 24, 2025 11:19

Merge branch 'main' into fix_safe_indexing_pyarrow

6202316

adrinjalali merged commit b51965a into scikit-learn:main Jun 26, 2025
36 checks passed

StefanieSenger deleted the fix_safe_indexing_pyarrow branch June 26, 2025 09:07

-            import pyarrow
-            if not isinstance(key, pyarrow.BooleanArray):
-                key = pyarrow.array(key, type=pyarrow.bool_())
+            try:
+                pa = sys.modules["pyarrow"]
+             except KeyError:  # pragma: no cover
+                raise ImportError("pyarrow must be installed to use pyarrow data objects")
+            if not isinstance(key, pa.BooleanArray):
+                key = pa.array(key, type=pyarrow.bool_())

Uh oh!

FIX use pyarrow types in pyarrow.filter() for older pyarrow versions #31605

FIX use pyarrow types in pyarrow.filter() for older pyarrow versions #31605

Uh oh!

Conversation

StefanieSenger commented Jun 20, 2025

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

github-actions bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

StefanieSenger commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StefanieSenger commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StefanieSenger commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StefanieSenger commented Jun 22, 2025

Uh oh!

lorentzenchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lorentzenchr Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lorentzenchr Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

lorentzenchr Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger left a comment

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 20, 2025 •

edited

Loading

StefanieSenger commented Jun 22, 2025 •

edited

Loading

StefanieSenger commented Jun 22, 2025 •

edited

Loading

StefanieSenger commented Jun 22, 2025 •

edited

Loading