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

Use SVG inheritance diagrams now that linking has been fixed #26567

Merged
merged 1 commit into from Mar 12, 2024

Conversation

ayshih
Copy link
Contributor

@ayshih ayshih commented Aug 21, 2023

PR summary

With sphinx-doc/sphinx#10614 and the to-be-merged sphinx-doc/sphinx#11634, links now work in SVG inheritance diagrams, including intersphinx links to external packages. This PR reverts PR #17913, which had switched the documentation to PNG inheritance diagrams.

PR checklist

@timhoffm
Copy link
Member

timhoffm commented Aug 21, 2023

Great to hear!

From currcent CI (e.g. https://output.circle-artifacts.com/output/job/49f53ea8-b87c-4ac6-9d04-9d6431a5a069/artifacts/0/doc/build/html/api/patches_api.html)

We definitively want to enforce sphinx >=7.2 with this, which means we should wait just a bit (7.2 is only 5 days old).

@QuLogic
Copy link
Member

QuLogic commented Aug 22, 2023

The links do not work in CI, but that may be a CI issue?

I don't think it's a CI issue; the SVG are placed in /_images and use links like ../html/_as_gen/matplotlib.patches.ConnectionPatch.html#matplotlib.patches.ConnectionPatch. This is entirely wrong; html is the base of build output directory and should not be part of the link.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 22, 2023

Hmm, these both appear to be bugs, so I'll drop this PR to draft for now.

The generated SVG files have incorrect scale factors, which appears to be a bug in graphviz. This line in your conf.py sets a gigantic maximum size of the graph, but the XML continues to include a non-unity scale factor, so essentially the graph gets additionally scaled on display.

I'll point out that you are currently installing graphviz via apt on Ubuntu, which is stuck on version 2.42.x despite being several years old. Version 2.42.x of graphviz has a known bug with the scaling factor – although that's not causing the issue here – so, for example, on sunpy we build the Read the Docs environment with graphviz installed via conda-forge (see https://github.com/sunpy/sunpy/blob/2e8c2de0de0ba855f626896ba0418f0b7e651cbf/.rtd-environment.yml#L8). Assuming that the above is a bug in graphviz, and it gets fixed, you probably won't get the bugfix unless you move away from installing graphviz via apt.

  • The links do not work in CI, but that may be a CI issue?

It looks like there is something particular to matplotlib's configuration that is tripping up generating the correct links, so sphinx might need a bug fix. I'll investigate.

@ayshih ayshih marked this pull request as draft August 22, 2023 02:45
@ayshih
Copy link
Contributor Author

ayshih commented Aug 22, 2023

Lest I come across as a crazy person, here's an example from the astropy documentation showing the SVG inheritance diagrams working with sphinx 7.2:
https://docs.astropy.org/en/latest/time/ref_api.html#class-inheritance-diagram

@ayshih
Copy link
Contributor Author

ayshih commented Aug 22, 2023

The links do not work in CI, but that may be a CI issue?

I don't think it's a CI issue; the SVG are placed in /_images and use links like ../html/_as_gen/matplotlib.patches.ConnectionPatch.html#matplotlib.patches.ConnectionPatch. This is entirely wrong; html is the base of build output directory and should not be part of the link.

Correct, something somewhere is getting confused about paths.

I'll note that when the links are fixed, it's possible that CircleCI will prevent being able to follow the links. The tokening for accessing artifacts seems rather aggressive, so may not like links from embedded SVG files. However, this is not something to worry about until the links are actually fixed.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 22, 2023

The dpi=100 that was added in #22273 leads graphviz to add the scaling factor, so I removed the dpi specification.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 22, 2023

  • The links do not work in CI, but that may be a CI issue?

I have posted a PR to sphinx (sphinx-doc/sphinx#11634) that fixes the bugs with the links. (I have built the docs locally to confirm.)

@timhoffm
Copy link
Member

Thanks for pushing the issues upstream to Sphinx! Let’s wait with this PR until Sphinx has released a fix.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 23, 2023

I added a (temporary) commit to install sphinx from my fork to verify that the bugfixes work, which would be reverted once sphinx-doc/sphinx#11634 is merged and released. For example, https://output.circle-artifacts.com/output/job/7ec86fa7-c90e-4edf-8ec4-7571950c07a4/artifacts/0/doc/build/html/api/patches_api.html. Unfortunately, as I feared, it looks like CircleCI doesn't let you access linked pages from the SVG even when they are correct, so you have to manually inspect the links to see that they will work.

@ayshih
Copy link
Contributor Author

ayshih commented Aug 31, 2023

sphinx-doc/sphinx#11634 is included in Sphinx 7.2.5, which was just released today. I've verified through building the docs locally that the inheritance-diagram links are correct, which can also be verified by inspecting the links in the CircleCI build (despite the fact that clicking on the links does not work).

@ayshih ayshih marked this pull request as ready for review August 31, 2023 03:09
@github-actions github-actions bot added the Documentation: build building the docs label Mar 10, 2024
@timhoffm
Copy link
Member

I've squashed and rebased on main. Let's see how this renders with current Sphinx.

@QuLogic
Copy link
Member

QuLogic commented Mar 11, 2024

Looks like you didn't rebase against a current main, as it doesn't have ninja installed.

@ayshih
Copy link
Contributor Author

ayshih commented Mar 12, 2024

Looks like this still works as intended to have SVG inheritance diagrams, e.g.:

https://output.circle-artifacts.com/output/job/85987d9b-2815-428a-bd67-8cae86e85de5/artifacts/0/doc/build/html/api/patches_api.html

As before, clicking links in the CircleCI build doesn't work, so links have to be manually inspected. For example, the Artist box in the SVG graph has the link:

https://circleci-tasks-prod.s3.us-east-1.amazonaws.com/forks/storage/artifacts/0e05288a-f55e-43b7-89a0-cc2c3c67bf8e/602836057/85987d9b-2815-428a-bd67-8cae86e85de5/0/doc/build/html/api/artist_api.html#matplotlib.artist.Artist

and the important part is that doc/build/html/api/artist_api.html#matplotlib.artist.Artist is correct.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

LGTM. Let's take this to our devdocs. We can always revert if we encounter issues.

@timhoffm timhoffm added this to the v3.9.0 milestone Mar 12, 2024
@timhoffm timhoffm merged commit 478dc5a into matplotlib:main Mar 12, 2024
33 of 34 checks passed
@ayshih ayshih deleted the svg branch March 14, 2024 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants