ARROW-15321: [Dev][Python] Also numpydoc-validate Cython-generated methods #12698

pitrou · 2022-03-23T16:02:53Z

The numpydoc-validation routine in Archery would skip over many Cython-generated methods and properties.

This PR also fixes and enhances the docstrings that would newly raise validation errors.

.github/workflows/docs_light.yml

github-actions · 2022-03-23T18:47:35Z

https://issues.apache.org/jira/browse/ARROW-15321

pitrou · 2022-03-23T20:08:03Z

Hmm, I need to fix more docstrings before this is ready. My local testing was unfortunately incomplete.

pitrou · 2022-03-24T10:26:22Z

@github-actions crossbow submit -g python

github-actions · 2022-03-24T11:04:22Z

Revision: 05919a88a00341e8b7c02cb6b0b936b470d1db54

Submitted crossbow builds: ursacomputing/crossbow @ actions-1790

Task	Status
test-conda-python-3.10
test-conda-python-3.7
test-conda-python-3.7-hdfs-2.9.2
test-conda-python-3.7-hdfs-3.2.1
test-conda-python-3.7-kartothek-latest
test-conda-python-3.7-kartothek-master
test-conda-python-3.7-pandas-0.24
test-conda-python-3.7-pandas-latest
test-conda-python-3.7-spark-v3.1.2
test-conda-python-3.7-turbodbc-latest
test-conda-python-3.7-turbodbc-master
test-conda-python-3.8
test-conda-python-3.8-hypothesis
test-conda-python-3.8-pandas-latest
test-conda-python-3.8-pandas-nightly
test-conda-python-3.8-spark-v3.2.0
test-conda-python-3.9
test-conda-python-3.9-dask-latest
test-conda-python-3.9-dask-master
test-conda-python-3.9-pandas-master
test-conda-python-3.9-spark-master
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3

kszucs · 2022-03-24T12:28:53Z

.github/workflows/docs_light.yml

Nice catch!

kszucs · 2022-03-24T12:30:41Z

dev/archery/archery/cli.py

dev/archery/archery/lang/python.py

kszucs · 2022-03-24T12:38:29Z

@pitrou I don't quite understand this change: inspect_signature already supports cython generated embedded signatures but this PR seems to disable that. On the other hand now it properly retrieves the module of the method, so I assume _get_module does the actual fix?

pitrou · 2022-03-24T12:56:50Z

@kszucs As answered in the comments: the problem is not the signature, it's that the Cython-generated docstring doesn't have parameter description, so this would fail the numpydoc rule PR01.

kszucs

The numpydoc validation changes LGTM, but haven't looked at the new docstrings.

pitrou · 2022-03-28T10:06:45Z

@amol- @jorisvandenbossche Could you give this a look?

jorisvandenbossche

Thanks for this nice docstring updates! Looks all good. I added some comments, but nothing that should block this PR to get merged quickly.

jorisvandenbossche · 2022-03-28T11:33:50Z

python/pyarrow/_compute.pyx

Not that it needs to be changed for this PR, but just a general future note: I think putting only "Expression" (i.e. the type) on this line is valid as well for numpydoc, and personally I find the repeating of the name "is_valid: " not giving any added value.

Or did numpydoc complain about this? (seeing you changes this also in some existing docstrings)

No, it seems to pass actually. Given that other docstrings used this convention, I just thought it was preferred.

python/pyarrow/_dataset.pyx

jorisvandenbossche · 2022-03-28T12:25:46Z

python/pyarrow/_dataset.pyx

For a separate JIRA: it might be nice to actually share this part of the docstring in both places (so users don't have to check another object's docstring to see the help), but for now it's good to deduplicate this and have a single up-to-date version.

Ok, I opened https://issues.apache.org/jira/browse/ARROW-16063

I also asked about this here: #12560 (comment) (I'll put this on Jira)

python/pyarrow/array.pxi

jorisvandenbossche · 2022-03-28T14:18:50Z

python/pyarrow/io.pxi

Do you remember why this was needed? (which error did it give?)

Aside, we should probably try to instruct sphinx/autodoc/numpydoc to not include __init__ functions in the reference docs, as I don't think we have any case where this adds value (compared to the class docstring). See eg https://arrow.apache.org/docs/dev/python/generated/pyarrow.parquet.ParquetDataset.html#pyarrow.parquet.ParquetDataset.__init__

