Improve compatibility between old sphinx and new mkdocs setup

[maxrjones]

This PR aims to address #3542.

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/user-guide/*.md
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[maxrjones]

I'm not able to request reviews from you, but FYI @ilan-gold @ianhi

[ilan-gold]

Commented here @maxrjones but I think the check (in ci) should be on the objects.inv file. I don't think breaking http links is a python package's problem (personally, maybe unpopular since I'm very intersphinxy), but breaking intersphinx links is probably bad.

[maxrjones]

Commented here @maxrjones but I think the check (in ci) should be on the objects.inv file. I don't think breaking http links is a python package's problem (personally, maybe unpopular since I'm very intersphinxy), but breaking intersphinx links is probably bad.

Can you please provide an example of an intersphinx link that is still broken following the changes in this PR?

[ilan-gold]

Can you please provide an example of an intersphinx link that is still broken following the changes in this PR?

Apologies, I should have been clearer. I wasn't commenting on how well this fixes things, more just responding to what Davis said about checking URLs directly. I think the unit of interest shouldn't be the actual URLs but the intersphinix lookup i.e., the later should be checked. I try to avoid using URLs for exactly that reason - they can change but intersphinx allows us to keep the location in the reference the same over that.

That being, here's what I got locally against https://zarr--3544.org.readthedocs.build/en/3544/:

/Users/ilangold/Projects/Theis/arrayloaders/docs/index.md:22: WARNING: undefined label: 'zarr:user-guide-sharding' [ref.ref]
/Users/ilangold/Projects/Theis/arrayloaders/docs/index.md:60: WARNING: py:mod reference target not found: zarr [ref.mod]
/Users/ilangold/Projects/Theis/arrayloaders/docs/index.md:60: WARNING: py:mod reference target not found: zarr [ref.mod]
/Users/ilangold/Projects/Theis/arrayloaders/docs/zarr-configuration.md:10: WARNING: py:mod reference target not found: zarr [ref.mod]
/Users/ilangold/Projects/Theis/arrayloaders/docs/zarr-configuration.md:35: WARNING: unknown document: 'zarr:user-guide/config' [ref.doc]

UPDATE: tried remotely as well: https://app.readthedocs.org/projects/annbatch/builds/30070579/

[d-v-b]

is there anything else we need here?

[ilan-gold]

I guess the question is whether or not the failures I saw in #3544 (comment) are acceptable or not.

[d-v-b]

I think we'd like the transition to the new docs to be as smooth as possible. What do we need in this PR to fix the failures you saw? Bear in mind that I know nothing about how intersphinx works!

[ilan-gold]

What do we need in this PR to fix the failures you saw? Bear in mind that I know nothing about how intersphinx works!

Without being able to say specifically how to accomplish this, these references need to appear in the objects.inv file that sphinx creates. I haven't used mkdocs, so I can't really say I know for sure either. Looking in https://github.com/zarr-developers/zarr-python/tree/main/docs I don't see a conf.py which could account for lack of mod references. Regarding the markdown, I would assume that this is something to do with mkdocs internally. I would guess there is a setting, but would need to look into it more to be sure.

[d-v-b]

the end of this section of the mkdocstrings docs might be useful: https://mkdocstrings.github.io/usage/#cross-references-to-other-projects-inventories

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.77%. Comparing base (b3e9aed) to head (61e845a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3544   +/-   ##
=======================================
  Coverage   61.77%   61.77%           
=======================================
  Files          85       85           
  Lines       10165    10165           
=======================================
  Hits         6279     6279           
  Misses       3886     3886           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maxrjones]

the end of this section of the mkdocstrings docs might be useful: mkdocstrings.github.io/usage#cross-references-to-other-projects-inventories

We're using that feature for the objects inventory. However, it only adds references to API objects rather than all headers and sub-headers/labels, which is what sphinx does. We are missing mkdocstrings/mkdocstrings#508.

Lots of libraries use intersphinx mapping for zarr: https://github.com/search?q=%22https%3A%2F%2Fzarr.readthedocs.io%22+path%3A**%2Fconf.py&type=code, but it may be more for the API cross-referencing than linking to docs pages.

If we want complete compatibility with intersphinx, I think we have three options:

1. switch back to sphinx
2. create a hook that modifies the objects inventory
3. update/create a plugin to populate the objects inventory with all pages (i.e., [autorefs] Support interlinking to non-object references mkdocstrings/mkdocstrings#508).

It'd be a bummer if the bespoke objects.inv has us locked into sphinx (1), but that might be the case. I'll look into how hard (2) or (3) would be.

[maxrjones]

the end of this section of the mkdocstrings docs might be useful: mkdocstrings.github.io/usage#cross-references-to-other-projects-inventories

We're using that feature for the objects inventory. However, it only adds references to API objects rather than all headers and sub-headers/labels, which is what sphinx does. We are missing mkdocstrings/mkdocstrings#508.

Lots of libraries use intersphinx mapping for zarr: github.com/search?q=%22https%3A%2F%2Fzarr.readthedocs.io%22+path%3A**%2Fconf.py&type=code, but it may be more for the API cross-referencing than linking to docs pages.

If we want complete compatibility with intersphinx, I think we have three options:

1. switch back to sphinx
2. create a hook that modifies the objects inventory
3. update/create a plugin to populate the objects inventory with all pages (i.e., [autorefs] Support interlinking to non-object references mkdocstrings/mkdocstrings#508).

It'd be a bummer if the bespoke objects.inv has us locked into sphinx (1), but that might be the case. I'll look into how hard (2) or (3) would be.

I think there are enough differences between how sphinx and mkdocs handle references that we will need to just switch to sphinx + sphinx-immaterial if we want full compatibility with intersphinx. For example, sphinx's autodoc seems to add headers for every attribute of a class (https://zarr.readthedocs.io/en/stable/api/zarr/abc/buffer/index.html#zarr.abc.buffer.BufferPrototype.buffer) whereas mkdocstrings does not (https://zarr.readthedocs.io/en/latest/api/abc/buffer/#zarr.abc.buffer.BufferPrototype). While we could try to reproduce sphinx' current logic in a hook to add more references to the object inventory from mkdocstrings, but the hook would likely deviate from sphinx' behavior over time.

[ianhi]

I think there are enough differences between how sphinx and mkdocs handle references that we will need to just switch to sphinx + sphinx-immaterial if we want full compatibility with intersphinx.

:(

can we at least keep your markdown changes by using myst?

[d-v-b]

I don't think complete sphinx compatibility needs to be our goal. Otherwise, we would have stuck with sphinx. I'm fine breaking some auto-generated documentation links as part of the release of a totally new docs backend, especially since we are doing this in a 3.2 release. What do other people think?

[ianhi]

but it may be more for the API cross-referencing than linking to docs pages.

I'd suspect that this is the case, and that direct linking to actual URLs is the case for linking to a section.

breaking some auto-generated documentation links as part of the release of a totally new docs backend, especially since we are doing this in a 3.2 release. What do other people think?

I'd be pretty happy if:

1. All API intersphinx works
2. Old linked URLs function

[maxrjones]

but it may be more for the API cross-referencing than linking to docs pages.

I'd suspect that this is the case, and that direct linking to actual URLs is the case for linking to a section.

breaking some auto-generated documentation links as part of the release of a totally new docs backend, especially since we are doing this in a 3.2 release. What do other people think?

I'd be pretty happy if:

1. All API intersphinx works
2. Old linked URLs function

I just pushed some changes to reach this goal.

[d-v-b]

thanks for this work max! @ilan-gold how do things look on your end with these changes?

[ilan-gold]

https://app.readthedocs.org/projects/annbatch/builds/30107997/ Still some errors, but maybe the way I'm linking to the pages is wrong. I saw you guys do local linking (i.e., not intersphinx) internally, so I couldn't glean much from that:

zarr-python/docs/user-guide/arrays.md

Line 26 in b3e9aed

[Persistent arrays](#persistent-arrays) for details on storing arrays in other stores,

. I tried running locally against anndata and got:

/Users/ilangold/Projects/Theis/anndata/docs/fileformat-prose.md:9: WARNING: unknown document: 'zarr:index' [ref.doc]
/Users/ilangold/Projects/Theis/anndata/docs/fileformat-prose.md:93: WARNING: unknown document: 'zarr:user-guide/arrays' [ref.doc]
/Users/ilangold/Projects/Theis/anndata/src/anndata/_types.py:docstring of anndata._types.Write.__call__:10: WARNING: unknown document: 'zarr:index' [ref.doc]
/Users/ilangold/Projects/Theis/anndata/src/anndata/_types.py:docstring of anndata._types.WriteCallback.__call__:14: WARNING: unknown document: 'zarr:index' [ref.doc]
/Users/ilangold/Projects/Theis/anndata/docs/tutorials/zarr-v3.md:9: WARNING: undefined label: 'zarr:user-guide-consolidated-metadata' [ref.ref]
/Users/ilangold/Projects/Theis/anndata/docs/tutorials/zarr-v3.md:26: WARNING: unknown document: 'zarr:user-guide/consolidated_metadata' [ref.doc]
/Users/ilangold/Projects/Theis/anndata/docs/tutorials/zarr-v3.md:38: WARNING: undefined label: 'zarr:user-guide-sharding' [ref.ref]
/Users/ilangold/Projects/Theis/anndata/docs/tutorials/zarr-v3.md:103: WARNING: undefined label: 'zarr:user-guide-gpu' [ref.ref]

These aren't headers, I think, just pages.

I don't think complete sphinx compatibility needs to be our goal. Otherwise, we would have stuck with sphinx.

At the end of the day, I think it's a decision for y'all whether you want the docs' guide pages to be intersphinx linkable in every way you were before. I personally like being able to point to a stable interphinx URL for full pages but I'm not sure it's worth blocking on at this point. I like the way the docs look so I'll survive either way.

[ilan-gold]

P.S: https://github.com/search?q=%22zarr%3Auser%22&type=code we appear to be the only people using this functionality.

[maxrjones]

FYI I moved the scripts to gists to not pollute the repo:

• check links - https://gist.github.com/maxrjones/ca4bb1d66b2327fa103223b710d58bf4
• check object inventories - https://gist.github.com/maxrjones/a97c65facb2197376f49c6fe5ce2702e

[ianhi]

Is there anything more you want to do here @maxrjones ? This situation is a bit of a bummer for @ilan-gold so how do you feel about this tradeoff?

[ilan-gold]

I would prefer to have errors when links break, but I am not sure it's holding up this release over this issue. It's not that many links. Maybe mkdocs fixes this issue at some point, also. So I wouldn't hold things up over only my use case if that's the question.

[maxrjones]

Thanks for understanding, @ilan-gold. I think this is ready for review. We can commit to providing absolute URLs via redirects at a minimum, but unfortunately it seems we cannot guarantee non-API intersphinx links work without extensive development.