Fix downstream marshmallow schema cross-references

[Marenz]

Use absolute cross-references in inherited marshmallow schema docstrings.

Fixes downstream strict documentation builds.

[llucax]

This is not broken. Things are building correctly, the docs are published correctly, with working links. Maybe you are using an older mkdocs version that doesn't support the new relative_crossrefs: true?

https://frequenz-floss.github.io/frequenz-quantities-python/latest/reference/frequenz/quantities/experimental/marshmallow/

[shsms]

IIUC, this is to fix this @luca: https://github.com/frequenz-floss/frequenz-sdk-python/actions/runs/34107815364/job/101696786441?pr=1454

I'd like that to be merged, so that I can merge the steam boiler PR and get it released tomorrow.

[llucax]

This is super weird, I don't think a project should ever be following or rendering docs from a dependency. But in any case the correct fix would be to use relative crossrefs in the SDK, which should be done automatically when upgrading to repo-config v0.19.

[llucax]

I see that's blocked on the exhaustion checks, so I will look into them now.

[llucax]

There seems something else going on, it seems for some reason now properdocs is being used to render docs instead of mkdocs 😱

[llucax]

The rest could be wrong too, I didn't check. So this fix might be valid, but we should keep the relative refs when it makes sense (maybe up to ...?). The idea about relative refs is to make refs shorter, and easier to understand, once you get ..... it is really hard to follow, so absolute refs make sense in those cases.

[llucax]

OK, I see now that this is really wrong, it is using four .:

1. . -> the class frequenz.quantities.experimental._QuantityField
2. .. -> the module frequenz.quantities.experimental
3. ... -> the parent package frequenz.quantities
4. .... -> frequenz

I guess we don't get a failure here because this is a private symbol, so it is never rendered.

[llucax]

The count is off by one, and there's an extra module level missing. Looking at AutorefsHook.expand_identifier in mkdocstrings-python, the first dot only marks the reference as relative and doesn't move anywhere; each additional dot walks up one parent. And the chain here starts at frequenz.quantities.experimental.marshmallow, the marshmallow module is a level of its own.

So for [`Quantity`][....Quantity] on the _QuantityField class docstring, the three walking dots go _QuantityField → marshmallow → experimental → frequenz.quantities, giving frequenz.quantities.Quantity. That's correct as written. Being private isn't what saves it.

[llucax]

In the SDK we get a failure here. This one is wrong too, 5 dots:

1. . -> the module frequenz.quantities.experimental
2. .. -> the parent package frequenz.quantities
3. ... -> frequenz
4. .... -> the root
5. .... -> ???

This one is public, so no idea why it is not failing in this repo.

[llucax]

Same off-by-one: TYPE_MAPPING → QuantitySchema → marshmallow → experimental → frequenz.quantities, so [.....Quantity] resolves to frequenz.quantities.Quantity and is correct here.

It doesn't fail in this repo because the walk starts at frequenz.quantities.experimental.marshmallow.QuantitySchema.TYPE_MAPPING. In the SDK the same docstring is re-rendered as frequenz.sdk.config.BaseConfigSchema.TYPE_MAPPING, since BaseConfigSchema subclasses QuantitySchema and the SDK sets inherited_members: true. Four levels up from there is frequenz, so it looks for frequenz.Quantity and finds nothing. The reference is right where it's written and wrong everywhere it's inherited.

[llucax]

I reproduced this outside both repos with two throwaway packages, qa holding a Base class whose attributes use relative refs and sb holding Child(Base) plus a re-export. It confirms the diagnosis behind this PR, and it also contradicts a few things I said earlier.

This is not broken. Things are building correctly, the docs are published correctly, with working links. Maybe you are using an older mkdocs version that doesn't support the new relative_crossrefs: true?

Both repos pin exactly the same doc toolchain, mkdocstrings-python == 2.0.5 and mkdocstrings[python] == 1.0.6. This repo's build is genuinely fine and always will be. The references only break in a package that inherits from QuantitySchema, which is why the failure shows up in the SDK and not here.

I don't think a project should ever be following or rendering docs from a dependency. But in any case the correct fix would be to use relative crossrefs in the SDK, which should be done automatically when upgrading to repo-config v0.19.

The SDK isn't rendering the dependency's docs, it's rendering its own class. BaseConfigSchema subclasses QuantitySchema and adds nothing but a Meta, so with inherited_members: true every documented member on it comes from here, docstrings included. That's deliberate and it's what makes the inherited API visible at all.

Turning on relative crossrefs in the SDK makes it worse, not better. With relative_crossrefs: true in the consuming project, mkdocstrings-python 2.0.5, the version we pin, aborts the build with AttributeError: 'NoneType' object has no attribute 'path' inside AutorefsHook.expand_identifier, because it tries to walk four parents up a chain that only has three. 2.0.8 added a None guard, so there it degrades to resolving frequenz.Quantity instead, still a failure, just a polite one. Either way the SDK can't fix this from its side without dropping inherited_members or filtering the member out, and both lose documentation.

we should keep the relative refs when it makes sense (maybe up to ...?). The idea about relative refs is to make refs shorter, and easier to understand, once you get ..... it is really hard to follow, so absolute refs make sense in those cases.

Depth isn't really the criterion, what matters is whether the walk leaves the object that gets re-rendered elsewhere. I measured all three cases downstream. ..SIBLING from a member resolves fine after inheritance, because the whole class is re-rendered under the new path and the sibling comes along. ...Base already breaks, because two levels up leaves the class and lands in whatever module the subclass happens to live in. So for a public class that others can subclass or re-export, the safe rule is that no reference may leave the class, which in practice means . and .. only. Anything deeper has to be absolute.

That makes this PR's scope right as it stands. experimental/marshmallow.py is the only module here with references that escape a public class. The other relative refs live in frequenz/quantities/__init__.py, and a module docstring can't be inherited or re-exported, so they're safe to keep.

Worth knowing that inheritance isn't the only trigger. In the repro a plain from qa import Quantity re-export breaks the same way, resolving ..experimental.marshmallow.Base against sb.config. Nothing in this repo hits that today, but it's the same failure mode if some downstream package ever re-exports a quantity.

The underlying behaviour is an upstream bug and I've filed it as mkdocstrings/python#341. For an inherited member griffe hands the renderer an Alias whose .path and .parent point at the inheriting class while .filepath, .target and .docstring.parent all point at the defining one, and expand_identifier happens to walk the first pair while get_context next to it uses the second, which is why the warning names this repo's file while the resolution climbs the SDK's tree.

One detail from that investigation argues for merging this regardless of what upstream does. A red build is the lucky outcome: when the downstream package happens to define its own symbol with the colliding name, the reference resolves to it, the build passes under strict: true, and the published docs carry a silently wrong link. mkdocstrings has also been in maintenance mode since November, so a fix there isn't something to wait on. Merging this and cutting 1.0.3 unblocks the SDK's docs builds now.

[llucax]

OK, so let's go with this one. Then we release v1.0.3 and bite the major bump that should have never happened, or sort of, the module is marked as experimental and says anything can break at any time (although history have told us that's a dumb idea, because as we saw right now, things will blow up in our face anyway, and we should avoid that).

Unrelated, but I would consider moving this marshmallow stuff from experimental (or to a different library).