bug: Wrong alias target path when inspecting class, causing stubs-merging to fail silently

[tlambert03]

Description of the bug

Hey @pawamoy. I'm back working on updating docs for one of my packages to work with the latest griffe. (I've been using a griffe fork for this specific package back since #115 and I'm trying to get off of it. This isn't exactly the same issue, but basically i'm still struggling to get griffe/mkdocstrings to find and inject the stubs provided in a pyi file. I just spent an hour or two trying to debug it, and i'm stumped. as best as I can tell, griffe does find all the members/methods, but mkdocs strings only shows 3 of them.

To Reproduce

mkdir example
cd example
python -m venv .venv
. .venv/bin/activate
pip install mkdocs-material mkdocstrings-python pymmcore
cat <<EOF > mkdocs.yml
site_name: My Docs
theme:
 name: material
plugins:
 - mkdocstrings:
     handlers:
       python:
         options:
           find_stubs_package: true
EOF
mkdir docs
echo "::: pymmcore.CMMCore" > docs/index.md
mkdocs serve

screenshot

This class has a ton of methods, all of which are documented in the stub file, but only a handful are found.

For what it's worth, the stubs i want it to find are at the top level __init__.pyi

➜ tree -L 3 .venv/lib/python3.12/site-packages/pymmcore
.venv/lib/python3.12/site-packages/pymmcore
├── __init__.py
├── __init__.pyi
├── _pymmcore_swig.cpython-312-darwin.so
├── _version.py
├── py.typed
├── pymmcore_swig.i
└── pymmcore_swig.py

(however, I have also tried moving them to other places including _pymmcore_swig.pyi, etc...
If you want to quickly see the stubs, they are here

Expected behavior

find all the methods

Environment information

➜ python -m mkdocstrings.debug
- __System__: macOS-14.5-arm64-arm-64bit
- __Python__: cpython 3.12.3 (/Users/talley/dev/self/example/.venv/bin/python)
- __Environment variables__:
- __Installed packages__:
  - `mkdocstrings` v0.25.1

[pawamoy]

Hi @tlambert03, thanks for the report, and sorry for the troubles 🙂

My first intuition is that Griffe doesn't understand how the stubs are expected to be merged. I'll investigate with the repo you linked 🙂

[pawamoy]

Oh, by the way, the find_stubs_package option is only meant to find *-stubs packages. The *.pyi files within regular packages are always found and merged.

[tlambert03]

Thanks, yeah I had a feeling that was the case :) included it "just in case" :)

[pawamoy]

OK so basically Griffe has trouble understanding your module/object layout. TL;DR at the end 😄

CMMCore lies¹ about its parent module:

>>> from pymmcore import CMMCore
>>> CMMCore.__module__
'pymmcore.pymmcore_swig'

It actually comes from pymmcore._pymmcore_swig. That's OK, objects from the standard lib do this too, and I can understand why. I don't think there is any recommendation about that, so it's hard to tell what's the right/best thing to do.

But CMMCore's methods don't lie the same way:

>>> from pymmcore import CMMCore
>>> CMMCore.__init__.__module__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'wrapper_descriptor' object has no attribute '__module__'. Did you mean: '__reduce__'?

Note that methods usually provide a __module__ attribute:

>>> import griffe
>>> griffe.GriffeLoader.__init__.__module__
'_griffe.loader'

So, without the __module__ attribute, Griffe first falls back to using inspect:

>>> import inspect
>>> from pymmcore import CMMCore
>>> inspect.getmodule(CMMCore.__init__)

...which returns None, so it falls back again by obtaining the module from the runtime object tree it built while loading the package. Basically, it gets pymmcore._pymmcore_swig, since that's what it's currently inspecting.

Then it compares the two, and sees that pymmcore.pymmcore_swig is not pymmcore._pymmcore_swig, so it decides that the object (__init__ here, same for every attribute of the CMMCore class) was not declared in the same module as its parent, and therefore should be aliased.

Finally, Griffe wrongly computes the following target path for the alias, pymmcore._pymmcore_swig.__init__, instead of pymmcore._pymmcore_swig.CMMCore.__init__. This alias is unresolvable, and Griffe later silently fails when trying to merge function overloads obtained from stubs (for other methods though, like defineConfig), short-circuiting the merge of the rest of the class attributes.