The problem is that numpydoc tries to match the parameters documented in the BufferReader docstring with its __init__ signature. To be honest, I'm not sure why some classes prefer to define __cinit__ rather than __init__, there doesn't seem to be a consistent convention across the codebase.

Opened https://issues.apache.org/jira/browse/ARROW-16123 for this

python/pyarrow/table.pxi

…thods The numpydoc-validation routine in Archery would skip over many Cython-generated methods and properties.

pitrou · 2022-03-28T16:55:30Z

CI passed on my fork:
https://github.com/pitrou/arrow/actions/runs/2053167401
https://github.com/pitrou/arrow/actions/runs/2053167403
https://github.com/pitrou/arrow/actions/runs/2053167405

ursabot · 2022-03-28T17:02:38Z

Benchmark runs are scheduled for baseline = 3eb5673 and contender = 6ab947b. 6ab947b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.17% ⬆️0.0%] test-mac-arm
[Failed ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️3.11% ⬆️13.36%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

pitrou marked this pull request as ready for review March 23, 2022 18:40

pitrou requested review from amol-, jorisvandenbossche and kszucs March 23, 2022 18:40

github-actions bot added the Component: Python label Mar 23, 2022

pitrou commented Mar 23, 2022

View reviewed changes

.github/workflows/docs_light.yml Outdated Show resolved Hide resolved

pitrou marked this pull request as draft March 23, 2022 19:36

pitrou force-pushed the ARROW-15321-numpydoc-cython branch from 850daec to e5feb59 Compare March 23, 2022 19:48

pitrou force-pushed the ARROW-15321-numpydoc-cython branch from e5feb59 to 05919a8 Compare March 24, 2022 10:13

pitrou marked this pull request as ready for review March 24, 2022 11:52

kszucs reviewed Mar 24, 2022

View reviewed changes

.github/workflows/docs_light.yml Outdated

Copy link

Member

kszucs Mar 24, 2022

Choose a reason for hiding this comment

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

Nice catch!

kszucs reviewed Mar 24, 2022

View reviewed changes

dev/archery/archery/cli.py Outdated

Copy link

Member

kszucs Mar 24, 2022

Choose a reason for hiding this comment

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

Nice!

kszucs reviewed Mar 24, 2022

View reviewed changes

dev/archery/archery/lang/python.py Outdated Show resolved Hide resolved

kszucs approved these changes Mar 24, 2022

View reviewed changes

jorisvandenbossche approved these changes Mar 28, 2022

View reviewed changes

pitrou added 6 commits March 28, 2022 16:50

ARROW-15321: [Dev][Python] Also numpydoc-validate Cython-generated me…

1f0c7b8

…thods The numpydoc-validation routine in Archery would skip over many Cython-generated methods and properties.

Fix "docs light" configuration

a45a770

Fix docstrings

05ec1c7

More fixes

b021998

Improve comment

9a29aa9

Address review comments

dd5c342

pitrou force-pushed the ARROW-15321-numpydoc-cython branch from 30b1159 to dd5c342 Compare March 28, 2022 15:11

pitrou closed this in 6ab947b Mar 28, 2022

pitrou deleted the ARROW-15321-numpydoc-cython branch March 28, 2022 16:56

This was referenced May 28, 2025

GH-46546: [CI][Dev][Python] Use pre-commit for numpydoc #46595

Merged

[CI][Dev][Python] Validate Cython docstrings with pre-commit + numpydoc #46641

Open

ARROW-15321: [Dev][Python] Also numpydoc-validate Cython-generated methods #12698

ARROW-15321: [Dev][Python] Also numpydoc-validate Cython-generated methods #12698

Uh oh!

Conversation

pitrou commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 23, 2022

Uh oh!

pitrou commented Mar 23, 2022

Uh oh!

pitrou commented Mar 24, 2022

Uh oh!

github-actions bot commented Mar 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kszucs commented Mar 24, 2022

Uh oh!

pitrou commented Mar 24, 2022

Uh oh!

kszucs left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Mar 28, 2022

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Mar 28, 2022

Uh oh!

ursabot commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou commented Mar 23, 2022 •

edited

Loading

ursabot commented Mar 28, 2022 •

edited

Loading