GH-34868: [Python] Share docstrings between classes #34894

danepitkin · 2023-04-04T20:02:58Z

Rationale for this change

Python classes sometimes duplicate docstrings, but change one word such as class name. Add a decorator function as a utility to help deduplicate docstring descriptions. Only works in Python. Does not work in Cython due to this CPython issue python/cpython#91309.

What changes are included in this PR?

Add a decorator @doc that can copy, concatenate, and/or format docstrings between classes.

Are these changes tested?

Tests added.

>>> import pyarrow
>>> from pyarrow.filesystem import FileSystem, LocalFileSystem, DaskFileSystem, S3FSWrapper
>>> from pyarrow.hdfs import HadoopFileSystem
>>> for fs in [FileSystem, LocalFileSystem, DaskFileSystem, S3FSWrapper, HadoopFileSystem]:
...     print(fs.__name__)
...     print(fs.isdir.__doc__)
... 
FileSystem

        Return True if path is a directory.

        Parameters
        ----------
        path : str
            Path to check.
        
LocalFileSystem

Return True if path is a directory.

Parameters
----------
path : str
    Path to check.

DaskFileSystem

Return True if path is a directory.

Parameters
----------
path : str
    Path to check.

S3FSWrapper

Return True if path is a directory.

Parameters
----------
path : str
    Path to check.

HadoopFileSystem

Return True if path is a directory.

Parameters
----------
path : str
    Path to check.

Note that FileSystem.isdir.__doc__ is not dedented because it does not use the @doc decorator.

Are there any user-facing changes?

No

Closes: [Python] Sharing docstrings between classes #34868

github-actions · 2023-04-04T20:03:20Z

Closes: [Python] Sharing docstrings between classes #34868

github-actions · 2023-04-04T20:03:23Z

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

AlenkaF

I think this is a good utility for writing docstrings 👍
Do we know if there are other places in pyarrow that would benefit from it?

danepitkin · 2023-04-05T19:40:01Z

arrow/python/pyarrow/types.py could be a good option! This is the first one that stood out to me, but I haven't yet thoroughly reviewed all python code. Most of the benefit would be in Cython code unfortunately..

Do you think we should add those to this PR? I was thinking of doing it in separate PRs, but who knows when that will happen!

jorisvandenbossche · 2023-04-06T14:02:34Z

Do we know if there are other places in pyarrow that would benefit from it?

I am not sure we have many places that currently are not in cython, and have repetition in docstrings. Looking through our python files, we do already have some manual templating in parquet/core.py, ipc.py or orc.py (eg to share the explanation of certain parameters between multiple docstrings), but I don't know if they would fit this pattern for using @doc.

pyarrow.filesystem is also deprecated, so that will be removed in a few releases (not that we can't improve the docs with this PR in the meantime). types.py indeed looks like the main obvious candidate where this could be useful.

danepitkin · 2023-04-06T17:53:52Z

Updated types.py:

>>> import pyarrow
>>> from pyarrow.types import *
>>> print(is_null.__doc__)

Return True if value is an instance of type: null.

Parameters
----------
t : DataType

>>> print(is_boolean.__doc__)

Return True if value is an instance of type: boolean.

Parameters
----------
t : DataType

>>> print(is_map.__doc__)

Return True if value is an instance of type: logical map.

Parameters
----------
t : DataType

>>> print(is_unicode.__doc__)

Alias for is_string.

Parameters
----------
t : DataType

>>> print(is_large_unicode.__doc__)

Alias for is_large_string.

Parameters
----------
t : DataType

danepitkin · 2023-04-06T17:56:26Z

python/pyarrow/types.py

    return t.id in _UNION_TYPES


+@doc(is_null, datatype="nested")


Do we like nested or nested type?

I chose nested type since it's describing where the type comes from.

danepitkin · 2023-04-06T17:57:43Z

python/pyarrow/types.py

    return t.id == lib.Type_DATE64


+@doc(is_null, datatype="logical map")


Do we want to keep "logical" in logical map?

The underlying implementation is @GARROW_TYPE_MAP: A repeated struct logical type.

I think we can remove that. We don't have a clear definition of "logical" types in the arrow format spec, and the fact that a map type is basically a list of structs is an implementation detail (and adding "logical" here doesn't help making that clear)