Phew.

---

So, what do we learn from this?

First, inspection of runtime objects is not trivial 😂 But this I already knew. Second, and more importantly, I see two things than can be improved in Griffe, while avoiding special cases:

• target path computation (it's currently wrong in some cases as shown above)
• comparison of module paths

To explain the second point, Griffe actually tries to trust objects when they tell it about their parent module. Since the case where an object from a private module says its parent is the equivalent public module is "common" (declared in _a, but says its parent is a), Griffe stopped caring about the truth and just goes with the lie. However it only understands a vs. _a, and not a.b vs. a._b. I remember deciding not to support that, but not why (maybe laziness? or I simply overlooked it. I will check my commit messages and notes on paper if I can find them). In this case, if Griffe supported a.b vs. a._b, there would be no issue.

I mentioned special cases above because Griffe actually has one for pymmcore (from previous tickets):

_cyclic_relationships = {
    ("os", "nt"),
    ("os", "posix"),
    ("numpy.core._multiarray_umath", "numpy.core.multiarray"),
    ("pymmcore._pymmcore_swig", "pymmcore.pymmcore_swig"),
}

        # Special cases: break cycles.
        if (parent_module_path, child_module_path) in _cyclic_relationships:
            return None

...but here it's in reverse, parent module path is pymmcore.pymmcore_swig and child module path is pymmcore._pymmcore_swig. Not sure how to explain why the tuple is reversed now. Maybe I should make it so that the order doesn't matter, but hard to say if this will have other implications.

Finally, it would also have helped if CMMCore's methods had a __module__ attribute saying the same thing as their parent class. I'm not sure what these slot/wrapper descriptors objects are though, and if you have control over them. At least you're using the same module name (safe the leading underscore) 🙏, not like numpy.core._multiarray_umath vs. numpy.core.multiarray 💀 🤣

---

TL;DR - Griffe thinks that the methods of CMMCore are declared in a different module from the class itself, so tries to alias them, but computes wrong alias target paths, which makes merging the stubs silently fail later. Workaround/fix on your end: add a __module__ attribute on CMMCore methods that says 'pymmcore.pymmcore_swig'. Possible fixes on my end too, see wall of text above 😂

Footnotes

1. the word "lie" feels a bit harsh when I write it, so please know that I intend no harm and just try to describe what the objects do 🙏 ↩

[tlambert03]

thank you so much! this is all suuuper helpful, and I really appreciate your time.

also, no need to qualify the word lie 😄 indeed, it's a big fat lie.

It actually comes from pymmcore._pymmcore_swig

fwiw, I did also play with moving the stubs a pymmcore/__init__.pyi to pymmcore/_pymmcore_swig.pyi... but that still didn't help. Does the fact that it didn't help make sense to you as well with what you've learned here?

But CMMCore's methods don't lie the same way:

fascinating... must be a result of how swig wraps methods

So, without the module attribute, Griffe first falls back to using inspect:

mind pointing me to the code in griffe that checks this and falls back to inspect? might want to poke around there a bit.

Finally, it would also have helped if CMMCore's methods had a module attribute saying the same thing as their parent class. I'm not sure what these slot/wrapper descriptors objects are though, and if you have control over them

in terms of "repo control", I can certainly contribute improvements there. I suspect that the core problem here though probably relates to swig itself.

Fwiw, my extremely naive expectation here (or one that I imagined was achievable via some configuration) was that once griffe encountered a stub object for the CMMCore class inside of pymmcore/__init__.pyi, then all the rest would follow from that. That is: it would essentially stop searching/inspecting any of the dynamic runtime stuff, and be fully satisfied with the static description that it found. I definitely recognize that that's not always going to be appropriate, but do you agree that that might be a useful configurable, particularly for C-extensions that may put the entirety of their public API in the stub?

I'm going to take some time to dig more carefully through your response here, and will reply with some more opinions later as to the "best" longer term fix.

[tlambert03]

also, would you prefer to move this topic to griffe?

[pawamoy]

fwiw, I did also play with moving the stubs a pymmcore/init.pyi to pymmcore/_pymmcore_swig.pyi... but that still didn't help. Does the fact that it didn't help make sense to you as well with what you've learned here?

Yes, that makes sense: Griffe's logic for finding and merging stubs works fine, but the logic for inspecting runtime objects does not (in this case at least). It causes the stubs-merging code to fail and silently skip the CMMCore class (because aliases can't be resolved when merging method overloads). If we fix the inspection logic, your stubs will happily get merged 🙂

