plum icon indicating copy to clipboard operation
plum copied to clipboard

Plum2 and interplay between mypy, __doc__ and sphinx

Open francesco-ballarin opened this issue 2 years ago • 18 comments

Hi @wesselb I was excited to see the improved doc support in #73, and decided to try it out in my project.

As you wrote there, integration with other tools is still an open problem, so I don't expect this issue to be closed any time soon. Still, I'd like to share what workarounds I've tried and how far they got me.

My setup: I have a workflow with flake8, mypy, sphinx and coverage, and they must all pass for the CI run to be successful.

To illustrate my workarounds, I refer to https://github.com/RBniCS/RBniCSx/blob/cd02831/rbnicsx/online/projection.py, lines from 39 to 100:

  • lines 39 to 76 contain the actual implementation;
  • lines 33 to 36 are there for the abstract version of the function to be on top when calling help;
  • lines 79 to 97 are to make mypy happy (see https://github.com/python/mypy/issues/8356#issuecomment-884548381);
  • finally line 100 is to show the documentation in the actual user-facing implementation.

The good news:

  • mypy runs successfully on my workflow,
  • the help command returns the expected documentation.

The issues:

  • [ ] this workaround introduce a lot of duplication in the code. I'll happily live with that until mypy integration is finalized.
  • [ ] sphinx complains that
CRITICAL: Unexpected section title.

when processing line 100, and the html output looks like the following: Screenshot from 2023-03-07 08-06-58 where the most noticeable differences from the standard formatting are the spurious ticks (highlighted in blue), and also the different formatting of input/outputs (highlighted in red). This is my configuration file: https://github.com/RBniCS/RBniCSx/blob/cd0283137b244f7963b2c04fd30f9166354b6f28/docs/conf.py. Are there any additional workarounds for this?

  • [ ] the __doc__ still has the leading underscore in the dispatched function names. I tried manually replacing _project_vector -> project_vector, but that didn't seem to replace anything. No big deal anyways ;)

Thanks.

francesco-ballarin avatar Mar 07 '23 07:03 francesco-ballarin

Hey @francesco-ballarin!

Thanks for opening this issue. Oof, whereas help(f) now is much more helpful, it totally slipped my mind that whatever f.__doc__ returns should also be nicely rendered by automated documentation tooling such as Sphinx. In fact, this bug even shows on in the Plum documentation!

If we take this as an example

from numbers import Number

from plum import dispatch


@dispatch.abstract
def f(x: Number, y: Number):
    """Multiply two numbers."""


@dispatch
def f(x: float, y: float):
    """Multiply two floats."""
    return x * y


@dispatch
def f(x: int, y: int):
    """Multiply two ints."""
    return x * y

then help(f) shows

f(x: numbers.Number, y: numbers.Number)
    Multiply two numbers.

    This function has further implementations documented below.

    f(x: float, y: float)
        Multiply two floats.

    f(x: int, y: int)
        Multiply two ints.

and f.__doc__ shows

'Multiply two numbers.

This function has further implementations documented below.

f\x08f(x: float, y: float)
    Multiply two floats.

f\x08f(x: int, y: int)
    Multiply two ints.
'

(In the above, I've manually replaced \ns with actual newline for readability.)

Hence, I can see three issues with the current rendering of __doc__:

  1. It contains characters such as \x08f which are used to nicely print boldface in help(f), but which trip up Sphinx.
  2. Just printing f(x: float, y: float) will not render nicely.
  3. The indentation, e.g. after f(x: float, y: float) , may cause issues. If I were to guess, this is where CRITICAL: Unexpected section title. comes from.

My proposal would be as follows. When generating __doc__, detect whether sphinx is loaded. If sphinx is loaded, render __doc__ in a way which is better compatible with Sphinx. One simple solution would be the following.

Instead of

Multiply two numbers.

This function has further implementations documented below.

f\x08f(x: float, y: float)
    Multiply two floats.

f\x08f(x: int, y: int)
    Multiply two ints.

render

Multiply two numbers.

This function has further implementations documented below.

.. code-block:: text

    f(x: float, y: float)

Multiply two floats.

.. code-block:: text

    f(x: int, y: int)

Multiply two ints.

(Or any variation on this.) I believe that this should rendered by Sphinx in a readable way. Obviously, it is still a bit of a hack, since really the additional methods should be properly rendered by Sphinx. I refer to @leycec's comment here. How would something like this sound as an intermediate solution until we've got proper Sphinx support?

wesselb avatar Mar 08 '23 08:03 wesselb

How would something like this sound as an intermediate solution until we've got proper Sphinx support?

Yeah, looks good as a workaround! However, I would probably test in a case which contains

    Parameters
    ----------

for instance

from numbers import Number

from plum import dispatch


@dispatch.abstract
def f(x: Number, y: Number) -> Number:
    """
    Multiply two numbers.
    
    Parameters
    ----------
    x
        A number
    y
        Another number
        
    Returns
    -------
    :
        Their product
    """


@dispatch
def f(x: float, y: float) -> float:
    """
    Multiply two floats.
    
    Parameters
    ----------
    x
        A float
    y
        Another float
        
    Returns
    -------
    :
        Their product
    """
    return x * y


@dispatch
def f(x: int, y: int) -> int:
    """
    Multiply two ints.
    
    Parameters
    ----------
    x
        An int
    y
        Another int
        
    Returns
    -------
    :
        Their product
    """
    return x * y

because I am unsure if having multiple blocks with the same name (Parameters or Returns) is a problem for sphinx.

francesco-ballarin avatar Mar 08 '23 11:03 francesco-ballarin

Great! And yes, you’re totally right about the test case. I might not have time to work on this in the next week and a half, but after that I should have some time. I’ll keep you posted!

wesselb avatar Mar 08 '23 19:03 wesselb

Sure, no worries! Thanks :)

francesco-ballarin avatar Mar 08 '23 19:03 francesco-ballarin

Just a brief update! A function with the following docstring

def test(x):
    """Test.

    Args:
        x (int): `x`.

    Returns:
        int: Sum.

    This function has further implementations documented below.

    .. py:function:: test(x, y)

    Args:
        x (int): `x`.
        y (int): `y`.

    Returns:
        int: Sum.

    .. py:function:: test(x, y, z)

    Args:
        x (int): `x`.
        y (int): `y`.
        z (int): `z`.

    Returns:
        int: Sum.
    """

renders in Sphinx as

image

I don't think that's bad at all! I'll proceed with this format.

wesselb avatar Apr 07 '23 14:04 wesselb

I don't think that's bad at all! I'll proceed with this format.

Looks great @wesselb, thanks!

francesco-ballarin avatar Apr 07 '23 14:04 francesco-ballarin

Perfect, @francesco-ballarin! I'll release a new version this weekend that includes this change.

wesselb avatar Apr 07 '23 15:04 wesselb

Sorry for not having released a new version yet. I'm working on it. I came across a few subtle bugs in the process resolving which is taking a while.

wesselb avatar Apr 20 '23 16:04 wesselb

This has been incorporated in the latest release v2.1.0. An example in the Plum documentation can be found here.

wesselb avatar May 27 '23 10:05 wesselb

I'm still seeing rendering issues when using something like in @francesco-ballarin's https://github.com/beartype/plum/issues/75#issuecomment-1459999960. That is, when using in conjunction with the napoleon extension, sphinx struggles to show the rest of the methods.

p-zubieta avatar Jun 05 '23 22:06 p-zubieta

Hey @pabloferz! Damn, I’m sorry to hear that. I thought this was working fine. :(

Could you post a screenshot to show how it’s looking like, and perhaps the docstrings as well?

wesselb avatar Jun 06 '23 05:06 wesselb

Hi @wesselb @pabloferz #93 looks like a great step towards closing this issue.

I attach a small module to reproduce the error related to the HTML rendering of the napoleon extension. This is basically the test case from above, properly updated to use overload as in #93. The provided makefile runs flake8, mypy, displays how help shows the documentation, and finally builds the sphinx documentation and echos the path to be used to see it in a browser.

issue75.zip

francesco-ballarin avatar Aug 29 '23 09:08 francesco-ballarin

Hey, is there any chance of decoupling plum's doc mutations from documentation systems? For example, sphinx has custom handling for functools.singledispatch, and I'd imagine there is a way via sphinx extension, etc.., to handle plum objects. (edit: sphinx handles singledispatch in a very hard-coded way :/ )

The challenge with the current approach is that it mutates every docstring with sphinx in mind, but there are several different doc systems (e.g. https://mkdocstrings.github.io/, https://machow.github.io/quartodoc/), and multiple docstring formats.

An analogous example might be numpydoc, which uses a sphinx extension to handle some special docstring behaviors. It seems like if we could have (1) a way to disable automatic appending to docstrings, like an environment variable, etc.., then (2) people could figure out hooks into doc systems separately.

machow avatar Aug 30 '23 18:08 machow

@francesco-ballarin The problem appears the NumPy-style docstrings. Specifically, Plum generates the following docstring for Sphinx:

Multiply two ints.

Parameters
----------
x
    An int
y
    Another int

Returns
-------
:
    Their product

.. py:function:: f(x: float, y: float) -> float
   :noindex:

Multiply two floats.

Parameters
----------
x
    A float
y
    Another float

Returns
-------
:
    Their product

This renders as

image

which doesn't properly parse the .. py directive.

On the other hand, with Google-style docstrings, we get

Multiply two ints.

Args:
    x (int): An int.
    y (int): Another int.

Returns:
    int: Their product

.. py:function:: f(x: float, y: float) -> float
   :noindex:

Multiply two floats.

Args:
    x (int): An int.
    y (int): Another int.

Returns:
    int: Their product

which renders as

image

This does look right, modulo some spacing that can be fixed with CSS.

I wonder if there is a way to set this up that it works for both styles of docstrings.

wesselb avatar Sep 09 '23 10:09 wesselb

Hey, is there any chance of decoupling plum's doc mutations from documentation systems?

@machow, definitely! Your suggestion of adding in an env variable or another kind should be simple. What do you envision would be the desired result? Don't append any additional docstrings whenever PLUM_SIMPLE_DOC=1 or something like that?

wesselb avatar Sep 09 '23 10:09 wesselb

Hey, yeah--something like that would be perfect! Thanks for entertaining all this weird territory of docstrings styles / doc systems 😅.

machow avatar Sep 11 '23 16:09 machow

@machow This is now implemented on master, but I'm waiting with releasing a new version until a possible beartype compatibility issue has been solved. I'll keep you up to date. :)

wesselb avatar Sep 20 '23 19:09 wesselb

@machow Plum 2.2.2 incorporates PLUM_SIMPLE_DOC.

wesselb avatar Sep 23 '23 08:09 wesselb