-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-32609: [CI] Add type checking infrastructure and CI workflow for type annotions #48618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…d script for including docstrings into stubfiles before building wheels.
afe4699 to
30017ff
Compare
AlenkaF
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @rok for working on this! Splitting up the main PR sounds like a great idea to me.
I only have one small question, otherwise the changes look good to me.
|
|
||
| if [ "${PYARROW_TEST_ANNOTATIONS}" == "ON" ]; then | ||
| # Install library stubs | ||
| pip install pandas-stubs scipy-stubs sphinx types-cffi types-psutil types-requests types-python-dateutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand why pandas stubs (and some others) are needed for testing. But am not sure about scipy and sphinx, for example. Could you help me understand? Thanks!
(in any case, stubs are probably not "expensive" dependencies?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are mostly stubs for libraries we're touching from our codebase. Stub should be fairly light and python only. I believe sphinx is inline annotated, but I'm not sure how urgent it is for us to check against it. I'll check and perhaps remove it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove sphinx I get the following on the final PR:
pyarrow/vendored/docscrape.py:622: error: Cannot find implementation or library stub for module named "sphinx.ext.autodoc" [import-not-found]
pyarrow/vendored/docscrape.py:622: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
So sphinx is not needed for functioning of pyarrow and it's probably better to then inline ignore this error (once we enable typechecking). Will remove sphinx here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pip install pandas-stubs scipy-stubs sphinx types-cffi types-psutil types-requests types-python-dateutil | |
| pip install pandas-stubs scipy-stubs types-cffi types-psutil types-requests types-python-dateutil |
|
Yes, let's wait for Raul 👍 |
Rationale for this change
This is the first in series of PRs adding type annotations to pyarrow and resolving #32609.
What changes are included in this PR?
This PR establishes infrastructure for type checking:
py.typedmarker to enable type checkingAre these changes tested?
No. This is mostly a CI change.
Are there any user-facing changes?
This does not add any actual annotations (only
py.typedmarker) so user should not be affected.