SOLVED: Docstring inheritance

[keckler]

There are multiple instances of methods that do not have docstrings, and in that case Sphinx will inherit the parent's docsting, if it exists. In some cases, this leads to misleading or inaccurate docstrings which are hard to identify unless one explicitly compares the documentation with the method source. Some particular examples of this are:

• https://terrapower.github.io/armi/.apidocs/armi.materials.zincOxide.html#armi.materials.zincOxide.ZnO.density
• https://terrapower.github.io/armi/.apidocs/armi.materials.yttriumOxide.html#armi.materials.yttriumOxide.Y2O3.density
• https://terrapower.github.io/armi/.apidocs/armi.materials.scandiumOxide.html

I understand that all those material classes are pretty niche, but those just happened to be some of the places that I noticed. The point is that this might be happening all over the codebase, and it is hard to immediately tell.

@john-science and I were discussing how this could be fixed. The most obvious solution is just to add docstrings for all the methods that are missing them. But another option that came up would be to disable to inheritance of docstrings from parent classes within Sphinx, as in something akin to: https://stackoverflow.com/questions/16518834/avoid-docstrings-from-parent-in-sphinx

The disabling of docstring inheritance would fix the described problem across the entire codebase with minimal effort. It would also have the added benefit of making it apparent when reading the documentation which methods are lacking docstrings entirely.

However, there could be undesirable consequences to disabling docstring inheritance, such as wiping out docstrings that indeed should be inherited. Or possibly leaving the codebase with many methods that have no descriptions at all, which could make the documentation possibly less useful.

Does anybody have any thoughts the possibility of disabling docstring inheritance within Sphinx?

@ntouran @jakehader @youngmit

[jakehader]

I like the idea of removing docstring inheritance on sphinx. @keckler, @john-science is there a way to configure the linter to tell us how many and which methods don't have docstrings implemented? This might be a good thing to have as part of the code review checklist.

[keckler]

Well actually Pylint does check for methods without docstrings and is supposed to warn us. It should even warn in the case that a docstring is inherited, as long as the method itself does not have its own docstring defined. I didn't realize this before, but the issue (at least with the examples that I cited in my original post) is that we have enabled the docstring-min-length option in our pylintrc so that the docstring check is not performed on any methods with less than 5 lines.

So I guess the problem really only exists for short methods which have inherited docstrings.

Not sure what the original reason was for implementing docstring-min-length, but maybe we want to do away with that?

[jakehader]

We should probably just disable that. Any objections @john-science?

[john-science]

Please do not disable that.

The Sphinx settings for docstrings are set the way they are because enabling that feature throws 10,000 warnings at build time. And the vast majority of those warnings are for numpy, scipy, and other third-party libraries that we can't control.

If you want more docstrings, great. Change the Sphinx-docstring-length command as a way to TEST which methods need docstrings. But as of the last time I tried this, we can't have that be the default behavior in trunk.

[jakehader]

Good points @john-science. Maybe we can use a different linter (e.g., https://github.com/terrencepreilly/darglint) specifically for checking docstring compliance throughout without sphinx providing the 1000s of warnings. Or if there isn't a better tool than sphinx we can just modify the pylintrc file locally for updating the docstrings and then make sure it's not modified for the PR.

[keckler]

To follow up on this dormant thread, here is another example of where docstring inheritance in ARMI becomes super annoying.

As you can see in the image below, all these attributes have the same docstring. And that docstring doesn't even appear to make much sense. Turns out that is a docstring propagating through from Yamlize, and it is all over our API docs (pretty much anytime a class has attributes that are read from user input). So yeah, kinda cluttered and annoying...

[john-science]

And here:

https://terrapower.github.io/armi/.apidocs/armi.reactor.blueprints.assemblyBlueprint.html#armi.reactor.blueprints.assemblyBlueprint.AssemblyBlueprint.radialMeshPoints

[ntouran]

• The traditional docstring linter is https://github.com/pycqa/pydocstyle
• We almost definitely can disable linting for code that is outside of our main tree (e.g. numpy)
• This is a problem for sure that we should fix. the examples just above are pretty bad.

[john-science]

I have been playing with this, this morning, and can't find the right flag to turn off the inherited docstrings. I'll keep playing with it.

We just have so many Sphinx extensions, I probably just need to find the one that is last in the chain.

[john-science]

Okay, I have managed to fix everything except the Yamlize objects. Progress.

[john-science]

PR is up: #628

[john-science]

Okay, the problem is generally fixed in the docs except for subclasses of yamlize.Attribute, which I have opened a ticket for here:

#629

[john-science]

An example of a fixed doc (zincOxide):

https://terrapower.github.io/armi/.apidocs/armi.materials.zincOxide.html