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

Fix ALL Sphinx warnings #393

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Conversation

melissawm
Copy link
Member

@melissawm melissawm commented Apr 6, 2024

References and relevant issues

Closes #111
Addresses #227 (will need a follow-up turning on sphinx -W option)

Description

This PR includes fixes to broken references and markdown syntax fixes. It also includes a new filter for napari.qt.threading module docstrings that were creating a few syntax warnings. These are not easily ignored by Sphinx and can't be fixed, as they are Markdown being injected into rst docstrings through the napari.qt module.

After this is merged, we will be able to make the build docs CI check to fail on new warnings, and new errors will be more visible.

@melissawm melissawm requested a review from psobolewskiPhD April 6, 2024 23:31
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 6, 2024
@melissawm melissawm requested a review from lucyleeow April 6, 2024 23:32
@@ -305,6 +308,38 @@ def add_google_calendar_secrets(app, docname, source):
if docname == 'community/meeting_schedule':
source[0] = source[0].replace('{API_KEY}', GOOGLE_CALENDAR_API_KEY)

class FilterSphinxWarnings(logging.Filter):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought you could filter warnings like this: https://github.com/sphinx-gallery/sphinx-gallery/pull/521/files

But I can't yet find documentation on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually yes - but this warning in particular you can't (or I couldn't!). See https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-suppress_warnings for a list of warnings that can be explicitly silenced. I think it falls into a similar category as sphinx-doc/sphinx#9750 - if you find a way to filter it, that would be great!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh man I didn't know any of this. Using warnings.filterwarnings in the conf.py file - how does this even work? I can't find documentation of this. Is this a Sphinx thing?

I guess in sphinx-gallery/sphinx-gallery#521 , the warning is passed on by the extension Sphinx-Gallery? In the Sphinx docs it says:

Then extensions can also define their own warning types.

Does that mean that extension warning types are also able to be suppressed?

Anyway, I had no idea, this seems reasonable way to fix it AFAICT! Thanks!

@Czaki Czaki added this to the 0.5.0 milestone Apr 7, 2024
@psobolewskiPhD
Copy link
Member

Looking at CircleCI output:
https://app.circleci.com/pipelines/github/melissawm/napari-docs/340/workflows/20e668a6-ad6f-4d21-bca7-31327ec1ed0a/jobs/328?invite=true#step-106-33254_29 Just 16 warnings! Down from >140!

Looks like a few are fixed by:
#388
and then a few more by:
napari/napari#6812

Copy link
Collaborator

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

Thank you!

@melissawm
Copy link
Member Author

@Czaki
Copy link
Contributor

Czaki commented Apr 8, 2024

This duplicated warning is signalig problem with cross references?

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Apr 8, 2024

Interesting this barks:
https://github.com/napari/docs/actions/runs/8600978497/job/23567119812?pr=393#step:9:663

WARNING: autodoc: failed to import function '_testsupport.make_napari_viewer' from module 'napari.utils'; the following exception was raised:
No module named 'pytest'

The relevant code, as far as I can tell:

#### `make_napari_viewer`
We provide a
[pytest fixture](https://docs.pytest.org/en/stable/explanation/fixtures.html) called
`make_napari_viewer` for tests that require the {class}`~napari.Viewer`. This
fixture is available globally and to all tests in the same environment that `napari`
is in (see [](test-organization) for details). Thus, there is no need to import it,
you simply include `make_napari_viewer` as a test function parameter, as shown in the
**Examples** section below:
```{eval-rst}
.. autofunction:: napari.utils._testsupport.make_napari_viewer()
```

And on napari.org/dev/ it's broken:
https://napari.org/dev/developers/contributing/testing.html#make-napari-viewer
But with this PR it's working!
https://output.circle-artifacts.com/output/job/54961a00-f23c-4027-bc52-acec2f1e3e84/artifacts/0/docs/docs/_build/html/developers/contributing/testing.html#make-napari-viewer

@melissawm
Copy link
Member Author

melissawm commented Apr 8, 2024

This duplicated warning is signalig problem with cross references?

Not really. As far as I can tell, this happens because some Attributes and Properties have the same name, which confuses sphinx (which doesn't deal with Attributes very well). In addition, in some cases when you have subclasses that inherit the parent's docstrings this can also happen (since both objects will be documented with the same parameters/attributes). But I will confess that as much as I've investigated this, it seems like nobody really knows what's going on exactly, including me 😅 (see, for example, sphinx-doc/sphinx#7817 which is the closest I could find)

We could potentially fix this without ignoring the errors, but that would mean having multiple templates for different classes which would make the docs generation less automatic and more manual. Since these warnings do not impact the rendering of the docs, I think they are safe to ignore.

@melissawm
Copy link
Member Author

Interesting this barks: napari/docs/actions/runs/8600978497/job/23567119812?pr=393#step:9:663

WARNING: autodoc: failed to import function '_testsupport.make_napari_viewer' from module 'napari.utils'; the following exception was raised:
No module named 'pytest'

The relevant code, as far as I can tell:

#### `make_napari_viewer`
We provide a
[pytest fixture](https://docs.pytest.org/en/stable/explanation/fixtures.html) called
`make_napari_viewer` for tests that require the {class}`~napari.Viewer`. This
fixture is available globally and to all tests in the same environment that `napari`
is in (see [](test-organization) for details). Thus, there is no need to import it,
you simply include `make_napari_viewer` as a test function parameter, as shown in the
**Examples** section below:
```{eval-rst}
.. autofunction:: napari.utils._testsupport.make_napari_viewer()
```

And on napari.org/dev/ it's broken:
napari.org/dev/developers/contributing/testing.html#make-napari-viewer
But with this PR it's working!
output.circle-artifacts.com/output/job/54961a00-f23c-4027-bc52-acec2f1e3e84/artifacts/0/docs/docs/_build/html/developers/contributing/testing.html#make-napari-viewer

I think we'd have to include pytest as a dependency for building the docs for this to work without this warning? Not sure why I didn't see this locally...

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Apr 8, 2024

I think we'd have to include pytest as a dependency for building the docs for this to work without this warning? Not sure why I didn't see this locally...

Local build instructions use dev install of napari, so you probably have it.
Edit: derp, so does circleCI workflow:

python -m pip install -e "napari/[pyside,dev]"

🤷

@lucyleeow
Copy link
Collaborator

is happens because some Attributes and Properties have the same name, which confuses sphinx (which doesn't deal with Attributes very well). In addition, in some cases when you have subclasses that inherit the parent's docstrings this can also happen

Maybe we could add this info in the FilterSphinxWarnings docstring?

This PR includes fixes to broken references and markdown syntax fixes.
It also includes a new filter for napari.qt.threading module docstrings
that were creating a few syntax warnings. These are not easily ignored
by Sphinx and can't be fixed, as they are Markdown being injected into
rst docstrings through the napari.qt module.
@melissawm
Copy link
Member Author

@lucyleeow let me know if this is enough or if I should elaborate!

Copy link
Collaborator

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

everything looks good to me -- and it's working!
Thanks!!

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Apr 12, 2024

SO I don't get the lxml warning:
https://github.com/napari/docs/actions/runs/8647550683/job/23709258930?pr=393#step:9:120

I tested locally and I also get it, but yet both lxml and lxml_html_clean are installed.

Why is it referring to html-clean? it's [html_clean] in the requirements.txt which matches their setup.py
It is a wierd setup.py, so maybe we should just use lxml_html_clean directly?

@melissawm
Copy link
Member Author

I don't get this warning locally - but it looks like changing that requirement did it!

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

Everything looks good, so this is great, will make real warnings be so much more obvious now.

@lucyleeow
Copy link
Collaborator

ping @psobolewskiPhD could this go in? 😬

@psobolewskiPhD
Copy link
Member

Yeah sorry; i'm terrible at circling back to merge things.

@psobolewskiPhD psobolewskiPhD merged commit c9e562d into napari:main Apr 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remaining Sphinx warnings
4 participants