Skip to content

Changed namespace fix to use Sphinx autodoc-process-* events #7103

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 19, 2024
Merged

Changed namespace fix to use Sphinx autodoc-process-* events #7103

merged 2 commits into from
Dec 19, 2024

Conversation

timohl
Copy link
Contributor

@timohl timohl commented Dec 18, 2024 •
edited
Loading

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

Open3D uses namespaces open3d.cpu.pybind. and open3d.cuda.pybind. to differentiate between both device builds.
This adds a lot of clutter to the documentation (see screenshots below).
There is already a partial fix for functions documented using docstring::ClassMethodDocInject and docstring::FunctionDocInject.
However, a big part of Open3D does not use those functions for documentation and #7081 removed those from the contribute docs (so they should probably be avoided in the future).

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results       (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

This PR moves the namespace fix away from docstring::ClassMethodDocInject and docstring::FunctionDocInject to the sphinx documentation generation.
It uses the autodoc events autodoc-process-signature and autodoc-process-docstring to change namespaces to open3d. during doc generation.
With this PR all namespaces are fixed in the documentation (checked using the search function).

This is a fix used in #6917 since mypy would complain about missing imports for functions with fixed namespaces.

Notice in the following before/after screenshots that the namespaces in the clear function were already fixed due to using docstring::ClassMethodDocInject. This PR improves the namespaces in __init__ and as_tensor though, resulting in better readability.

Here before:
before
and after:
after

@update-docs Update Docs
Copy link

update-docs bot commented Dec 18, 2024

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@timohl
Copy link
Contributor Author

timohl commented Dec 18, 2024

Documentation CI was already failing in previous commits.
With the following error:

The following packages have unmet dependencies:
 cmake : Depends: libssl1.1 (>= 1.1.1) but it is not installable

The following is the function installing docs dependencies.
Maybe an issue in the kitware repository?
Not sure why this is even required.
Why not use the cmake from ubuntu repositories?

Open3D/util/ci_utils.sh

Lines 338 to 375 in 0ae7b1a

# Install dependencies needed for building documentation (on Ubuntu 20.04)
# Usage: install_docs_dependencies "${OPEN3D_ML_ROOT}"
install_docs_dependencies() {
echo
echo Install ubuntu dependencies
echo Update cmake needed in Ubuntu 20.04
sudo apt-key adv --fetch-keys https://apt.kitware.com/keys/kitware-archive-latest.asc
sudo apt-add-repository --yes 'deb https://apt.kitware.com/ubuntu/ focal main'
./util/install_deps_ubuntu.sh assume-yes
sudo apt-get install --yes cmake
sudo apt-get install --yes libxml2-dev libxslt-dev python3-dev
sudo apt-get install --yes doxygen
sudo apt-get install --yes texlive
sudo apt-get install --yes texlive-latex-extra
sudo apt-get install --yes ghostscript
sudo apt-get install --yes pandoc
sudo apt-get install --yes ccache
echo
echo Install Python dependencies for building docs
command -v python
python -V
python -m pip install -U -q "wheel==$WHEEL_VER" \
"pip==$PIP_VER"
python -m pip install -U -q "yapf==$YAPF_VER"
if [[ -d "$1" ]]; then
OPEN3D_ML_ROOT="$1"
echo Installing Open3D-ML dependencies from "${OPEN3D_ML_ROOT}"
python -m pip install -r "${OPEN3D_ML_ROOT}/requirements.txt"
python -m pip install -r "${OPEN3D_ML_ROOT}/requirements-torch.txt"
python -m pip install -r "${OPEN3D_ML_ROOT}/requirements-tensorflow.txt"
else
echo OPEN3D_ML_ROOT="$OPEN3D_ML_ROOT" not specified or invalid. Skipping ML dependencies.
fi
echo
python -m pip install -r "${OPEN3D_SOURCE_ROOT}/python/requirements.txt"
python -m pip install -r "${OPEN3D_SOURCE_ROOT}/python/requirements_jupyter_build.txt"
python -m pip install -r "${OPEN3D_SOURCE_ROOT}/docs/requirements.txt"
}

benjaminum
benjaminum approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@benjaminum benjaminum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timohl This is great! Thanks!

We are moving away from the *Inject functions to simplify writing docstrings.

@benjaminum benjaminum merged commit 8dbccb6 into isl-org:main Dec 19, 2024
38 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminum benjaminum benjaminum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timohl @benjaminum