mind pointing me to the code in griffe that checks this and falls back to inspect? might want to poke around there a bit.

Of course. The crux of it is here:

griffe/src/griffe/agents/nodes/_runtime.py

Line 236 in ce186a4

def alias_target_path(self) -> str | None:

This function is used to decide whether to alias an object, or inspect it in place and set it as a member of the current object in the tree. It is used by the inspector, here:

griffe/src/griffe/agents/inspector.py

Line 233 in ce186a4

if target_path := child.alias_target_path:

Fwiw, my extremely naive expectation here (or one that I imagined was achievable via some configuration) was that once griffe encountered a stub object for the CMMCore class inside of pymmcore/init.pyi, then all the rest would follow from that. That is: it would essentially stop searching/inspecting any of the dynamic runtime stuff, and be fully satisfied with the static description that it found. I definitely recognize that that's not always going to be appropriate, but do you agree that that might be a useful configurable, particularly for C-extensions that may put the entirety of their public API in the stub?

That's a completely valid point! And we have an option for that: allow_inspection=False ☺️ (in Griffe obviously, but also through mkdocstrings' Python handler: https://mkdocstrings.github.io/python/usage/configuration/general/?h=allow#allow_inspection). An alternative is to tell Griffe to load data from your sources, where there are no compiled modules.

also, would you prefer to move this topic to griffe?

Nah that's fine 👍

[pawamoy]

Also, based on my notes, I initially decided that supporting a.b vs. a._b made no sense because I was only thinking in terms of automatic loading of "external" objects (another feature of the Griffe loader that makes the loading of _ast transparent when we load ast, for example).

Click to see the actual notes and my beautiful hand-writing

[pawamoy]

Actually yeah I'll move the issue to Griffe 👍

[tlambert03]

allow_inspection=False

amazing, i'm sorry i missed that. indeed setting that gets me much farther! now we're finding everything:

And inheritance is working great using something like this:

         options:
           preload_modules: [pymmcore]
           allow_inspection: false
           inherited_members: true

This may very well suffice for my needs. But now that you've pinpointed the more interesting guts of it all, I'm going to step back a bit and re-evaluate a) how I'm designing my docs and b) whether there's anything I can contribute to improve the state of things.

You've put much more time into this than expected, thanks so much. Feel free to put it on the back-burner until I check in again (unless you've found some things that you definitely want to work on here)

[pawamoy]

