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.

Sign up for GitHub

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

Correctly handle case where "See Also" section follows "Examples" in global_enable_try_examples #251

Merged
merged 7 commits into from
Jan 13, 2025
Merged

Correctly handle case where "See Also" section follows "Examples" in global_enable_try_examples #251

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f24cd2d
Add `..seealso` section as a pattern
agriyakhetarpal Jan 10, 2025
10abc82
Fix escaping of regex characters
agriyakhetarpal Jan 11, 2025
5f187f3
Reword comment about SymPy's special case
agriyakhetarpal Jan 12, 2025
79e864e
Add subset of docutils + Sphinx directives to end at
agriyakhetarpal Jan 13, 2025
08e70bd
Revert "Add subset of docutils + Sphinx directives to end at"
agriyakhetarpal Jan 13, 2025
5397180
Reword comment about "See Also" section
agriyakhetarpal Jan 13, 2025
631dfeb
Remove references to historical PRs
agriyakhetarpal Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion jupyterlite_sphinx/_try_examples.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,17 @@ def _process_literal_blocks(md_text):
_next_section_pattern = re.compile(
"|".join(
[
rf".. (rubric|admonition)::\s*{header}"
rf"\.\. (rubric|admonition)::\s*{header}"
for header in _non_example_docstring_section_headers
]
# If examples section is last, processed by numpydoc may appear at end.
+ [r"\!\! processed by numpydoc \!\!"]
# Attributes section sometimes has no directive.
+ [r":Attributes:"]
# The See Also section shows up for old numpydoc versions such as that
# of SymPy where it is accounted for as an admonition and not a rubric,
# therefore we check for it as a special case for now.
Copy link
Collaborator

@steppi steppi Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is again not quite right. If you check line 311, it also checks for admonitions. The regex rf"\.\. (rubric|admonition)::\s*{header} will match .. followed by one of rubric or admonition, followed by any number of whitespace, followed by one of the headers in _non_example_docstring_headers.

But in at least SymPy, the See Also section doesn't get a directive like .. admonition:: See Also but just .. seealso:. What I'm curious about is, for current versions of numpydocs, does a See Also section get the directive .. admonition:: See Also like I assumed, or was I incorrect about this?

Since the above logic does check for See Also sections, it can't be relevant that the See Also section shows up for old numpydoc versions.

Suggested change
# The See Also section shows up for old numpydoc versions such as that
# of SymPy where it is accounted for as an admonition and not a rubric,
# therefore we check for it as a special case for now.
# SymPy uses a custom numpydoc version which has a different pattern for
# the generated directives. Rather than investigating further, take advantage
# of See Also being the only section appearing after Examples in SymPy and
# hardcode the directive that appears in SymPy. This can be removed after
# SymPy is updated to use regular numpydoc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point someone should sit down and figure out what the generated directives can actually look like for each section header in mainline numpydoc.

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I spent some time debugging this and while I couldn't get a definite answer about what happens where in the code, here's what I have realised –

There are two things at play here:

  1. There is a a .. seealso:: directive that's native to Sphinx, and a "See Also" section that upstream numpydoc can interpret and convert to a rubric/admonition, which we correctly parse and escape with TryExamples' _next_section_pattern if we retain compatibility with upstream numpydoc.
  2. However, SymPy's numpydoc is dated and can't do this for SymPy's documentation, and thus we don't get to catch it.

Therefore, a "See Also" section in the documentation for SciPy for an example, will caught in numpydoc, and this problem isn't specific to just .. seealso::, all native Sphinx-based directives won't get caught. It's just that this hasn't been reported as a bug before because of the following reasons:

A. most projects don't put anything after "Examples", and only SymPy that we know of (so far) has done so
B. Upstream numpydoc does indeed reorder the sections to some extent (which was my original comment a few commits ago, so I was right). Even if a project does add a section after "Examples", it gets caught by the pattern or re-ordered before it's rendered in the HTML.

As a confirmation for point A, I tried another Sphinx directive from the page I linked above, such as .. versionadded::, and here we go:

SymPy documentation screenshot with erroneous versionadded directive
image

The same experiment with NumPy confirms point B. For example, the docstring below:

NumPy's exceptions.AxisError docstring (modified) 


"""
    Examples
    --------
    >>> import numpy as np
    >>> array_1d = np.arange(10)
    >>> np.cumsum(array_1d, axis=1)
    Traceback (most recent call last):
      ...
    numpy.exceptions.AxisError: axis 1 is out of bounds for array of dimension 1

    Negative axes are preserved:

    >>> np.cumsum(array_1d, axis=-2)
    Traceback (most recent call last):
      ...
    numpy.exceptions.AxisError: axis -2 is out of bounds for array of dimension 1

    The class constructor generally takes the axis and arrays'
    dimensionality as arguments:

    >>> print(np.exceptions.AxisError(2, 1, msg_prefix='error'))
    error: axis 2 is out of bounds for array of dimension 1

    Alternatively, a custom exception message can be passed:

    >>> print(np.exceptions.AxisError('Custom error message'))
    Custom error message

    See Also
    --------

    numpy.core._exceptions._AxisError : The internal version of this exception.

    Attributes
    ----------
    axis : int, optional
        The out of bounds axis or ``None`` if a custom exception
        message was provided. This should be the axis as passed by
        the user, before any normalization to resolve negative indices.

        .. versionadded:: 1.22
    ndim : int, optional
        The number of array dimensions or ``None`` if a custom exception
        message was provided.

        .. versionadded:: 1.22
"""

where the "See Also" section is after the "Examples" section – where we could assume that it would be ordered in the same manner. However, generating the NumPy docs, it shows up before the examples:

Screenshot 1 from NumPy documentation

image

But, if you were to add the .. versionadded:: directive or similar ones for NumPy, they are not reordered. I added a .. versionchanged:: Agriya debugging directive after the Examples section, and this is what we get:

NumPy documentation screenshots 2 and 3 with a native Sphinx-based directive after the Examples section


The HTML

image

and the interactive notebook

image

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We'll need to add almost all of the directives listed under https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#paragraph-level-markup to be able to restrict them from percolating into the notebooks; I don't think it makes sense to leave it with just .. seealso especially if others like ..versionadded are also included with both SymPy's numpydoc and the upstream numpydoc.

I used this command

from docutils.parsers.rst import directives
all_directives = list(directives._directive_registry.keys())

to get a list:

['attention',
 'caution',
 'code',
 'danger',
 'error',
 'important',
 'note',
 'tip',
 'hint',
 'warning',
 'admonition',
 'sidebar',
 'topic',
 'line-block',
 'parsed-literal',
 'math',
 'rubric',
 'epigraph',
 'highlights',
 'pull-quote',
 'compound',
 'container',
 'table',
 'csv-table',
 'list-table',
 'image',
 'figure',
 'contents',
 'sectnum',
 'header',
 'footer',
 'target-notes',
 'meta',
 'raw',
 'include',
 'replace',
 'unicode',
 'class',
 'role',
 'default-role',
 'title',
 'date',
 'restructuredtext-test-directive']

and we know some Sphinx-specific directives (outside docutils) manually that aren't present in this list:

['seealso',
 'versionadded',
 'versionchanged',
 'deprecated',
 'versionremoved',
 'index'
]

Only a subset of them will need to be added to the regex pattern, I assume?

(Edit: added a subset, please keep reading below)

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I remembered this numpydoc PR from last year that was xref-ed from the NumPy PR where I had reported some incorrect ordering of the Attributes section, as it was showing up after the Examples section. It sheds some light on how the sections are ordered/reordered: numpy/numpydoc#571.

The order preservation is mandated here: https://github.com/numpy/numpydoc/blob/6d5eb428b8acea6ced4d86d086e36daf3f03ffbf/numpydoc/validate.py#L28-L41, and via an error message in https://github.com/numpy/numpydoc/blob/6d5eb428b8acea6ced4d86d086e36daf3f03ffbf/numpydoc/validate.py#L61. We know that dictionaries are ordered in Python post 3.7.

Comparing the relevant code with SymPy's numpydoc: https://github.com/sympy/sympy/blob/3c33a8283cd02e2e012b57435bd6212ed915a0bc/doc/ext/docscrape_sphinx.py#L208-L240, not only are the headings and sections different, but also that upstream numpydoc has a structural templating mechanism thingy where an order is enforced, and SymPy's version explicitly calls _str_see_also at the point where it wants the section to appear.

So, to answer your question (quoting it below) with all I've mentioned:

But in at least SymPy, the See Also section doesn't get a directive like .. admonition:: See Also but just .. seealso:. What I'm curious about is, for current versions of numpydocs, does a See Also section get the directive .. admonition:: See Also like I assumed, or was I incorrect about this?

Since the above logic does check for See Also sections, it can't be relevant that the See Also section shows up for old numpydoc versions.

If upstream numpydoc had not enforced the order, we would have also faced this SymPy-specific problem in NumPy. It is about the ordering of the sections rather than what the sections are themselves (whether Sphinx directives or named sections in numpydoc).

Upstream numpydoc also has https://github.com/numpy/numpydoc/blob/6d5eb428b8acea6ced4d86d086e36daf3f03ffbf/numpydoc/validate.py#L24-L27 to run checks version-changed directives like the above, but it does not enforce an order for them as they are not a part of standard sections but are directives.

So, I think we should indeed add at least a few of these directives to the patterns we need to ignore – maybe just the most commonly used ones from the above list will work. This would help in both getting SymPy to play nicely, and in the scenario that some docstrings are incorrectly added to any other projects that use the modern numpydoc, we don't let that mistake in with the interactive examples.

Copy link
Member Author

@agriyakhetarpal agriyakhetarpal Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

At the same time, if someone were to put a directive in the middle of a doctest, that could cause a bug report for us because the doctest would end midway... but I feel we're technically correct and we could say "works as intended" and close it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a minimal set of directives to halt at in commit 79e864e and this shouldn't cause any problematic behaviour with most websites, while also getting us working with SymPy.

Copy link
Collaborator

@steppi steppi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm still confused. The documentation for TryExamples says this

image

Can you end up with a raw Sphinx heading directive inside of the processed docstring when using numpydoc or sphinx.ext.napoleon when using them properly?

I still think if the current implementation only works because of the enforced ordering, then it's because it was only working incidentally because of the enforced ordering, because the code is trying to catch all possible next section directive patterns for numpydoc.

+ [r"\.\. seealso::"]
)
)

Expand Down
Loading