Skip to content
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

Plum2 and interplay between mypy, __doc__ and sphinx #75

Open
3 tasks
francesco-ballarin opened this issue Mar 7, 2023 · 18 comments
Open
3 tasks

Plum2 and interplay between mypy, __doc__ and sphinx #75

francesco-ballarin opened this issue Mar 7, 2023 · 18 comments

Comments

@francesco-ballarin
Copy link
Contributor

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:

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.

@wesselb
Copy link
Member

wesselb commented Mar 8, 2023

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?

@francesco-ballarin
Copy link
Contributor Author

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.

@wesselb
Copy link
Member

wesselb commented Mar 8, 2023

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!

@francesco-ballarin
Copy link
Contributor Author

Sure, no worries! Thanks :)

@wesselb
Copy link
Member

wesselb commented Apr 7, 2023

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.

@francesco-ballarin
Copy link
Contributor Author

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

Looks great @wesselb, thanks!

@wesselb
Copy link
Member

wesselb commented Apr 7, 2023

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

@wesselb
Copy link
Member

wesselb commented Apr 20, 2023

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
Copy link
Member

wesselb commented May 27, 2023

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

@pabloferz
Copy link

I'm still seeing rendering issues when using something like in @francesco-ballarin's #75 (comment). That is, when using in conjunction with the napoleon extension, sphinx struggles to show the rest of the methods.

@wesselb
Copy link
Member

wesselb commented Jun 6, 2023

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?

@francesco-ballarin
Copy link
Contributor Author

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

@machow
Copy link

machow commented Aug 30, 2023

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.

@wesselb
Copy link
Member

wesselb commented Sep 9, 2023

@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
Copy link
Member

wesselb commented Sep 9, 2023

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?

@machow
Copy link

machow commented Sep 11, 2023

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

@wesselb
Copy link
Member

wesselb commented Sep 20, 2023

@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
Copy link
Member

wesselb commented Sep 23, 2023

@machow Plum 2.2.2 incorporates PLUM_SIMPLE_DOC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants