Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorporation of TensorStore Python autosummary extension #91

Closed
jbms opened this issue May 3, 2022 · 14 comments · Fixed by #99
Closed

Incorporation of TensorStore Python autosummary extension #91

jbms opened this issue May 3, 2022 · 14 comments · Fixed by #99

Comments

@jbms
Copy link
Owner

jbms commented May 3, 2022

I would like to potentially migrate the TensorStore autosummary extension into this theme, but I wanted to get feedback as to whether that would be useful to anyone else.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

Does the autosummary customization span C++ and python or just python? Previous discussions have been a bit convoluted about the feature.

Is there a module in the tensorstore repo that I can inspect? I dislike sounding like a noob when I ask so many questions.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

oops, disregard my first question. I see the new issue (#92) now

@jbms
Copy link
Owner Author

jbms commented May 3, 2022

This is the module in TensorStore:

https://github.com/google/tensorstore/blob/master/docs/tensorstore_sphinx_ext/autosummary.py

It includes some of the functionality that I recently ported over to this theme, like parameter cross linking and synopses, so that would be removed.

It has some tensorstore stuff hard coded in it that would need to be changed to config options.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

Wow! 1500 lines is a bit much for a glance. Are you working from a local branch that has the recent abstractions applied?

@jbms
Copy link
Owner Author

jbms commented May 3, 2022

I haven't yet done that. I just barely started but then figured I'd file this issue to see whether it makes sense to incorporate it.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

Well, I'm open to any addition here. I imagine that it wouldn't collide with the well-known autosummary ext because that is namespaced already (sphinx.ext.autosummary).

Considering #92, I wonder if you'd namespace the tensorstore autosummary for python in sphinx_immaterial.py_ext.autosummary or just prefix the module with the domain like sphinx_immaterial.py_autosummary. The sphinx_immaterial root package is starting to look pretty cluttered...

@jbms
Copy link
Owner Author

jbms commented May 3, 2022

I was planning to call them python_apigen and cpp_apigen. But open to suggestions. I agree it could make sense to group by language.

Possibly:

sphinx_immaterial.domains.python.xxx

sphinx_immaterial.domains.cpp.xxx

sphinx_immaterial.domains.json.xxx

etc.

One notable capability of the Python autosummary is support for pybind11 overloaded functions (which get documented as separate functions).

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

One notable capability of the Python autosummary is support for pybind11 overloaded functions (which get documented as separate functions).

oh you tease! 🤣 😍 But that requires an overload id though, right?

What about grouping by ext?

  • sphinx_immaterial.autosummary.python
  • sphinx_immaterial.autosummary.cpp

@jbms
Copy link
Owner Author

jbms commented May 3, 2022

With the current implementation, the overload ids can be left out but then you get a warning and the functions will display as func(1), func(2) etc. in the toc, and the page name will similarly include the number. Potentially there are some other options for generating the toc titles and page names, but requiring manual IDs seemed like the best option.

Note that the C++ autosummary extension similarly uses manually-defined overload ids.

I was thinking of organizing by language rather than function, since we already have a number of different python and.cpp-specific extensions. Also currently the two autosummary extensions are completely independent in their implementation, even though they produce similar output. But I can see the benefit of grouping the two autosummary extensions.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 3, 2022

I'm not against grouping by domain. It seems appropriate if the refactor will include the json domain. For backward compatibility, we could modify the __init__.py file to maintain the shorter namespaces that we're currently endorsing in the docs.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jun 7, 2022

I just had a thought. Would it be possible to build from stub files? I know its a rather specific use case (mainly for python c-extensions), but it would be an awesome feature that autodoc cannot (or will not) support.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jun 23, 2022

I would still prefer an option to allow consolidating documented members on scope/group page instead of putting each member's endpoint documentation in its own file.

@jbms
Copy link
Owner Author

jbms commented Jun 23, 2022

The extension ultimately still uses autodoc so is mostly limited by what it can do. Maybe we could use a custom loader to convince Python to load the stub file as modules.

An option to consolidate members makes sense as well --- I think that could easily be added as an additional option after the initial version, though.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jun 23, 2022

The stub files isn't too important for me, but it is a highly requested feature among C-ext devs with autodoc. Maybe when they get around to introducing the feature there, we can extend it to this.

Tagging sphinx-doc/sphinx#7630 for reference

@jbms jbms closed this as completed in #99 Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants