Sphinx Python Documentation

[StefanHabel]

Work in progress.

This PR adds a new build target named MaterialXDocsPython, which generates Python API documentation using Sphinx.

The existing developer guide contents are incorporated into the new HTML documentation, which lives side-by-side to the existing Doxygen-generated C++ API documentation.

Docstrings were added to all modules, functions, classes, methods, and attributes.

All parameters were named. None should appear as arg0, arg1, arg2 etc anymore.

Statistics about the Python API have been added to the Sphinx build process.

The Sphinx build output currently ends with the following messages, providing an overview of the state of the API docs:

The MaterialX Python API consists of:
    * 11 modules
    * 48 functions
    * 139 classes
    * 1180 methods
    * 6 exception types

build succeeded.

Docstring example:

$ python3 -c 'import PyMaterialXFormat; help(PyMaterialXFormat.flattenFilenames)'

Help on built-in function flattenFilenames in module PyMaterialXFormat:

flattenFilenames(...) method of builtins.PyCapsule instance
    flattenFilenames(doc: PyMaterialXCore.Document, searchPath: PyMaterialXFormat.FileSearchPath = mx.FileSearchPath(), customResolver: PyMaterialXCore.StringResolver = None) -> None

    Flatten all filenames in the given document, applying string resolvers at
    the scope of each element and removing all fileprefix attributes.

    :param doc: The document to modify.
    :type doc: Document
    :param searchPath: An optional search path for relative to absolute path
        conversion.
    :type searchPath: FileSearchPath
    :param customResolver: An optional custom resolver to apply.
    :type customResolver: StringResolver

Preview of generated HTML pages:

---

---

Development environment:

• Apple clang version 15.0.0 (clang-1500.0.40.1)
• Python 3.12.0 (v3.12.0:0fb18b02c8, Oct 2 2023, 09:45:56) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
• Sphinx 7.2.6
• cmake version 3.27.6

Update #342

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: StefanHabel / name: Stefan Habel (bef6e1f, 9d4b492, e129d65, 6a5da31, e1113ea, e4382bb, c983b87, 3567610, abc2edb, 133a0b4, 67bb241, e715c0f, 6a9d8e5, 2c04542, 8790578, 0439c07, 1e745bd, 4c11bde, 9d73ddc, 0e1e502, 805f13c, c0d65ed, ab6860b, 26a972f, b8eecc0, bf427e4, fbe2947, f064a22, d30276b, 86030d0, 90c78d4, aed7f8f, 052f38e, 939c996, 77b5944, be2db5e, 71bd3fd, f7bea44, e66a3f5, 28fd863, 74872ea, 572089b, 513ef90, c9a2469, 21eecdd, 40079f1, dfa106a, f62be3a, 77ca69c, 6253abc, c1b31c0, 0ca9f23, f73acc4, b589282)
• ✅ login: jstone-lucasfilm / name: Jonathan Stone (37f1ef5)

[kwokcb]

This is a great addition to have !
I left some minor niggly comments. The only real question I had is any inter-dependence on building the actual Python modules.
Thanks.

[kwokcb]

I think this should un un-indented and as a suggestion can be placed after the GenShader modules. Then it's easy to see that it has the most dependencies :).

[StefanHabel]

Thanks Bernard!

I've actually removed this part from the documentation again, after exploring solutions to the import order dependency issue, and finding something that appears to be working:

• e1113ea
• ab6860b

The approach is to import the required Python modules from the bindings of the Python modules that need them. This is similar to how we'd import required modules in Python code, implemented as a C++ macro definition using pybind11::module::import(). The macro supports imports relative to the current module, or as part of the MaterialX Python package:

