Resolve intersphinx to astropy as current build #11690

nstarman · 2021-05-04T01:16:22Z

This enables astropy to use astropy intersphinx links and correctly resolve them to the version being built.
This is implemented in SkyCoord so that links to the docs works in affiliate packages.

Part of #11555
Request Review: @mhvk @adrn @pllim @embray

Signed-off-by: Nathaniel Starkman (@nstarman) nstarkman@protonmail.com

github-actions · 2021-05-04T01:17:06Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

nstarman · 2021-05-04T01:38:10Z

Success! the links in SkyCoord are still live and refer to the current build.

docs/conf.py

pllim · 2021-05-04T14:14:35Z

astropy/coordinates/sky_coordinate.py

@@ -1035,7 +1035,7 @@ def separation(self, other):
            not give the same answer in this case.

        For more on how to use this (and related) functionality, see the
-        examples in :doc:`/coordinates/matchsep`.
+        examples in :doc:`astropy:/coordinates/matchsep`.


I haven't been following the various conversation on Sphinx xref and just by looking at this diff, I don't understand why this is needed in the core repo. Why not use the built-in Sphinx :ref: again? What is an example use case where this would benefit affiliated package? Thanks!

Why not use the built-in Sphinx :ref: again?

The link is to a :doc:, not a specific section, so :ref: can't be used. Perhaps the link could be changed entirely, but for page references :doc: is the Sphinx built-in.

What is an example use case where this would benefit affiliated package?