danepitkin · 2023-04-06T17:58:03Z

python/pyarrow/types.py

    return t.id in _DECIMAL_TYPES


+@doc(is_null, datatype="decimal128")


Added "128" to decimal128 here.

danepitkin · 2023-04-06T17:58:17Z

python/pyarrow/types.py

    return t.id == lib.Type_DECIMAL128


+@doc(is_null, datatype="decimal256")


Added "256" to decimal256 here.

danepitkin · 2023-04-06T17:58:53Z

python/pyarrow/types.py

    return t.id == lib.Type_INTERVAL_MONTH_DAY_NANO


+@doc(is_null, datatype="primitive")


Do we like primitive or primitive type better?

I chose primitive type since it's describing a family of types.

danepitkin · 2023-04-07T13:32:40Z

I applied @doc to the types.py!

danepitkin · 2023-04-07T19:03:10Z

One interesting limitation is if there are docstrings that use curly braces {}. If it's a doctest, we would need to replace {} with an equivalent like dict() or set(). If it's part of the descriptions, it can be escaped e.g. {{}}

example:

        Examples
        --------
        >>> import pyarrow as pa
        >>> import pandas as pd
        >>> df = pd.DataFrame({'year': [2020, 2022, 2019, 2021],
        ...                    'n_legs': [2, 4, 5, 100],
        ...                    'animals': ["Flamingo", "Horse", "Brittle stars", "Centipede"]})

vs.

        Examples
        --------
        >>> import pyarrow as pa
        >>> import pandas as pd
        >>> df = pd.DataFrame(dict('year': [2020, 2022, 2019, 2021],
        ...                    'n_legs': [2, 4, 5, 100],
        ...                    'animals': ["Flamingo", "Horse", "Brittle stars", "Centipede"]))

AlenkaF · 2023-04-11T10:13:24Z

Thanks so much for the updates, the optimisation in the types.py looks great! 👏

One interesting limitation is if there are docstrings that use curly braces {}. If it's a doctest, we would need to replace {} with an equivalent like dict() or set(). If it's part of the descriptions, it can be escaped e.g. {{}}

Oh, this is not ideal.
What exactly happens in the case where the examples use curly braces? Does this happen often?

danepitkin · 2023-04-11T19:27:52Z

Thanks so much for the updates, the optimisation in the types.py looks great! 👏

One interesting limitation is if there are docstrings that use curly braces {}. If it's a doctest, we would need to replace {} with an equivalent like dict() or set(). If it's part of the descriptions, it can be escaped e.g. {{}}

Oh, this is not ideal. What exactly happens in the case where the examples use curly braces? Does this happen often?

If the example uses curly braces, pyarrow will throw a runtime error:

@doc(is_null, datatype="primitive type")
def is_primitive(t):
    """
    Example

    >>> pydict = {"cat": 0, "dog": 1}
    """
    return lib._is_primitive(t.id)

>>> import pyarrow as pa
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/dane/code/arrow/python/pyarrow/__init__.py", line 288, in <module>
    import pyarrow.types as types
  File "/Users/dane/code/arrow/python/pyarrow/types.py", line 291, in <module>
    @doc(is_null, datatype="primitive type")
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dane/code/arrow/python/pyarrow/util.py", line 76, in decorator
    params_applied = [
                     ^
  File "/Users/dane/code/arrow/python/pyarrow/util.py", line 77, in <listcomp>
    component.format(**params)
KeyError: '"cat"'

The fix would be to replace them like this:

    >>> pydict = dict(cat = 0, dog = 1)

This mostly shows up in Cython docstrings, which are not applicable at the moment!

AlenkaF · 2023-04-12T04:47:51Z

OK. Well, I do not have much against that (using dict or set instead of {}).

jorisvandenbossche · 2023-04-13T14:44:07Z

python/pyarrow/types.py

    return t.id == lib.Type_BOOL


+@doc(is_null, datatype="integer")


Suggested change

@doc(is_null, datatype="integer")

@doc(is_null, datatype="any integer")

I would keep the "any" to clearly indicate this is for any integer (i.e. all bitwidths and signed/unsigned)

Good call! Will update

jorisvandenbossche · 2023-04-13T14:47:18Z

python/pyarrow/types.py

    return t.id == lib.Type_DATE64


+@doc(is_null, datatype="logical map")


I think we can remove that. We don't have a clear definition of "logical" types in the arrow format spec, and the fact that a map type is basically a list of structs is an implementation detail (and adding "logical" here doesn't help making that clear)