// Define a macro to import a PyMaterialX module, e.g. `PyMaterialXCore`,
// either within the `MaterialX` Python package, e.g. in `installed/python/`,
// or as a standalone module, e.g. in `lib/`
#define PYMATERIALX_IMPORT_MODULE(MODULE_NAME)                               \
    try {                                                                    \
        pybind11::module::import("MaterialX." #MODULE_NAME);                 \
    }                                                                        \
    catch (const py::error_already_set&) {                                   \
        pybind11::module::import(#MODULE_NAME);                              \
    }

For example, the module code for PyMaterialXFormat uses it as follows:

    // PyMaterialXFormat depends on types defined in PyMaterialXCore
    PYMATERIALX_IMPORT_MODULE(PyMaterialXCore);

While this works in my dev environment (macOS), it is yet to be scrutinized and tested on other platforms.

[jstone-lucasfilm]

This sounds very promising, @StefanHabel, and this would potentially resolve a dependency issue that we've long wanted to address in MaterialX.

[kwokcb]

Doesn't need to be done with this PR, but as "accessibility" is a active area in ASWF, perhaps it's worthwhile to run an accessibility test on the page for the colors and fonts ?

[StefanHabel]

Oh, good point!

Given that we're aiming for some consistency in styles between the various MaterialX sites and docs, could we maybe make this a separate ticket, to look at accessibility across all of the docs?

[kwokcb]

Wanted to double check if there is any dependence on the actual building the Python modules as that is a build option?

I assume not, but just checking.

[StefanHabel]

Yes indeed, good point -- there is!

I've tweaked this in 9d4b492, to build the Python bindings when Python API documentation is requested to be built:

option(MATERIALX_BUILD_PYTHON_DOCS "Create HTML documentation using Sphinx. Requires that Sphinx be installed." OFF)
if (MATERIALX_BUILD_PYTHON_DOCS)
    set(MATERIALX_BUILD_PYTHON ON)
endif()

In the same commit, I've also added DEPENDS to the custom MaterialXDocsPython target:

        add_custom_target(
            MaterialXDocsPython
            ${SPHINX_EXECUTABLE}
                -c ${CMAKE_CURRENT_BINARY_DIR}
                ${SPHINX_SOURCE_DIR}
                ${SPHINX_HTML_OUTPUT_DIR}
            WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
            DEPENDS ${MATERIALX_PYTHON_DOCS_TARGET_DEPENDENCIES}
            COMMENT "Building MaterialX Python API Documentation: ${SPHINX_HTML_OUTPUT_DIR}/index.html"
        )

The variable that's referenced in DEPENDS is a list of targets for all Python C extension modules:

        set(
            MATERIALX_PYTHON_DOCS_TARGET_DEPENDENCIES
            MaterialXCore
            PyMaterialXCore
            PyMaterialXFormat
            PyMaterialXGenShader
            PyMaterialXGenGlsl
            PyMaterialXGenOsl
            PyMaterialXGenMdl
            PyMaterialXGenMsl
            PyMaterialXRender
            PyMaterialXRenderGlsl
            PyMaterialXRenderOsl
            PyMaterialXRenderMsl
        )

It seems to work well: when making changes to the docstrings in one of the Python C extensions, only that module is rebuilt, and the docs are regenerated.

I'm using the following command line while iterating on the docs, using rm to make sure that the docs are rebuilt from scratch (replaced with RM below for safety):

RM -rf  documents/DeveloperGuide/generated/  documents/html-sphinx/  && cmake --build . --target MaterialXDocsPython

While it seems to work, I'm happy to explore alternatives, if this isn't quite hitting the mark.

[kwokcb]

This looks good to me. Thanks for looking into it.

[kwokcb]

Not specific to this PR but I noticed that images don't show up properly on the PyPi package page for MaterialX.
If were adding more images, just curious is there a more robust way to specify image references -- such as an absolute reference to the web site location instead of using relative paths ?

[StefanHabel]

Oh, I see the issue: https://pypi.org/project/materialx/

It's great to see that the documentation content is reused in many places!

I've migrated the Markdown docs, including README.md, to use absolute paths now:

• e129d65

[jstone-lucasfilm]

In case it allows additional simplifications of this logic, I should note that the MaterialX project requires Python 3.6 or greater!

https://github.com/AcademySoftwareFoundation/MaterialX#supported-platforms

[jstone-lucasfilm]

@StefanHabel Starting with this edit, I'm seeing a great set of fixes to spelling, letter case, and other details in the C++ libraries for MaterialX. Would it make sense to separate these out into their own pull request, so that they can be reviewed independently of the new Python Documentation functionality in this proposal?

[StefanHabel]

yes, thanks @jstone-lucasfilm !

This PR is getting a little unwieldy.

We could generally split this into separate PRs:

1. Base functionality of generating the Python API Documentation via Sphinx.
2. Fixes in header files for typos encountered while copying docs in headers to docstrings in the Python bindings.
3. Added argument names and docstrings in the Python bindings.

We could treat this PR here (1567) as the one for the Sphinx base functionality, splitting off parts 2 and 3 into separate PRs.

The third part (adding the actual docstrings) could be broken apart further into four separate PRs for the main (groups of) MaterialX Python modules:

• PyMaterialXCore
• PyMaterialXFormat
• PyMaterialXGenShader
  • PyMaterialXGenGlsl
  • PyMaterialXGenOsl
  • PyMaterialXGenMdl
  • PyMaterialXGenMsl
• PyMaterialXRender
  • PyMaterialXRenderGlsl
  • PyMaterialXRenderOsl
  • PyMaterialXRenderMsl

While working on this, I also encountered a couple of issues in the bindings themselves:

• PyMaterialXCore.UnitDef.setUnitType() is wrongly bound to hasUnitType(): https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXCore/PyDefinition.cpp#L76-L77
• PyMaterialXCore.GeomPropDef: duplicate definitions can be removed: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXCore/PyGeom.cpp#L60-L71
  • setGeomProp()
  • hasGeomProp()
  • getGeomProp()
• PyMaterialXGenShader.HwShaderGenerator static functions are wrongly bound as instance methods: https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXGenShader/PyHwShaderGenerator.cpp#L31-L33
  • bindLightShader()
  • unbindLightShader()
  • unbindLightShaders()
• PyMaterialXRenderMsl.MetalTextureHandler.bindImage() is wrongly bound to unbindImage(): https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/PyMaterialX/PyMaterialXRenderMsl/PyMetalTextureHandler.mm#L17-L18

We could treat these issues in the Python bindings as a separate ticket and PR.

Here's a list of tickets and corresponding PRs in which we could organize the overall work here:

• New issue ticket for existing issues in Python bindings
  • New PR to address the issues
• Sphinx Python Documentation #342
  • Sphinx Python Documentation #1567 for the Sphinx part
  • Separate new PR for docstrings for PyMaterialXCore
  • Separate new PR for docstrings for PyMaterialXFormat
  • Separate new PR for docstrings for PyMaterialXGen{Shader,Glsl,Osl,Mdl,Msl}
  • Separate new PR for docstrings for PyMaterialXRender{,Glsl,Osl,Msl}
  • Separate new PR for fixes in header files

Alternatively:

• New issue ticket for existing issues in Python bindings
  • New PR to address the issues
• Sphinx Python Documentation #342
  • Sphinx Python Documentation #1567 for the Sphinx part and docstrings in all Python modules
  • Separate new PR for fixes in header files

I'd be happy with either approach.

I have more commits to add to this PR here, adding more argument names and docstrings. I'll keeping using this PR here for this for now.

[jstone-lucasfilm]

@StefanHabel One approach that could be useful here is to break out one separable change at a time from this larger pull request (e.g. fixes to C++ libraries), so that it can be reviewed and merged to MaterialX main on its own.

As each of these subsets is reviewed and merged to main, we can then click "Update Branch" on this larger pull request, effectively removing that subset from the main proposal.

This should help to focus reviewer attention on a single, independent change at a time, rather than considering the entire proposal at once.

[StefanHabel]

Understood! Sounds good!

I'll upload a few more commits to this PR here, to have everything in one place.

I'll then create separate PRs for subsets of this work, which can be reviewed and merged independently.