astropy / sphinx-automodapi

Sphinx extension for generating API documentation
https://sphinx-automodapi.readthedocs.io
BSD 3-Clause "New" or "Revised" License
63 stars 45 forks source link

Incompatible with sphinx>4 (submodules of submodules) #130

Closed pllim closed 1 year ago

pllim commented 3 years ago

Not sure what is happening but we need to make sure it is not a bug here before reporting it upstream to sphinx. This is blocking astropy/astropy#11763 .

@larrybradley narrowed down the issue in https://github.com/astropy/astropy/issues/11723#issuecomment-838733176


I think this is a real issue. I'm getting this same error in photutils. It appears to be due to submodules of submodules:

mod1
    __init__.py  (from mod2 import *)
    mod2
       __init__.py (from a import *)
       a.py (defines myfunc, and __all__ = ['myfunc'])

In this case, Sphinx is now defining duplicate references as: package.mod1.myfunc and package.mod1.mod2.myfunc and raising the "duplicate object descriptions".

This started with Sphinx 4 release. Not sure if this was an unintended bug there or if something needs to be fixed in astropy-sphinx for compatiblity with Sphinx 4.

astrojuanlu commented 3 years ago

I'll have a look at this the coming week ✋🏼

pllim commented 3 years ago

🙏 Thank you! 🙏

astrojuanlu commented 3 years ago

xref: https://github.com/sphinx-doc/sphinx/issues/9121 and https://github.com/sphinx-doc/sphinx/pull/9128/

pllim commented 3 years ago

I'll close this after I confirm that the unpinning would work again. Thanks for the heads up!

astrojuanlu commented 3 years ago

To clarify, this patch was released in v4.0.0b2, and it's present in all versions since then. @pllim I misled you into thinking that our bug https://github.com/astropy/sphinx-automodapi/issues/130 would be fixed by this, but it's the opposite: it's caused by it.

In the case of photutils, these are the affected classes: photutils.psf.matching.windows.CosineBellWindow, photutils.psf.matching.windows.HanningWindow, photutils.psf.matching.windows.SplitCosineBellWindow, photutils.psf.matching.windows.TopHatWindow, photutils.psf.matching.windows.TukeyWindow.

It's interesting that photutils.psf.matching.fourier functions do not seem to be affected. I added a photutils.psf.matching.fourier.TESTCLASS class, and this also triggered a warning. Not sure why classes and functions work differently on this.

Will add more information soon.

pllim commented 3 years ago

@astrojuanlu , any update from the Sphinx side? If not, we might implement a workaround so we can remove the pin.

astrojuanlu commented 3 years ago

Sorry I didn't have time to have a deeper look at this. I can do the week of August 9th, feel free to proceed with a workaround if you're in a hurry.

pllim commented 3 years ago

No hurry (yet). Thanks for the update!

astrojuanlu commented 3 years ago

See some analysis here https://github.com/astropy/photutils/pull/1238

maxnoe commented 2 years ago

I have a minimal sphinx project producing the issue here:

https://github.com/maxnoe/sphinx-automodapi-duplicated-warning

pllim commented 2 years ago

@eteq said he was gonna look at this... any update, Erik? Thanks!

If nothing is happening here, see astropy/photutils#1306 for downstream fix.

Related astropy PRs: astropy/astropy#12270 and astropy/astropy#13015

saimn commented 2 years ago

So it seems we should somehow generate the autodoc templates using :canonical: for the classes etc. that are duplicated: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#directive-option-py-class-canonical (But I have no idea how to do that)

maxnoe commented 2 years ago

This needs to go into the output of sphinx-automodapi somehow, which in turn seems to rely on sphinx-autodoc. Which doesn't seem to forward options to the class directive.

E.g. in ctapipe/docs/api I have three files for the same class cause it is imported in two __init__.py:

ctapipe.calib.camera.calibrator.CameraCalibrator.rst
ctapipe.calib.camera.CameraCalibrator.rst
ctapipe.calib.CameraCalibrator.rst