jorisvandenbossche · 2023-04-13T14:49:47Z

If it's a doctest, we would need to replace {} with an equivalent like dict() or set(). If it's part of the descriptions, it can be escaped e.g. {{}}

I think also in a doctest you can use the {{ .. }} escape approach?

(and to be clear, the current PR doesn't run into that, right? It's only for when we would want to use it more broadly for docstrings with examples?)

danepitkin · 2023-04-13T14:55:52Z

If it's a doctest, we would need to replace {} with an equivalent like dict() or set(). If it's part of the descriptions, it can be escaped e.g. {{}}

I think also in a doctest you can use the {{ .. }} escape approach?

(and to be clear, the current PR doesn't run into that, right? It's only for when we would want to use it more broadly for docstrings with examples?)

Correct! There are no uses of {} in the docstrings of this PR. You can also use {{ .. }} for descriptions, but my guess is that doctests would fail if you tried to use it in the code examples. I noticed Pandas usage always uses dict() instead of {{ .. }} in code examples explicitly, but does use {{ .. }} in the descriptions.

danepitkin · 2023-04-14T17:13:29Z

All feedback is applied!

ursabot · 2023-04-24T09:13:40Z

Benchmark runs are scheduled for baseline = f4bd43d and contender = cd14e20. cd14e20 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
[Failed] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.75% ⬆️0.12%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] cd14e201 ec2-t3-xlarge-us-east-2
[Failed] cd14e201 test-mac-arm
[Finished] cd14e201 ursa-i9-9960x
[Finished] cd14e201 ursa-thinkcentre-m75q
[Finished] f4bd43d2 ec2-t3-xlarge-us-east-2
[Failed] f4bd43d2 test-mac-arm
[Finished] f4bd43d2 ursa-i9-9960x
[Finished] f4bd43d2 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

### Rationale for this change Python classes sometimes duplicate docstrings, but change one word such as class name. Add a decorator function as a utility to help deduplicate docstring descriptions. Only works in Python. Does not work in Cython due to this CPython issue python/cpython#91309. ### What changes are included in this PR? Add a decorator `@ doc` that can copy, concatenate, and/or format docstrings between classes. ### Are these changes tested? Tests added. ``` >>> import pyarrow >>> from pyarrow.filesystem import FileSystem, LocalFileSystem, DaskFileSystem, S3FSWrapper >>> from pyarrow.hdfs import HadoopFileSystem >>> for fs in [FileSystem, LocalFileSystem, DaskFileSystem, S3FSWrapper, HadoopFileSystem]: ... print(fs.__name__) ... print(fs.isdir.__doc__) ... FileSystem Return True if path is a directory. Parameters ---------- path : str Path to check. LocalFileSystem Return True if path is a directory. Parameters ---------- path : str Path to check. DaskFileSystem Return True if path is a directory. Parameters ---------- path : str Path to check. S3FSWrapper Return True if path is a directory. Parameters ---------- path : str Path to check. HadoopFileSystem Return True if path is a directory. Parameters ---------- path : str Path to check. ``` Note that `FileSystem.isdir.__doc__` is not dedented because it does not use the `@ doc` decorator. ### Are there any user-facing changes? No * Closes: apache#34868 Authored-by: Dane Pitkin <dane@voltrondata.com> Signed-off-by: Alenka Frim <frim.alenka@gmail.com>

danepitkin requested a review from AlenkaF as a code owner April 4, 2023 20:02

github-actions bot added Component: Python awaiting review Awaiting review labels Apr 4, 2023

AlenkaF approved these changes Apr 5, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 5, 2023

danepitkin marked this pull request as draft April 6, 2023 17:35

