bug: "regression" in 1.1.0 regarding "Multiple URLs found for"

[llucax]

Warning

This report might look like:

After writing all of this, this probably should have been a feature request instead...

TL;DR: Could an option be added to disable the "Multiple URLs found for" warning and just let references to silently resolve locally as before and/or have an option to resolve links with multiple URLs found to a specific page?

Description of the bug

After doing an update from 1.0.1 to 1.1.0 I started to get many warnings in the form:

Multiple URLs found for 'x.y.z': ['reference/x/y/z/#x.y.z', 'user-guide/x/y/z#x.y.z']. Make sure to use unique headings, identifiers, or Markdown anchors (see our docs).

And since we are using strict mode, this means an error and docs that won't build.

To Reproduce

The thing is we are using this pattern to try to have as much documentation as possible as part of docstrings, but still have a nice and more organized "user guide": we use some internal modules docstrings as the user guide section and then include it in the markdown documentation. For example:

$ cat docs/user-guide/formula-engine.md
# Formula Engine

::: frequenz.sdk.timeseries.formula_engine.FormulaEngine
    options:
        members: None
        show_bases: false
        show_root_full_path: false
        show_source: false

::: frequenz.sdk.timeseries.formula_engine.FormulaEngine3Phase
    options:
        members: None
        show_bases: false
        show_root_full_path: false
        show_source: false

And we also generate API reference documentation using some script based on what's suggested in the docs, which will create a file like:

$ cat docs/reference/frequenz/sdk/timeseries/formula_engine/index.md 
::: frequenz.sdk.timeseries.formula_engine

Hence, we have the same anchor in 2 different pages.

Expected behavior

Before this upgrade all worked as more or less fine, links just being resolved locally when possible, and externally when not.

But I think things could be even improved for this use case, as before in the example above FormulaEngine in the user guide will resolve to the user guide, but ideally it should resolve to the API reference.

