rok commented on PR #55264: URL: https://github.com/apache/arrow/pull/55264#issuecomment-6443359738
> 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 (`mypy`, `pyright` and `ty`). `mypy` and `ty` emit no errors or warnings, but `pyright` does emit `reportMissingImports` and `reportMissingModuleSource` which get fixed with adding `_compute.pyi` placeholder (similar to existing `io.pyi`) and `reportMissingModuleSource = false` under `[tool.pyright]` in `pyproject.toml`. > > The reason for these warnings and errors in `pyright` is that locally I have editable pyarrow install where scikit-build-core puts the `.so` files in a separate folder (not python/pyarrow) without `__init__.py` and pyright can't find them. As we do not need the compiled modules for the type checkers, I think it is fine to use `reportMissingModuleSource = false`. > > I propose to push the changes directly if we agree the additions makes sense. That makes sense and please go for it. I think we should aim for `pyproject.toml` setting to be as close to the state we want to maintain as possible. So far I've tried to exclude modules that are not covered and ignore as few things as possible. `reportMissingModuleSource = false` seems good, we will want to challenge it when we have complete coverage. > Part of these are also Type_* stubs, for example pyarrow.lib.Type_UINT8. As we already have TypesEnum defined here, might it make sense to also include Type_* stubs now? (I am not inclined towards adding them, just wondering). Huh. I've never noticed these were missing. Given we're not really using them as types I'd skip them if we can. > Besides the reports mentioned above which will be fixed in subsequent PRs, stubtest did found one bug in pyarrow: > > ``` > pyarrow.lib.Decima32Array is not present in stub > ``` > > 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. Huh. Let's do the fix as separate PR, but also include a fix here? (And we'll just rebase) > I've run `pre-commit run --hook-stage manual python-stubs --all-files` which currently uses [pyarrow==26.0.0 wheel](https://github.com/apache/arrow/pull/55264/changes#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9R203) modules in site-packages/pyarrow, and got all passing (`Python Stubs Lint (ruff)`, `Python Stubs Type Check (mypy/pyright/ty)`) - except for `ruff-format` which requires 10 files to be reformatted, fyi @rok `Python Stubs Format (ruff)...............................................Failed` 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. -- 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]
