Repository navigation
Conversation
|
|
|
Thanks for continuing to push this forward Rok! I think this PR is a great next step and is quite clear to review. I have checked out the PR locally and started with running type checkers ( The reason for these warnings and errors in I propose to push the changes directly if we agree the additions makes sense. Meanwhile I will continue with running the |
|
I have run I have also run mypy's stubtest with Besides the reports mentioned above which will be fixed in subsequent PRs, stubtest did found one bug in pyarrow: Note missing "L" in "Decimal". I will work on the fix for this now and would suggest to add it as part of this PR. Can commit directly, if that is ok. |
|
I've run Running on editable dev build 27, with pyright==1.1.414 in my conda env, I also hit errors |
That makes sense and please go for it. I think we should aim for
Huh. I've never noticed these were missing. Given we're not really using them as types I'd skip them if we can.
Huh. Let's do the fix as separate PR, but also include a fix here? (And we'll just rebase over the actual fix on main)
I'm not sure we want (can) bring in ruff as a CI dependency. However we should autoformat before we merge, so we don't do it later and change history for no good reason. So I propose one of you runs the ruff formatter and commits the change into this branch. |
I think
Sounds good. Will open an issue and a PR and add a fix here also. |
Yeah, they are internal and exposed through |
Ok, I'll do that reformatting in a bit. |
|
I have started with reviewing pyarrow tests with running click for output$ mypy --check-untyped-defs --warn-unused-ignores pyarrow/tests/test_array.py
pyarrow/tests/test_array.py:21: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:22: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:25: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:80: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:790: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:986: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:1300: error: List item 3 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:1302: error: List item 1 has incompatible type "None"; expected "list[tuple[bytes, int]]" [list-item]
pyarrow/tests/test_array.py:1313: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:1320: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:1324: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:1348: error: List item 0 has incompatible type "None"; expected "list[tuple[bytes, int]]" [list-item]
pyarrow/tests/test_array.py:1348: error: List item 1 has incompatible type "None"; expected "list[tuple[bytes, int]]" [list-item]
pyarrow/tests/test_array.py:1362: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:2513: error: Need type annotation for "buffers" (hint: "buffers: list[<type>] = ...") [var-annotated]
pyarrow/tests/test_array.py:2539: error: "object" has no attribute "dtype" [attr-defined]
pyarrow/tests/test_array.py:2541: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:2542: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:2543: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:2885: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:2917: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:2938: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:2939: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:3183: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:3299: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:3720: error: Incompatible types in assignment (expression has type "None", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
pyarrow/tests/test_array.py:3729: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:3740: error: Value of type "object" is not indexable [index]
pyarrow/tests/test_array.py:3817: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:4004: error: Incompatible types in assignment (expression has type "MyArrayInvalid", variable has type "MyArray") [assignment]
pyarrow/tests/test_array.py:4017: error: Incompatible types in assignment (expression has type "MyArray2", variable has type "MyArray") [assignment]
pyarrow/tests/test_array.py:4031: error: Incompatible types in assignment (expression has type "MyArray3", variable has type "MyArray") [assignment]
pyarrow/tests/test_array.py:4095: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:4099: error: Unused "type: ignore" comment [unused-ignore]
pyarrow/tests/test_array.py:4315: error: List item 3 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:4319: error: List item 5 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:4324: error: List item 2 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:4324: error: List item 4 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:4395: error: List item 1 has incompatible type "None"; expected "int" [list-item]
pyarrow/tests/test_array.py:4407: error: List item 0 has incompatible type "None"; expected "int" [list-item]
Found 40 errors in 1 file (checked 1 source file)Type checkers are currently configured to skip I propose to temporarily add the |
|
Another thing: I can't run |
|
Oh, I was too fast here. Unfortunately Would it make sense to remove I will leave this for discussion and start with reading the stubs and comparing them with the APIs, starting with |
|
Ignore statements not being needed is more likely due to changes to typecheckers than due to changes in stubs. Or they might be needed once we import other parts of the library. Also note camel cased (e.g. reportArgumentType) ignores are likely coming from pyright. So if mypy thinks it's a redundant ignore you might want to check pyright too. |
Let's rather remove |
|
OK, agree, when reviewing I plan to do that later and am focusing on reading the stubs currently. |
|
Ehhh... I've fallen into this interesting rabbit hole - There are over 1.5k mypy errors % mypy --check-untyped-defs --warn-unused-ignores pyarrow/tests/test_convert_builtin.py
pyarrow/tests/test_convert_builtin.py:78: error: Module not callable [operator]
pyarrow/tests/test_convert_builtin.py:79: error: Module not callable [operator]
...
pyarrow/tests/test_convert_builtin.py:168: error: Module not callable [operator]
...
Found 321 errors in 1 file (checked 1 source file)
% mypy --check-untyped-defs --warn-unused-ignores pyarrow/tests/test_*.py
Found 1866 errors in 35 files (checked 44 source files)
% ... | grep -c "Module not callable"
1562test_convert_builtin.py:168 : pa.array(... A stub So I propose we reintroduce subfolder |
|
Just to note: with commit eda3425 stubs |
|
@tadeja I suppose the |
|
@jorisvandenbossche oh, yes, I think |
|
@tadeja @rok I have checked typed.pyi and decided to push a commit (4b97c69) after all =) Instead of suggestions I will post the reasoning behind the changes here:
Future TODO:
I have run
|
Match the runtime for the entries of the stubtest allowlist that were marked as stubs to fix: static methods for the from_arrays, from_storage, from_numpy and from_numpy_ndarray constructors, the obj parameter of array(), the integer values of DeviceAllocationType, the signatures of concat_tables and Schema.__getitem__, and no __all__ in pyarrow.types. Remove UnionArray.child, ChunkedArray.data, ChunkedArray.format and Schema.field_by_name, which no longer exist at runtime. Make the type aliases that only the table stubs define private, so that stubtest does not look for them at runtime.
…ings Replace the TYPE_CHECKING_INVALID_USAGE constant of the stub tests with typing.TYPE_CHECKING, in the tests and in the developer documentation. Require the argument of MonthDayNano, and test it together with RunEndEncodedArray.from_buffers, the names of StructArray.from_arrays and tables being unhashable. Move typing_extensions from the test dependency group to requirements-stubs.txt, and link the issue that tracks shipping the stubs in the documentation.
Pass the run-end encoded children as a tuple and remove two unused ignore comments reported by mypy, pyright and ty.
| obj: Iterable[Any] | ||
| | SupportsArrowArray | ||
| | SupportsArrowDeviceArray | ||
| | SupportsPyArrowArray, |
There was a problem hiding this comment.
Array/ChunkedArray are already covered by Iterable[Any] and the protocols, so adding them to stubs isn't needed to pass type checkers. But should we still list them explicitly? They're the most common inputs and listing them makes it clearer to users when in IDE.
There was a problem hiding this comment.
Yes, personally I would also include the typical (internal) objects like Array, whenever that is accepted
There was a problem hiding this comment.
Plan to unify this in all the files, just waiting for others to chime in.
FWIW, I put some of the more general comments intentionally not in an inline comment, because while it nice that github keeps them as a thread together, they are basically impossible to find back later on .. (although maybe this redesigned PR overview page will help with that, who knows 🤷♂️ ;)) |
Nope |
| mypy only uses the stubs when it checks them too, so pass ``pyarrow-stubs`` | ||
| along with any other files you check: | ||
|
|
||
| .. code-block:: | ||
|
|
||
| $ mypy pyarrow-stubs pyarrow/tests/test_array.py |
There was a problem hiding this comment.
That is not what I see locally. Running eg mypy pyarrow/tests/test_table.py works just fine and finds the same issues whether I add pyarrow-stubs or not.
(assuming I pass --follow-untyped-imports --check-untyped-defs, otherwise it finds nothing related to pyarrow anyhow)
True. Let's then power through this one and try to make future PRs smaller. |
| | SupportsArrowDeviceArray | ||
| | SupportsPyArrowArray, | ||
| type: Any | None = None, | ||
| mask: BooleanArray | Iterable[bool] | NDArray[np.bool_] | None = None, |
There was a problem hiding this comment.
I have lost track of whether is was replied to elsewhere, but IMO this should be Array instead of BooleanArray
| mask: BooleanArray | Iterable[bool] | NDArray[np.bool_] | None = None, | |
| mask: Array | Iterable[bool] | NDArray[np.bool_] | None = None, |
| @staticmethod | ||
| def from_arrays( | ||
| offsets: Int32Array | list[int] | list[int | None] | np.ndarray, | ||
| values: Array | list[Any], |
There was a problem hiding this comment.
The <>ListArray.from_arrays functions are explicitly typed with list (also in contrast with many other places that accept "values").
But so in practice that is too narrow (eg a numpy array would now be flagged by a type checker?). In practice, this keyword is passed through to pa.array(), and so accepts anything that pa.array() accepts (so at least should be relaxed to include Iterable[Any] instead of list[Any])
There was a problem hiding this comment.
And the same goes for offsets (but at least that one includes np.ndarray explicitly)
Also, is there a reason that list[int] | list[int | None] are included both? (instead of just list[int | None])
There was a problem hiding this comment.
Fix in 08bccc8. offsets/sizes are now Iterable[int | None]. values (plus keys/items for MapArray) are now Iterable[Any] . So this gets NumPy arrays, tuples, range etc. correctly accepted instead of rejected. Downside: ChunkedArray is accepted although runtime rejects it.
|
|
||
| _ChunkedArrayConstructorInput: TypeAlias = ( | ||
| Iterable[ | ||
| Iterable[Any] |
There was a problem hiding this comment.
Related to one of the other comments @AlenkaF made, but I would include Array here as well inside the Iterable (strictly speaking it is covered by Iterable or SupportsArrowArray, but it is probably the most common way to create a chunked array to pass a list of Arrays)
…not typed with CapsuleType
|
Checked error.pyi and opened a separate issue to remove dead code: #55500. |
Rationale for this change
This is the next step in adding type annotations to PyArrow (#32609). It merges #49103 (internal type system stubs) and #49193 (core data structures) because the two depend on each other and can't be type checked separately.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
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: