Built API docs from the docstrings using Sphinx+autodocs

[thisisrick25]

Introduce a script to fetch external repositories for the Brainglobe project, update Sphinx configuration for improved documentation paths and extensions, and enhance the API documentation for the brainglobe-atlasapi module. Include setup instructions for Python and external packages in the Sphinx build workflow.

closes #4

[thisisrick25]

@adamltyson I have added docs only for brainglobe-atlasapi, for now.
Sphinx cannot directly import and document code from remote GitHub repositories. It requires the actual Python packages/source code to be present locally so it can import them and extract docstrings.
I wrote fetch_external.py, which will clone and install the BG repos and added it to the workflow.

[adamltyson]

Thanks @thisisrick25. Do you need any feedback at this stage? Or shall we wait until all the repos are included in this PR?

[thisisrick25]

I am facing a problem @adamltyson. I can generate the docs in my local environment, but when I implement them in GA workflow, neuroinformatics-unit/actions/build_sphinx_docs interferes with my additional action setup. The action performs its own checkout potentially into a clean state/overwriting the workspace, not preserving the external dir.

One way I have yet to try is to write a completely different workflow action independent of neuroinformatics-unit/actions/build_sphinx_docs.

[adamltyson]

That action is maintained by us (https://github.com/neuroinformatics-unit/actions/tree/main/build_sphinx_docs), so if you can work out what needs to be changed, you're welcome to submit a PR.

[thisisrick25]

I will open a new issue @neuroinformatics-unit/actions

edit: issue neuroinformatics-unit/actions#83

[thisisrick25]

@adamltyson I want to point out that the docstring syntax is inconsistent in brainglobe-atlasapi (have not checked in other repos). I got this warning locally

which also throws an error in GA because of this -W flag (see logs here)

I checked by fixing the syntax and can confirm it works as expected.

Do we need the -W flag to build Sphinx doc for the API workflow? My conscience says yes, but it will take some time to fix the docstrings.

I'm not very familiar with how pre-commit works, but I think it might be possible to create a hook with the numpydoc repo to check the docstring for numpydoc validation. This would be helpful in the future.

[alessandrofelder]

Thanks @thisisrick25 - you make some good points here.

I want to point out that the docstring syntax is inconsistent in brainglobe-atlasapi (have not checked in other repos).

Indeed, this is expected (a consequence of BrainGlobe's "organic" growth) and would be nice to fix.

Do we need the -W flag to build Sphinx doc for the API workflow? My conscience says yes, but it will take some time to fix the docstrings.

I agree.

I'm not very familiar with how pre-commit works, but I think it might be possible to create a hook with the numpydoc repo to check the docstring for numpydoc validation. This would be helpful in the future.

I don't know either, but agree it should be possible, and would be helpful.

To help keep things tidy and manageable, I would tackle each point in a separate PR, in the following order

1. a PR that only fixes docstrings
2. this PR (and make sure things work)
3. a pre-commit PR (if possible)

Does that make sense, and would you have capacity to try (for this repo only)?

[thisisrick25]

@alessandrofelder Yes I will be happy to help.

[thisisrick25]

I want to address a few more things:

1. What should be the URL path to the API reference? i.e where should the api dir be located? Outside or embedded in the documentation dir? (Current config is outside)

   If outside, clicking on the API reference link will change the URL path from documentation/brainglobe-atlasapi/index.html to api/brainglobe_atlasapi.html. If embedded, then documentation/brainglobe-atlasapi/api/brainglobe_atlasapi.html.

   My recommendation would be to embed the API reference within the brainglobe-atlasapi documentation structure.

2. I think we should be able to run the Sphinx makefile/bash script from the root directory instead of changing to the docs folder first

[IgorTatarnikov]

1. I think the embedded URL has a better structure
2. I think we keep the make as is, https://github.com/neuroinformatics-unit/actions/blob/94f7334d4334c921e672a7ea48674f57ed7d0893/build_sphinx_docs/action.yml#L84C1-L85C29 the build action runs make html from inside the docs directory.

[thisisrick25]

1. I think the embedded URL has a better structure

@IgorTatarnikov I have fixed the file structure, added some extra comments and print statements for better verbosity.

Are any of them in a decent state to go online other than the atlas API once brainglobe/brainglobe-atlasapi#584 is merged?

@adamltyson Unfortunately, no other repo other than brainglobe/brainglobe-atlasapi PR #584 has a correct docstring format.

Otherwise, this PR and brainglobe/brainglobe-atlasapi#584 are good to go.

[IgorTatarnikov]

Looks good! Now that brainglobe/brainglobe-atlasapi#584 is merged we should be good to merge this once the list of packages is restricted.

[thisisrick25]

Only brainglobe-atlasapi will be fetched, for now.

[IgorTatarnikov]

Perfect, to avoid trying to resolve an environment where all packages required for atlas packing are present it might be best to not generate API docs for the atlas_generation scripts. I'll track this in brainglobe/brainglobe-atlasapi#624, once this is resolved, this PR can be merged as well.

[IgorTatarnikov]

I've done some final local testing, now that brainglobe/brainglobe-atlasapi#624 is merged, we need to add atlas_scripts to the excluded directories in make_api_index.py.

Also, I'm seeing quite a few warnings thrown specifically for Class docstrings:

/path/to/repo/brainglobe.github.io/docs/downloads/brainglobe-atlasapi/brainglobe_atlasapi/bg_atlas.py:docstring of brainglobe_atlasapi.bg_atlas.BrainGlobeAtlas:79: WARNING: autosummary: stub file not found 'brainglobe_atlasapi.bg_atlas.BrainGlobeAtlas.check_latest_version'. Check your autosummary_generate setting.

I'm also seeing a duplicate method section in the class docs:

I was able to fix both of these issues by adding numpydoc_show_class_members = False to the conf.py file, but I'm not sure that's the best/only solution.

[IgorTatarnikov]

Also, some of the mesh_generation functions remain undocumented due to failed imports. Perhaps we could install the dev extras for each package, and ensure that the dev extras for each package contain all versions of the install that need to be documented.

E.g. in brainglobe-atlasapi we could change pyproject.toml so that when dev is install atlasgen is also fetched:

dev = [
    "black",
    "check-manifest",
    "coverage",
    "mypy",
    "pre-commit",
    "pytest-cov",
    "pytest-mock",
    "pytest",
    "ruff",
    "setuptools_scm",
    "tox",
	"brainglobe-atlasapi["atlasgen"],
]

[thisisrick25]

I was able to fix both of these issues by adding numpydoc_show_class_members = False to the conf.py file, but I'm not sure that's the best/only solution.

Turns out these warnings happened because both sphinx.ext.napoleon and numpydoc Sphinx extensions are used.
Removing numpydoc extensions removes the class warnings.

I also added a new attributes section in the class template because numpydoc was responsible for it.

[thisisrick25]

Also, some of the mesh_generation functions remain undocumented due to failed imports. Perhaps we could install the dev extras for each package, and ensure that the dev extras for each package contain all versions of the install that need to be documented.

This should be addressed in a new issue at brainglobe-atlasapi.

[IgorTatarnikov]

Looks good on my end, I've tested it again with a local build. LGTM 🎉