danepitkin added 4 commits April 6, 2023 13:52

apacheGH-34868: [Python] Share docstrings between classes

f7be2d2

Refactor test cases

c3af53c

Trim trailing whitespace

92d238c

Apply doc decorator to types.py

ae963a9

danepitkin force-pushed the danepitkin/ARROW-34868 branch from 6faa4dc to ae963a9 Compare April 6, 2023 17:53

danepitkin commented Apr 6, 2023

View reviewed changes

clean up type descriptions for primitive and nested

553b7a4

danepitkin requested a review from AlenkaF April 7, 2023 13:26

danepitkin marked this pull request as ready for review April 7, 2023 19:13

jorisvandenbossche reviewed Apr 13, 2023

View reviewed changes

github-actions bot added awaiting review Awaiting review awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 13, 2023

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Apr 14, 2023

danepitkin added 2 commits April 14, 2023 12:56

Apply feedback to docstrings, add test showing curly brace escaping

347c87d

Linter fix

2448662

AlenkaF approved these changes Apr 14, 2023

View reviewed changes

AlenkaF merged commit cd14e20 into apache:main Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34868: [Python] Share docstrings between classes #34894

GH-34868: [Python] Share docstrings between classes #34894

danepitkin commented Apr 4, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Apr 4, 2023

github-actions bot commented Apr 4, 2023

AlenkaF left a comment

danepitkin commented Apr 5, 2023 •

edited

Loading

jorisvandenbossche commented Apr 6, 2023

danepitkin commented Apr 6, 2023

danepitkin Apr 6, 2023

danepitkin Apr 6, 2023 •

edited

Loading

danepitkin Apr 6, 2023

danepitkin Apr 6, 2023

jorisvandenbossche Apr 13, 2023

danepitkin Apr 6, 2023

danepitkin Apr 6, 2023

danepitkin Apr 6, 2023

danepitkin Apr 6, 2023

danepitkin commented Apr 7, 2023

danepitkin commented Apr 7, 2023

AlenkaF commented Apr 11, 2023

danepitkin commented Apr 11, 2023 •

edited

Loading

AlenkaF commented Apr 12, 2023

jorisvandenbossche Apr 13, 2023

danepitkin Apr 14, 2023

jorisvandenbossche Apr 13, 2023

jorisvandenbossche commented Apr 13, 2023

danepitkin commented Apr 13, 2023

danepitkin commented Apr 14, 2023

ursabot commented Apr 24, 2023

		return t.id == lib.Type_DATE64


		@doc(is_null, datatype="logical map")

		return t.id in _DECIMAL_TYPES


		@doc(is_null, datatype="decimal128")

		return t.id == lib.Type_DECIMAL128


		@doc(is_null, datatype="decimal256")

		return t.id == lib.Type_INTERVAL_MONTH_DAY_NANO


		@doc(is_null, datatype="primitive")

		return t.id == lib.Type_BOOL


		@doc(is_null, datatype="integer")

	@doc(is_null, datatype="integer")
	@doc(is_null, datatype="any integer")

GH-34868: [Python] Share docstrings between classes #34894

GH-34868: [Python] Share docstrings between classes #34894

Conversation

danepitkin commented Apr 4, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Apr 4, 2023

github-actions bot commented Apr 4, 2023

AlenkaF left a comment

Choose a reason for hiding this comment

danepitkin commented Apr 5, 2023 • edited Loading

jorisvandenbossche commented Apr 6, 2023

danepitkin commented Apr 6, 2023

Choose a reason for hiding this comment

danepitkin Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin commented Apr 7, 2023

danepitkin commented Apr 7, 2023

AlenkaF commented Apr 11, 2023

danepitkin commented Apr 11, 2023 • edited Loading

AlenkaF commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 13, 2023

danepitkin commented Apr 13, 2023

danepitkin commented Apr 14, 2023

ursabot commented Apr 24, 2023

danepitkin commented Apr 4, 2023 •

edited by github-actions bot

Loading

danepitkin commented Apr 5, 2023 •

edited

Loading

danepitkin Apr 6, 2023 •

edited

Loading

danepitkin commented Apr 11, 2023 •

edited

Loading