I took a couple slow days, so I guess I was energized enough to actually write down my whole investigation 😂 No problem at all, I like hunting bugs 🙂 If allow_inspection is all you need, that's perfect to me! If you still want to improve the state of things regardless, you can look into swig internals to see if it's possible to add __module__ attributes to methods too (I'd be curious to know that). Other than that, yes, I already have a fix for the wrong alias target paths, and will probably also work on supporting a.b vs. a._b too, because only the first fix will not solve the issue I think (as it would create cyclic aliases anyway, which is a bit better than plain wrong aliases, but still not good).

[tlambert03]

minor note about swig while looking into this. I was at least able to determine what is "special" about the few staticmethods that do show up. Here is a section of the swig_wrap.cpp file that swig generates when building the c-extension for pymmcore:

SWIGINTERN PyMethodDef SwigPyBuiltin__CMMCore_methods[] = {
  { "noop", (PyCFunction)(void(*)(void))_wrap_CMMCore_noop, METH_STATIC|METH_NOARGS, "noop()" },
  { "enableFeature", (PyCFunction)(void(*)(void))_wrap_CMMCore_enableFeature, METH_STATIC|METH_VARARGS, "\n"
		"enableFeature(char const * name, bool enable)\n"
		"\n"
		"Parameters\n"
		"----------\n"
		"name: char const *\n"
		"enable: bool\n"
		"\n"
		"" },
  { "isFeatureEnabled", (PyCFunction)(void(*)(void))_wrap_CMMCore_isFeatureEnabled, METH_STATIC|METH_O, "\n"
		"isFeatureEnabled(char const * name) -> bool\n"
		"\n"
		"Parameters\n"
		"----------\n"
		"name: char const *\n"
		"\n"
		"" },
  { "loadDevice", _wrap_CMMCore_loadDevice, METH_VARARGS, "\n"
		"loadDevice(CMMCore self, char const * label, char const * moduleName, char const * deviceName)\n"
		"\n"
		"Parameters\n"
		"----------\n"
		"label: char const *\n"
		"moduleName: char const *\n"
		"deviceName: char const *\n"
		"\n"
		"" },

SwigPyBuiltin__CMMCore_methods is an array of PyMethodDef structes. The second item in the struct is supposed to be a PyCFunction... but as you can see, only the first three static methods (noop, enableFeature, and isFeatureEnabled) actually are... which explains why they showed up in the screenshot in the original post.

all the other are simply defined as PyObject. such as the first one for the loadDevice method:

SWIGINTERN PyObject *_wrap_CMMCore_loadDevice(PyObject *self, PyObject *args) { ...

so, it would appear that swig isn't generating a proper method definition 🤷

[pawamoy]

Ah, super interesting, thanks! So, supposedly, if it generated all the methods the same way, casting them with (PyCFunction)(void(*)(void)), they would all get a __module__ attribute, allowing Griffe to function properly. But maybe PyCFunction is only meant for staticmethods, which the rest are not? Well, I'm out of my depth here 😅

[tlambert03]

yep, I'm out of my depth as well... however, it does seem based on that C-Python docs for PyMethodDef (and indeed it fits with my understanding of methods) that all methods, and not only static methods, are regular python functions (not just Callable, but actually type FunctionType or BuiltinFunctionType). that's obviously not to say that all callable objects on a class must be methods, they can also be descriptors or many other types, so swig isn't doing anything illegal (outside of declaring that array to be an array of PyMethodDef?)

In [8]: from types import BuiltinFunctionType

In [9]: isinstance(pymmcore.CMMCore.enableFeature, BuiltinFunctionType)
Out[9]: True

In [10]: isinstance(pymmcore.CMMCore.__init__, BuiltinFunctionType)
Out[10]: False

In [11]: type(pymmcore.CMMCore.enableFeature)
Out[11]: <class 'builtin_function_or_method'>

In [12]: type(pymmcore.CMMCore.__init__)
Out[12]: <class 'wrapper_descriptor'>

relevant sections of the swig source code:

• https://github.com/swig/swig/blob/ce2bb71a486bd575afb3c19ef98aae5f5e147905/Source/Modules/python.cxx#L4696-L4722
• https://github.com/swig/swig/blob/ce2bb71a486bd575afb3c19ef98aae5f5e147905/Source/Modules/python.cxx#L4092-L4093
• and here, where PyObject is chosen rather than PyCFunction: https://github.com/swig/swig/blob/ce2bb71a486bd575afb3c19ef98aae5f5e147905/Source/Modules/python.cxx#L5116-L5120

---

• that all methods, and not only static methods, are regular python functions (not just Callable, but actually type FunctionType or BuiltinFunctionType)

I suspect that from SWIG's perspective, it's different because they don't need to wrap the first argument, unlike for a classmethod or regular method

[pawamoy]

I've pushed two bug fixes 😌 They will make it to the next 0.x version. I believe we can close this now 🙂

[tlambert03]

thanks as always! Will try it out later today.

[tlambert03]

wow, great work! definitely fixes this issue 👍

[pawamoy]

You confirm this works well even with allow_inspection=True?

[tlambert03]

i was just trying my original example here and seeing all the new stuff it found. however, for posterity. here is what i get with allow_inspection: true

(and it goes on with all the methods)

---

and with allow_inspection: false (where it sees my stubs and stops)

---

so, what you did certainly fixed the issue resulting from weirdness of SWIG-wrapped extensions 👍

[pawamoy]

Perfect, thanks for confirming!