1. Add an option to ignore multiple URLs for warnings.

   a. Globally in mkdocs.yaml
   b. Locally in the `:: x.y.z`` block

   Example:

   # Formula Engine
   
   ::: frequenz.sdk.timeseries.formula_engine.FormulaEngine
       options:
           members: None
           show_bases: false
           show_root_full_path: false
           show_source: false
           disable_multiple_urls_warning: true

2. A way to tell where to resolve multiple references to a specific page. For example:

   For example something like:

   # Formula Engine
   
   ::: frequenz.sdk.timeseries.formula_engine.FormulaEngine
       options:
           members: None
           show_bases: false
           show_root_full_path: false
           show_source: false
           resolve_multiple_urls:
               "frequenz.sdk.timeseries.formula_engine.FormulaEngine": reference/frequenz/sdk/timeseries/formula_engine.md

Additional context

• Failing CI job trying to generate the docs: Fix glossary links and restrict mkdocs-autorefs updates frequenz-floss/frequenz-sdk-python#1043
• Link to the working documentation generated previous to the update: https://frequenz-floss.github.io/frequenz-sdk-python/v1.0-pre/
• Script generating the reference docs: https://github.com/frequenz-floss/frequenz-repo-config-python/blob/v0.x.x/src/frequenz/repo/config/mkdocs/api_pages.py#L43

[pawamoy]

Thanks for the report! I was expecting to get one but still thought this warning had to be enabled by default. I definitely didn't anticipate this use-case you have though.

But I think things could be even improved for this use case, as before in the example above FormulaEngine in the user guide will resolve to the user guide, but ideally it should resolve to the API reference.

That's exactly the issue the warning is aiming to solve. With non-unique heading ids across pages, cross-references might link you to one or the other, non-deterministically (or deterministically, but depending on the order the pages are built, and this might be more complex than that).

I agree that this could be a feature request indeed: the feature I think we need here is a way to change the heading ids of the ones if your Guide pages. By the way, I can't seem to find any mkdocstrings-generated headings in your Guide pages, could you link me to some examples? I'd like to see how you render them. EDIT: I see subheadings such as #frequenz.sdk.actor--the-run-function, but not headings themselves. EDIT 2: OK I see #frequenz.sdk.timeseries.formula_engine.FormulaEngine. Well, since there isn't even a signature rendered, I believe the manual heading approach below would work for you? Even if you rendered a signature I think it would work.

It might be possible to workaround this right now, by doing the following:

• manually write the headings in your guide pages: ### MyClass
• but change their id: ### MyClass {#guide-MyClass} or ### MyClass {#guide-complete.path.to.MyClass}
• render the object by omitting its heading:

  ::: complete.path.to.MyClass
      options:
          show_root_heading: false

We have to change the Guide headings because we cannot change the API ones (yet), as that would break cross-site references. Maybe not afternall. Will need to think about it.

[llucax]

By the way, I can't seem to find any mkdocstrings-generated headings in your Guide pages

Sorry, I don't know what this means. 😞, so I don't know how to point you to examples. Not sure if you still need them given the EDITs.

It might be possible to workaround this right now, by doing the following:
...
Maybe not afternall. Will need to think about it.

Yeah, no, sadly it doesn't. We are even using show_root_heading: false already in some places where we get warnings too.

[pawamoy]

@llucax I'd like to inspect a bit more your docs sources, is there a particular branch I should look at? One that triggers the warnings. I can't seem to find the sources for https://frequenz-floss.github.io/frequenz-sdk-python/v1.0-pre/user-guide/formula-engine/. I don't think the gen-files script generates that too, right?

[llucax]

The default branch should have the issue. We just pinned the dependency to avoid CI failures until we can find a solution, but if you manually update the dependency it should break.

All non-generated docs are in docs/. The doc you are looking for is here: https://github.com/frequenz-floss/frequenz-sdk-python/blob/v1.x.x/docs/user-guide/formula-engine.md.

https://github.com/frequenz-floss/frequenz-sdk-python/blob/v1.x.x/docs/user-guide/microgrid-concepts.md also have the issue.

Thanks a lot for looking at it! ❤️

[pawamoy]

Thanks, I wasn't looking into the right repo.

So, by adding the following options:

        show_root_heading: false
        show_root_toc_entry: false

(which you already have in actors and microgrid-concepts)

...that reduces the number of warnings. The four remaining ones come from subheadings in the corresponding objects' docstrings (### The run() Function): these subheadings are rendered and their id is prefixed with the object path, whether we've rendered the root heading or not, so whether we're in the guide or the reference. In the end: duplicated ids.

A few solutions that come to mind:

• (in mkdocstrings) detect that the root heading (and toc entry) are not rendered, and therefore avoid prefixing the subheadings. But the issue would pop its head again with more than two places where we render the docstring.
• (in mkdocstrings) add an option to configure the prefixing behavior for subheadings: no prefix, default behavior (prefix with object path), custom prefix. We do something similar in Markdown Exec 👍

Obviously, we could also consider reverting, and putting these warnings behind an option. But that means a lot of maintenance, to maintain incorrect/un-deterministic behavior a few projects rely on. If many projects are impacted by this and come here to add their voice, we'll do something, but for now it seems only two projects are impacted.

WDYT? Would an option to configure (sub)headings prefixes work for you? I imagine something like this:

# Actors

::: frequenz.sdk.actor
    options:
        members: []
        headings_prefix: ""
        show_bases: false
        show_root_heading: false
        show_root_toc_entry: false
        show_source: false

---

There's an alternative solution by the way. You could use pymdownx.snippets instead of mkdocstrings to inject your docstrings into your "Guide" Markdown pages. See https://facelessuser.github.io/pymdown-extensions/extensions/snippets/#snippet-sections. Snippets lets you declare sections in your sources, whathever the language (and comment syntax). Then you can inject just these sections into pages.

In src/frequenz/sdk/actor/__init__.py:

# License: MIT
# Copyright © 2022 Frequenz Energy-as-a-Service GmbH

# Please note this docstring is included in the getting started documentation in docs/.
"""Actors are a primitive unit of computation that runs autonomously.

<!-- --8<-- [start:docstring] -->

## Actor Programming Model
...

<!-- --8<-- [end:docstring] -->
"""

I'm using HTML comment syntax since we're in a Markdown docstring. Note that the docstring summary is not included in the snippet section because it would be weird. So here you'd have to repeat just this summary in the Markdown page:

# Actors

Actors are a primitive unit of computation that runs autonomously.

--8<-- "src/frequenz/sdk/actors/__init__.py:docstring"

Well, it's not perfect, just wanted to list this alternative 🙂

[llucax]

Hi, thanks again for having a deep look at this. I also agree that just disabling the warning is not ideal, as there is a potential issue, it would be much better to have a way to disambiguate something ambiguous instead.

About the snippets solution, I prefer to avoid it, another goal of us is to keep docstrings as readable as possible, same reason we try to keep most docs in docstrings, they are accessible via the IDE and code editors, which is in our experience the main way our users look at docs.

So your proposed solution of headings_prefix means that these headings will get a different prefix depending on where they were "included", so for example we could leave no prefix for the reference docs and have a headings_prefix: "user-guide" in the user guide for example? I guess that could work.

So we are currently linking to that "The run() method" using:

[_run]: #the-_run-method

In this case, if we leave the link as is it will point to the reference, and if we change it to user-guide-the-_run_method it will point to the user guide, right?

I guess we can live with that, but ideally the link should resolve "locally" when possible, it would be best if it resolved to the user guide when rendered in the user guide and to the reference when rendered in the reference.

This is why I was thinking if it would be possible to have some sort of "resolve multiple URL using this page" kind of option I suggested before. So in this case mkdocstrings will find 2 locations for #the-_run-method, but we'll have something like this:

::: frequenz.sdk.timeseries.formula_engine.FormulaEngine
    options:
        members: None
        show_bases: false
        show_root_full_path: false
        show_source: false
        resolve_multiple_urls:
            "#the-_run-method": reference/frequenz/sdk/actor.md

So it will use the link to that page without giving any warnings.

I can imagine it might be a bit of work and maybe it is not justified if we one of a kind using this documentation scheme, but it would be good to know if that would be even doable :)

[pawamoy]

About the snippets solution, I prefer to avoid it, another goal of us is to keep docstrings as readable as possible

Completely agree 👍

so for example we could leave no prefix for the reference docs and have a headings_prefix: "user-guide" in the user guide for example?

That's the idea 🙂

In this case, if we leave the link as is it will point to the reference, and if we change it to user-guide-the-_run_method it will point to the user guide, right?

Yes!

ideally the link should resolve "locally" when possible, it would be best if it resolved to the user guide when rendered in the user guide and to the reference when rendered in the reference.

Then you shouldn't use autorefs for this. Autorefs will always resolve references to one and only one location, be it on the same page or somewhere else. Instead, you should use the native reference syntax [some title](#some-anchor-on-this-very-page): this will always be local to the page, wherever the docstring is rendered.

[...] I can imagine it might be a bit of work and maybe it is not justified [...]

Such a solution would have to be implemented in autorefs, but would only be supported through mkdocstrings, which would be a bit weird. Autorefs will definitely have more features in the future (and this might be one), but we're not ready for that yet. We need solid foundations for this type of features first (there's some work going in that direction with #46) 🙂

[pawamoy]

Actually, even prefixing ids might be complicated to implement 🤔 If the-run-method gets prefixed as user-guide-the-run-method, then what are we supposed to do with [the run() method][the-run-method]? Should we prefix the id there too? But we don't know yet that user-guide-the-run-method exists. And even if it does, how do we know we don't really want to use the-run-method untouched, because it could exist somewhere else later? 🤔 🤔 🤔

[llucax]

Then you shouldn't use autorefs for this. Autorefs will always resolve references to one and only one location, be it on the same page or somewhere else. Instead, you should use the native reference syntax [some title](#some-anchor-on-this-very-page): this will always be local to the page, wherever the docstring is rendered.

I see, so if all the anchors pointing to symbols are fixed by using:

        show_root_heading: false
        show_root_toc_entry: false

(not sure if the later is desirable or not, will need to check it out)

And for regular anchors we can avoid autorefs and it does exactly what we want (resolve locally) without any warnings, I think that's all we need. I'm not sure why we used autorefs there, but I wouldn't surprised if it was just a mistake.

I will try it out.

Actually, even prefixing ids might be complicated to implement 🤔 If the-run-method gets prefixed as user-guide-the-run-method, then what are we supposed to do with [the run() method][the-run-method]? Should we prefix the id there too? But we don't know yet that user-guide-the-run-method exists. And even if it does, how do we know we don't really want to use the-run-method untouched, because it could exist somewhere else later? 🤔 🤔 🤔

If the above works out, then I don't think I need prefixes at all. I'm not sure if we are using deep linking to link into a section of a docstring, but if we do I guess we could live without those.

Will try to give this a try (next week) and report back.

Once again, thanks a lot for your support! ❤️

[pawamoy]

I'm not sure why we used autorefs there, but I wouldn't surprised if it was just a mistake.

Possibly for consistency. For example, the Actor class also uses an autorefs link to the-_run-method IIRC.

It's possible that there are places where you cannot use a "local reference" (direct link using # instead of autorefs) because the link is not rendered on the same page, and using an autoref will emit warnings. So you might find yourself stuck again 😕

If the above works out, then I don't think I need prefixes at all.

Yes please, don't hesitate to share your progress with me, I'll gladly chime in again and try to help 😄 I'll leave this open in the meantime.