Sphinx + Type Hint difficulties

[nstarman]

The Problem:

This issue arose in #15852, where it was discovered that adding type hints to files can result in Sphinx erroneously believing the private source of the type hint, rather than where it is actually made public, should be included in the docs. The result is that Sphinx has too many locations for the object and raises a warning that there are multiple references.
These look like
WARNING: more than one target found for cross-reference 'Equivalency': astropy.units.Equivalency, astropy.units.equivalencies.Equivalency.

The currently-adopted way to fix this is to add :noindex: to documentation modules in the docs, e.g. docs/units/ref_api.rst.

How do we fix this?

1. It would be nice if Sphinx itself could fix this. Type hints are irregularly documented (+link generation broken) sphinx-doc/sphinx#9813 might fix the issue. We should check if it is ever resolved by a PR. This would require the least work from us and would allow us to punt other issues to future maintainers. They'll thank us, I'm sure.

2. We modify automodapi so that we can sort and section according to files / custom specification. Then we need never call automodapi on a private file and confuse Sphinx into thinking it's public. Maybe this will work. I'm not entirely sure. From work on other repos I am sure of option 3. Even if it doesn't work, this could be a nice feature to have!

3. We fix it ourselves. The issue only arises when we have an automodapi (or similar documentation directive) for a private file that is named as if it's a public file, e.g. astropy.units.quantity.py. Sphinx has no good way to know that this is actually private. Sphinx will prefer astropy.units but this is a user-friendly heuristic only. When the file astropy.units.quantity.py is renamed to astropy.units._quantity.py then this whole issue is resolved as Sphinx will only find astropy.units to be public. We could fix this issue and avoid all similar issues by adhering to PEP8 standards in file naming.

[pllim]

The upstream issue been open since 2021... 👀 but yeah would be nice to fix it at the source.

Renaming files just for Sphinx to work with type hints, with no other benefits seems excessive.

[nstarman]

Renaming files just for Sphinx to work with type hints, with no other benefits seems excessive.

It dovetails with the larger discussion of astropy/astropy-APEs#85 for how tools build to standards, standards evolve according to tools, and we can benefit from moving to inside this development cycle.

🐦🐦🐦🐦+🪨 = 🧑‍🍳

[eerovaher]

Renaming files just for Sphinx to work with type hints, with no other benefits seems excessive.

What is excessive is the amount of wrestling with Sphinx that adding type annotations requires.

[nstarman]

What is excessive is the amount of wrestling with Sphinx that adding type annotations requires.

Especially since other projects don't encounter these difficulties.

[pllim]

Especially since other projects don't encounter these difficulties.

That is a not a fair comparison. I doubt all projects are fully annotated.

[nstarman]

Only ones that are typed are needed for this to be an informative comparison. Those that aren't typed aren't comparable to our current situation. I work with (and have written) a number of projects that are. And they don't have this hard-to-diagnose problem.

It would be fantastic if there's a simple solution we could implement to make Sphinx happy. Perhaps if we added functionality to automodapi to add custom sections and sorting of top-level namespaces then we wouldn't need to call automodapi on private modules (which sphinx doesn't know are private so ends up building multiple references), which may be /hopefully(?) is the major issue driving our problems. I added this to the top comment. @mhvk and I discussed something about this in another Issue, though I don't recollect where...

[pllim]

Just focusing on renaming quantity.py to _quantity.py here: That is going to break everything that mentioned astropy.units.quantity.somethingsomething downstream. It is too scary to think of the consequences. Why don't we try to prod Sphinx devs to see if they can fix it upstream first before doing drastic refactoring on our end?

[nstarman]

That is going to break everything that mentioned astropy.units.quantity.somethingsomething downstream.

Yes, but this is also what we mean by private API, which astropy.units.quantity.somethingsomething is (meant to be), while astropy.units.somethingsomething is not. People shouldn't be using astropy.units.quantity.somethingsomething over astropy.units.somethingsomething. If they are, that's a problem. Maybe one we could fix by an APE...

It is too scary to think of the consequences.

This is exactly the constellation of problems astropy/astropy-APEs#85 is aimed at. If changing private API is too scary because it problematically appears to be public and people are using it we have a major problem for future maintainers! Improving and refactoring code is a necessity. Maintaining a public API MUST be separate from private implementation details.

Why don't we try to prod Sphinx devs to see if they can fix it upstream first before doing drastic refactoring on our end?

Sure. That would be nice! And a great short / medium term solution. 👍
IMO this is like driving around big potholes (I'm a US Midwesterner where we have champion size potholes). It get's you there. And until the road is fixed it's a workable option. I want to do both: get where I'm going and fix the roads so it's easier to get there again.

But not focusing on fixing deeper structural issues for a sec. I do think we should pursue options 1 & 2! If we can get something working, that's good. If we decide to invest in the future with option 3, it'll take a while and we need 1 or 2 in the meantime.

[mhvk]

From a user perspective, we really do not need to show the API for the the private modules - so maybe just refactoring that and only show ones that are actually used (i.e., the ones that contain units) would be good enough?

I really do not want to go the renaming route just because some piece of software that we can make PRs too is mistaken otherwise if one starts using classes in typing -- without typing, there is no issue.

[nstarman]

From a user perspective, we really do not need to show the API for the the private modules - so maybe just refactoring that and only show ones that are actually used (i.e., the ones that contain units) would be good enough?

👍

I really do not want to go the renaming route just because some piece of software that we can make PRs too is mistaken otherwise if one starts using classes in typing -- without typing, there is no issue.

astropy/astropy-APEs#85 highlights other reasons. But I think for this Issue the relevant point is we all agree that private APIs shouldn't be used downstream and, though we want to minimize downstream friction, if they are using private APIs some friction is inevitable. Disagreements on astropy/astropy-APEs#85 stem from whether the fix should be de jure or de facto. Regardless, for this Issue we might be able to fix the RTD / Sphinx problems in our docs by not referencing private modules ourselves!

[mhvk]

@nstarman - getting back to the sphinx problems: would removing modules like quantity from the docs resolve the sphinx issues? I ask since :noindex: was not enough, but actually not having them at all might be different. If so, I'm quite happy to minimize the modules listed.

I guess the actual modules that would need to be "de-listed" are quantity and equivalencies, right? I think that's actually fine (the list of functions in units itself is mostly equivalencies any way). Let me try to make a PR that implements that, so we can use that to test whether the typing parts in cosmology work with that.