These are the files autogenerated by automod-api and contain rst instructions using sphinx.ext.autodoc: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-autoclass:

CameraCalibrator
================

.. currentmodule:: ctapipe.calib.camera

.. autoclass:: CameraCalibrator
   :show-inheritance:

   .. rubric:: Attributes Summary

   .. autosummary::

      ~CameraCalibrator.apply_peak_time_shift
      ~CameraCalibrator.apply_waveform_time_shift
      ~CameraCalibrator.data_volume_reducer_type
      ~CameraCalibrator.image_extractor_type

   .. rubric:: Methods Summary

   .. autosummary::

      ~CameraCalibrator.__call__

   .. rubric:: Attributes Documentation

   .. autoattribute:: apply_peak_time_shift
   .. autoattribute:: apply_waveform_time_shift
   .. autoattribute:: data_volume_reducer_type
   .. autoattribute:: image_extractor_type

   .. rubric:: Methods Documentation

   .. automethod:: __call__

So probably we should make try to get autodoc to include the :canonical: path of the class when it's run?

maxnoe commented 2 years ago

autodoc has apparently a way to include that:

https://github.com/sphinx-doc/sphinx/blob/b4276edd848d112b4e981011c334d27cbcb20018/sphinx/ext/autodoc/__init__.py#L1674-L1676

Not sure how to trigger it though...

Commit adding this: https://github.com/sphinx-doc/sphinx/commit/acf66bc4d5b53189f893a50a235e710f063d629d

saimn commented 2 years ago

The method you pointed @maxnoe (get_canonical_fullname) is correctly executed. But then it goes through

https://github.com/sphinx-doc/sphinx/blob/b4276edd848d112b4e981011c334d27cbcb20018/sphinx/domains/python.py#L1218-L1237

which triggers the warning.

If I take your reproduction project (very useful btw !), the generated docs seems correct. First we have a duplicate for duplicated.Foo:

.. py:class:: Foo()
   :module: duplicated
   :canonical: duplicated.foo.foo.Foo

   Bases: :py:class:`object`

   Awesome Foo class

And then another one for duplicated.foo.Foo (which is the ones triggering the warning):

.. py:class:: Foo()
   :module: duplicated.foo
   :canonical: duplicated.foo.foo.Foo

   Bases: :py:class:`object`

   Awesome Foo class

In the method I pointed above there is a dict with the duplicates:

(Pdb++) pp self.objects
{'duplicated.Foo': ObjectEntry(docname='api/duplicated.Foo', node_id='duplicated.Foo', objtype='class', aliased=False),
 'duplicated.foo.Foo': ObjectEntry(docname='api/duplicated.foo.Foo', node_id='duplicated.foo.Foo', objtype='class', aliased=False),
 'duplicated.foo.foo.Foo': ObjectEntry(docname='api/duplicated.Foo', node_id='duplicated.Foo', objtype='class', aliased=True)}

So I'm thinking that this may actually be a bug in Sphinx which correctly handles the case of one duplicate but not when there are more than one ?

saimn commented 2 years ago

See https://github.com/sphinx-doc/sphinx/issues/10348#issuecomment-1100513041= for a very helpful and detailed description of the problem and the different options. Maybe we could add an option in automodapi to add :noindex: when necessary ?

maxnoe commented 2 years ago

Reading the comment, I think a good solution (however I don't know how easy it would be) would be 2):

  1. Enable sphinx_automodapi to add the :noindex: option sometimes. This way, you can still use the extension but the newly documented objects are not added to the index, thus not exactly cross-referenceable and would not trigger any warning.

What do you think about the feasibility of adding a :no-index: option to .. automod-api that is a list of objects for which :no-index should be added?

E.g. for the minimal example you'd put:

.. automodapi:: duplicated.foo
   :no-index: duplicated.foo.Foo, duplicated.foo.Bar

?