Skip to content

GH-49452: [Python] Reintroduce docstring injection for stubfiles - #49453

Merged
rok merged 9 commits into
apache:mainfrom
rok:docstring-injection
Mar 13, 2026
Merged

GH-49452: [Python] Reintroduce docstring injection for stubfiles#49453
rok merged 9 commits into
apache:mainfrom
rok:docstring-injection

Conversation

@rok

@rokrok commented Mar 4, 2026

Copy link
Copy Markdown
Member

Rationale for this change

Warning: should not be merged before #49259.
See #49452 and #48618

What changes are included in this PR?

Adds a wheel build time script to populate stubfiles with runtime docstrings.

Are these changes tested?

Not yet.

Are there any user-facing changes?

Users will get docstrings.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #49452has been automatically assigned in GitHub to PR creator.

@rok
rokforce-pushed the docstring-injection branch from 095ee4c to 2056fecCompareMarch 9, 2026 17:37
@rok

rok commented Mar 9, 2026

Copy link
Copy Markdown
MemberAuthor

@raulcd this is ready for review

@rok
rokforce-pushed the docstring-injection branch from 2f0f841 to f885111CompareMarch 10, 2026 00:57
@raulcd

Copy link
Copy Markdown
Member

@github-actions crossbow submit wheel*-cp313-*

@github-actions

Copy link
Copy Markdown

Revision: f885111

Submitted crossbow builds: ursacomputing/crossbow @ actions-c1678cd8dd

TaskStatus
wheel-macos-monterey-cp313-cp313-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313-arm64GitHub Actions
wheel-macos-monterey-cp313-cp313t-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64GitHub Actions
wheel-musllinux-1-2-cp313-cp313-amd64GitHub Actions
wheel-musllinux-1-2-cp313-cp313-arm64GitHub Actions
wheel-musllinux-1-2-cp313-cp313t-amd64GitHub Actions
wheel-musllinux-1-2-cp313-cp313t-arm64GitHub Actions
wheel-windows-cp313-cp313-amd64GitHub Actions
wheel-windows-cp313-cp313t-amd64GitHub Actions

@raulcdraulcd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rok the new check on the ci/scripts/python_wheel_validate_contents.py doesn't seem to work :(

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Mar 10, 2026
@rok
rokforce-pushed the docstring-injection branch from d638b95 to 10509c4CompareMarch 10, 2026 12:49
@rok

rok commented Mar 10, 2026

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit wheel*-cp313-*

@github-actions

Copy link
Copy Markdown

Revision: 10509c4

Submitted crossbow builds: ursacomputing/crossbow @ actions-f3f679ce77

TaskStatus
wheel-macos-monterey-cp313-cp313-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313-arm64GitHub Actions
wheel-macos-monterey-cp313-cp313t-amd64GitHub Actions
wheel-macos-monterey-cp313-cp313t-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313-arm64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-amd64GitHub Actions
wheel-manylinux-2-28-cp313-cp313t-arm64GitHub Actions
wheel-musllinux-1-2-cp313-cp313-amd64GitHub Actions
wheel-musllinux-1-2-cp313-cp313-arm64GitHub Actions
wheel-musllinux-1-2-cp313-cp313t-amd64GitHub Actions
wheel-musllinux-1-2-cp313-cp313t-arm64GitHub Actions
wheel-windows-cp313-cp313-amd64GitHub Actions
wheel-windows-cp313-cp313t-amd64GitHub Actions

@rok
rokforce-pushed the docstring-injection branch from 10509c4 to e4903ebCompareMarch 10, 2026 13:18
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 12, 2026
@rok

rok commented Mar 12, 2026

Copy link
Copy Markdown
MemberAuthor

Thanks for the review @raulcd, I've pushed some changes, can you check if this now makes sense?

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 12, 2026
@rok

rok commented Mar 12, 2026

Copy link
Copy Markdown
MemberAuthor

@kou could you do a pass here too? Especially feedback on CMake changes would be valuable :).

@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 12, 2026
@rok
rokforce-pushed the docstring-injection branch from 5576c51 to e7e51dbCompareMarch 12, 2026 21:45
@rok
rokforce-pushed the docstring-injection branch from fab8f56 to a1b43d8CompareMarch 12, 2026 22:36
kou
kou approved these changes Mar 13, 2026

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Mar 13, 2026

@raulcdraulcd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rok this looks simpler now, thanks for the improvements. I have a nit about a possibly unnecessary warning appearing everywhere.

Comment threadpython/CMakeLists.txt Outdated
Comment on lines +1060 to +1061
message(WARNING "PyArrow stub source directory not found at ${PYARROW_STUBS_SOURCE_DIR}; "
"wheel will be built without .pyi files.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will happen for everything not only for wheels, I am wondering whether we want this warning at all we already have the checks for the wheels and we are setting PYARROW_REQUIRE_STUB_DOCSTRINGS on those. The validation of the wheel will fail if files are not present and this warning will appear every time we build locally now. Probably we can just remove the else case here?

Suggested change
message(WARNING "PyArrow stub source directory not found at ${PYARROW_STUBS_SOURCE_DIR}; "
"wheel will be built without .pyi files.")
message(WARNING "PyArrow stub source directory not found at ${PYARROW_STUBS_SOURCE_DIR}; "
"PyArrow will be built without .pyi files.")

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I removed the else clause now, so we don't get too much noise.

rokand others added 2 commits March 13, 2026 10:48
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Mar 13, 2026
@rok

rok commented Mar 13, 2026

Copy link
Copy Markdown
MemberAuthor

Thanks for reviews @raulcd@kou! Will merge when CI passes.

@rok
rok merged commit 0a2a0c7 into apache:mainMar 13, 2026
17 of 18 checks passed
@rok
rok deleted the docstring-injection branch March 13, 2026 10:49
@rokrok removed the awaiting changes Awaiting changes label Mar 13, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0a2a0c7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 37 possible false positives for unstable benchmarks that are known to sometimes produce them.

thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Apr 6, 2026
apache#49453)
### Rationale for this change
Warning: should not be merged before apache#49259.
See apache#49452 and apache#48618
### What changes are included in this PR?
Adds a wheel build time script to populate stubfiles with runtime docstrings.
### Are these changes tested?
Not yet.
### Are there any user-facing changes?
Users will get docstrings.
* GitHub Issue: apache#49452
Lead-authored-by: Rok Mihevc <rok@mihevc.org>
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
apache#49453)
### Rationale for this change
Warning: should not be merged before apache#49259.
See apache#49452 and apache#48618
### What changes are included in this PR?
Adds a wheel build time script to populate stubfiles with runtime docstrings.
### Are these changes tested?
Not yet.
### Are there any user-facing changes?
Users will get docstrings.
* GitHub Issue: apache#49452
Lead-authored-by: Rok Mihevc <rok@mihevc.org>
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python] Reintroduce docstring injection for stubfiles

3 participants

@rok@raulcd@kou