In this case, for subclassing SkyCoord. Both :doc: and :ref: are not inter-sphinxed automatically, so any link in the SkyCoord subclass can break when building the docs (if warnings -> errors). This PR enables astropy to set up internal links as if they were intersphinx links, but have them be interpreted as internal links. Affiliate packages would see these links as intersphinx links. This is quite similar to the astropy-dev label, where Astropy sees it as internal (if it's the dev build), while affiliate packages see it as an intersphinx.

I hope this PR will enable astropy to provide more links in docstrings and affiliate packages won't have to worry about a thing.

I cannot find any documentation of :doc: in the official Sphinx doc (which is kinda ironic), can you please link me to its doc? Thanks!

As for :ref: intersphinx, I haven't seen a problem downstream where I use ~astropy.coordinates.SkyCoord and so on. I did see a problem when the doc here refers to simply SkyCoord but the fix is to mention the full namespace in core API doc.

As for referring to page vs section, I don't see the point of the former. Why not have a :ref: at the top of the page and then use :ref:?

Sorry if all these had been discussed before. If so, please point me to the old discussions. Thanks for your patience!

Actually, I do remember discussion quite a while ago that docstrings should use proper http links so that people who see them in an interactive session can actually follow them....

But in that respect the scheme with :doc: is better than a :ref: - with that, the user truly has no hope to find this except by going to the docstring online. Also, the :doc:astrop seems a nice clear marker. Plus, this scheme has the advantage over an http link that we'll notice breakage sooner.

One question: is the / in front of /coordinates necessary? Or does it try to find the link in the local environment of the SkyCoord docstring rendering otherwise?

Thanks for the explanation. It makes sense now. The whole sphinx stuff remains a bit of black magic to me... Not sure that expanding the changelog will be needed; if anything, we'd need some docs for affiliate package maintainers, but not clear how they would find that... So, let's just stick with fixing a possible bug with subclassing SkyCoord here.

Allowing a project to refer to itself via intersphinx seems generically useful for protecting from docstring inheritance. I wish sphinx did this automatically.

sphinx-doc/sphinx#9169

Ah, I think I get it now. I was thinking about :ref: in docstring pointing to another API, but here you are talking about human doc.

Is this PR standalone or it depends on the other PR you opened over at sphinx-astropy?

This PR is standalone in that it works and does the docs stuff independently.

It's definitely part of a larger push to make intersphinx and particularly xref useful for downstream packages. In some respects, this PR is the foundation on which future PRs can be built since all the downstream packages need astropy xref links to be intersphinxed, but without this PR, those links wouldn't work inside of Astropy. Now sphinx-astropy will work equally well for both Astropy and downstream packages.

mhvk

I think this looks all good, now I understand it. Perhaps still worth elaborating a bit in the changelog entry? Though not a blocker, so feel free to ignore.

docs/changes/coordinates/11690.other.rst

mhvk · 2021-05-05T00:32:00Z

@pllim - what do you think, does it make sense to do this?

pllim

Some nitpicks on the change log but I'll leave it to your judgement which suggested changes make sense to you.

Probably also want to add a comment on the affected code about the Sphinx issue you raised. If this is implemented upstream, then we can remove it in the future.

I would still like Madison to have a look as they implemented the original function you are adding on to.

Thanks!

docs/changes/coordinates/11690.other.rst

nstarman · 2021-05-05T17:49:03Z

Probably also want to add a comment on the affected code about the Sphinx issue you raised. If this is implemented upstream, then we can remove it in the future.

Agreed. I've added a little note on lines 375-377

I would still like Madison to have a look as they implemented the original function you are adding on to.

👍

embray · 2021-05-17T18:32:52Z

How many other places in the astropy docs use :doc: and is there a reason we couldn't just replace them with normal :ref: links?

IIUC though that wouldn't solve the problem you're trying to solve anyways.

But then why single out SkyCoord? In principle there are tons of other classes containing docstrings that link to other pages in the astropy docs. And this new special feature isn't really documented anywhere, so how would remind developers writing new docstrings that it should be used?

I'm 👍 to solving this problem, and don't have any objections to this approach. But ISTM like a deeper problem related to Sphinx...

mhvk · 2021-05-17T18:47:22Z

Agreed on a more general solution. Though there may not be that many other instances: I think before we really tried hard to avoid having sphinx links in docstrings, since those are not useful when doing interactive work; although hard-coded URLs are also far from ideal....

embray · 2021-05-17T18:52:28Z

I have in the past toyed with the idea of having a packaging hook that would go through and render all sphinx links in docstrings to URLs. This mapping would in turn be used by an extension to IPython's help system to replace the links.

I have a little utility function called desphinxify which strips some other Sphinx markup from docstrings and makes them look a bit nicer, but it doesn't handle links yet.

nstarman · 2021-05-17T21:27:22Z

How many other places in the astropy docs use :doc: and is there a reason we couldn't just replace them with normal :ref: links?
IIUC though that wouldn't solve the problem you're trying to solve anyways.

Unfortunately, no that wouldn't solve this problem. Wish it did.

But then why single out SkyCoord? In principle there are tons of other classes containing docstrings that link to other pages in the astropy docs.

Necessity as the mother of invention. I just first encountered this in SkyCoord. Doing a search for :doc:, SkyCoord is currently the only docstring that has an astropy doc link. There are a number of :ref: links to specific sections. I'll attend to those shortly.

And this new special feature isn't really documented anywhere, so how would remind developers writing new docstrings that it should be used?

Hmm. good point. I'll add something to the developer docs.

I'm 👍 to solving this problem, and don't have any objections to this approach. But ISTM like a deeper problem related to Sphinx...

Agreed that an upstream fix would be nice. I've raised an issue at sphinx-doc/sphinx#9169. If that is implemented then the docstring changes here would be kept the same and only the intersphinx override magic in conf.py will be deprecated.

nstarman · 2021-05-17T22:00:13Z

Ok. @embray I think I got all :ref: in the docstrings and made them "intersphinx". I didn't do the ones in the docs as those cannot be inherited. Likewise, I added to the dev docs saying that all links in the docstrings should be made intersphinxed, including Astropy's. It would be nice to have some test for this, but IDK how one would test in the core package a change intended for affiliate packages.
@mhvk, this is a somewhat large diff from what you approved, so I'm re-requesting a review.

@nstarman

This enables astropy to use astropy intersphinx links and correctly resolve them to the version being built. Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@pllim

Co-authored by @pllim Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

mhvk

@nstarman - I think the case has gotten stronger, and thanks for sticking with it! Only one real comment, about the location of the new instructions.

mhvk · 2021-05-18T20:43:30Z

docs/changes/coordinates/11690.other.rst

@@ -0,0 +1,3 @@
+Sphinx RST links in ``SkyCoord`` now use intersphinx for downstream packages' convenience.


Isn't it all through astropy now? Or is SkyCoord still the only thing that could be subclassed and thus be wrong?

yes, it's now all over astropy. I'm moving the note up to the top level. I've contributed a number of such fragments, all about xref, docstrings, and the documentation. Perhaps there should be a specific category / folder for such changelog fragmets, e.g. documentation/. @saimn how feasible is this with towncrier?

It's easy to do but until now the policy has been that we do not mention documentation changes. Exceptions are possible such as for big changes like you did but I'm not sure that justifies a new category. "other" seems fine to me. @pllim @bsipocz - Thoughts ?

I don't think this would need a changelog to begin with. And as a rule of thumb, the changelog doesn't have any links. The links tend to broke on the many year timescale, and while SkyCoord is expected not to be one of those, the blanket rule of no links works better than trying to maintain a list of OK and not OK types of links.

mhvk · 2021-05-18T20:48:32Z

docs/development/docguide.rst

+  + cross-reference links (``:ref:``, ``:doc:``, etc.), **including internal
+    Astropy links**, should use the `intersphinx format
+    <https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html>`_.
+    Note that in Astropy these internal links are to the current build, while


I think this is not super helpful, maybe add an example? Also, I thought this was required only for the docstrings, not for "all documentation" (which the bullet is about), so I think you need it to one of the following bullet points (maybe the numpydoc one? Or just its own bullet point "References in docstrings ..."

Done! and noted 'astropy-dev' links to the dev version of astropy.

examples/coordinates/plot_sgr-coordinate-frame.py

nstarman · 2021-05-18T22:28:45Z

Agreed on a more general solution. Though there may not be that many other instances: I think before we really tried hard to avoid having sphinx links in docstrings, since those are not useful when doing interactive work; although hard-coded URLs are also far from ideal....

@mhvk Agreed on both points.
I do think that sphinx links in docstrings can be useful for providing a note on where to find further reading and examples. Those don't detract from looking at the docstring in an interactive environment. I like @embray's idea of hot swapping for URLs. If this were done on compile so the PIP version had URLs, Jupyter Lab makes the links live!

mhvk

OK, looks all good now. Thanks!

Though I'm not sure what's wrong with readthedocs; did the docs pass locally and did you check the new addition in the dev docs?

Anyway, approving under the assumption that's a false positive (I had a failure recently too that I did not understand), and that the changelog fragment is located OK.

nstarman · 2021-05-19T05:14:30Z

Yes to the above. Let me check again the RTD is unrelated.

pllim · 2021-05-19T13:32:49Z

RTD warnings might be related. I don't see this in main.

WARNING: :533: (WARNING/2) Inline literal start-string without end-string.
...
<unknown>:533: WARNING: undefined label: link
<unknown>:533: WARNING: undefined label: astropy:link

nstarman · 2021-05-19T14:42:12Z

Thanks @pllim. It was the example code I added in the news fragment! I edited that after doing a local build. But I put the links in an in-line code block, so I didn't think they would get picked up.

@nstarman

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

pllim · 2021-05-19T16:08:17Z

RTD is green now but I would still like @embray to approve if they are okay with this.

embray

Having to put astropy: everywhere is quite a mess, and I think people (by which I mean me) will definitely forget to do this. But it's also exactly what I suggested you do, so I can't complain ;)

It's an unfortunate Sphinx limitation that doesn't come up very often, but for something like Astropy it is a real problem. Thanks for opening the issue against Sphinx.

embray · 2021-05-21T18:04:29Z

astropy/coordinates/builtin_frames/equatorial.py

@@ -64,7 +64,7 @@ class TETE(BaseRADecFrame):
    with local apparent sidereal time to calculate the apparent (TIRS) hour angle.

    For more background on TETE, see the references provided in the
-    :ref:`astropy-coordinates-seealso` section of the documentation.
+    :ref:`astropy:astropy-coordinates-seealso` section of the documentation.


These astropy:astropy references are unfortunate. I won't ask you to change them, but I wonder why those labels contained "astropy-" in the first place. Seems redundant. We can deal with that in a follow-up, if at all.

Agreed. I didn't like those either. And they're now doubly redundant.

github-actions bot added coordinates Docs labels May 4, 2021

nstarman marked this pull request as ready for review May 4, 2021 01:38

nstarman commented May 4, 2021

View reviewed changes

docs/conf.py Outdated Show resolved Hide resolved

nstarman force-pushed the coord_SkyCoord_links branch from a6028b2 to b16d3aa Compare May 4, 2021 01:57

This was referenced May 4, 2021

Use sphinx-astropy xref aliases #11684

Merged

Meta Issue: ABCD (Always be Code Documenting) followups to X-Ref PR #11118 #11555

Open

pllim reviewed May 4, 2021

View reviewed changes

pllim added this to the v5.0 milestone May 4, 2021

nstarman mentioned this pull request May 4, 2021

intersphinx xref astropy/sphinx-astropy#40

Merged

mhvk approved these changes May 4, 2021

View reviewed changes

docs/changes/coordinates/11690.other.rst Outdated Show resolved Hide resolved

nstarman force-pushed the coord_SkyCoord_links branch 2 times, most recently from 14896cc to 5cb496c Compare May 4, 2021 21:38

nstarman mentioned this pull request May 4, 2021

Resolve intersphinx to own project as current build sphinx-doc/sphinx#9169

Open

pllim reviewed May 5, 2021

View reviewed changes

docs/changes/coordinates/11690.other.rst Outdated Show resolved Hide resolved

docs/changes/coordinates/11690.other.rst Outdated Show resolved Hide resolved

docs/changes/coordinates/11690.other.rst Outdated Show resolved Hide resolved

pllim requested a review from embray May 5, 2021 14:13

pllim added the Enhancement label May 5, 2021

nstarman and others added 4 commits May 17, 2021 18:01

Resolve intersphinx to astropy as current build

dc46993

This enables astropy to use astropy intersphinx links and correctly resolve them to the version being built. Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

Apply suggestions from code review

81aa1e7

Co-authored by @pllim Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

note potential upstream fix

6e2d101

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

localize intersphinx :ref: links

81f2f66

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

dev docs guide

f7d2f81

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the coord_SkyCoord_links branch from 979ae64 to f7d2f81 Compare May 17, 2021 22:01

nstarman requested a review from mhvk May 17, 2021 22:01

mhvk reviewed May 18, 2021

View reviewed changes

mhvk approved these changes May 19, 2021

View reviewed changes

change documentation to reflect all code

470d1ba

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>

nstarman force-pushed the coord_SkyCoord_links branch from 7fa7ed2 to 470d1ba Compare May 19, 2021 15:45

embray approved these changes May 21, 2021

View reviewed changes

embray merged commit e183e39 into astropy:main May 21, 2021

nstarman deleted the coord_SkyCoord_links branch May 21, 2021 18:52

nstarman mentioned this pull request May 21, 2021

change RST links to get ride of redundant “astropy” in links #11769

Closed

embray mentioned this pull request May 21, 2021

Updating spinner API, keeping enum #11046

Merged

		@@ -0,0 +1,3 @@
		Sphinx RST links in ``SkyCoord`` now use intersphinx for downstream packages' convenience.

Resolve intersphinx to astropy as current build #11690

Resolve intersphinx to astropy as current build #11690

Conversation

nstarman commented May 4, 2021 • edited

github-actions bot commented May 4, 2021

nstarman commented May 4, 2021

Choose a reason for hiding this comment

nstarman May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

mhvk commented May 5, 2021

pllim left a comment

Choose a reason for hiding this comment

nstarman commented May 5, 2021

embray commented May 17, 2021

mhvk commented May 17, 2021

embray commented May 17, 2021

nstarman commented May 17, 2021

nstarman commented May 17, 2021

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstarman commented May 18, 2021 • edited

mhvk left a comment

Choose a reason for hiding this comment

nstarman commented May 19, 2021

pllim commented May 19, 2021

nstarman commented May 19, 2021 • edited

pllim commented May 19, 2021

embray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nstarman commented May 4, 2021 •

edited

nstarman May 4, 2021 •

edited

nstarman commented May 18, 2021 •

edited

nstarman commented May 19, 2021 •

edited