AlenkaF commented on PR #55264: URL: https://github.com/apache/arrow/pull/55264#issuecomment-6683566347
Yay Joris, thanks for the review! <3 I went through the main comments, will check inline ones after lunch =) These are my opinions, Tadeja and Rok will be able to add more here. > * How should the type checkers be run? Yeah, I currently run pre-commit and also all 3 type checkers separately in my local environment. The two runs differ due to possibility of missing deps. I agree we should try to make this more clear and unified, maybe using pre-commit only or providing a requirements file for type checking locally in the dev env? (connected to ignores question) > Do we already want to run the type checkers on the pyarrow tests? I think we would have to run type checking on the tests locally until we have added all the stubs. That is what we have planned after the reviews and before the merge. See: https://github.com/apache/arrow/pull/55264#issuecomment-6466766125 > Should we specify [--follow-untyped-imports](https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-follow-untyped-imports) for mypy? Yes, I already did some testing, see: https://github.com/apache/arrow/pull/55264#issuecomment-6463891967 > Should we add stubtest to CI? (that seems important to ensure the code and stubs don't drift on subsequent changes) Yes, but that would make sense only after all the parts of PyArrow are typed. We did a stubtest check for the part from this PR and already added changes. See https://github.com/apache/arrow/pull/55264#issuecomment-6442437688 > It would be good to get some consistency in how certain input arguments are typed, eg for array-like input, I see all of Iterable, Sequence and list, while I think those all three should be the same (often, we might also functions where we are in practice stricter) Yes, I think consistency would be good. But going through the code I see we have very diverse way of checking the types in PyArrow. For example, sometimes a parameter is typed in Cython which checks types at runtime so we have to be stricter there. Or we check specifically for list or tuple with `isinstance` which then also needs a narrower stub. I tried to create and use type aliases more (for example added `ChunkedArrayConstructorInput` type alias for input when using `chunked_array` method) but do not yet see how to make this more general. > Alternatively, could we ensure those optional dependencies are installed whenever doing type checks? (which is already the case in practice?) That avoids having to add such ignores? Yeah, I think that makes sense. If we agree on the first item (how to run stubs) I can push the change and also add some documentation already (or at least add it on my todo). > Would be good to add a test for that (so we don't accidentally change that to Capsule later) Yes! I have it on my todo list 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
