Python auto doc #665

wrongbad · 2023-12-18T21:45:56Z

This is based on top of the "more numpy" branch, so look at just the top commit for now.

I threw together a quick proof of concept. Obviously the script can use a lot of refactoring to add more sources with less redundancy.

One pain point already is method overloading and how to generate keys/ids to reference them. I put a simple "_1", "_2" mechanism in, but this can cause problems when adding new overloads and docstrings silently get shifted to wrong functions. I looked at doxygen XML outputs and it seems they generate some arbitrary hash for overloads, so it would likely not be a stable reference from the binding cpp file.

I could also try parentheses matching to scoop the full function name, remove all whitespace and hash it, then you only need to update the key if the function signature changes.

codecov · 2023-12-18T21:55:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7d564cf) 91.66% compared to head (50ab9f7) 91.66%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #665   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files          37       37           
  Lines        4737     4737           
=======================================
  Hits         4342     4342           
  Misses        395      395

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pca006132 · 2023-12-18T23:46:30Z

For overloads, we can probably use the parameter names as keys. It will be nice if we can make some macro that set the nb::arg and fill in the parameter name in the docstring.

wrongbad · 2023-12-19T00:08:20Z

While parsing parameters names and types would be nice, I fear it quickly spirals into general-purpose c++ parsing, which is more than a 100-line python script can handle

pca006132 · 2023-12-19T00:14:32Z

I think the function signature is simple in our case, we can make sure that there are no parenthesis in the parameter type. This can make it readable using regex. We can just change our source file to make it easy to parse, instead of changing the script to parse whatever c++ madness.

bindings/python/examples/gyroid_module.py

wrongbad · 2023-12-19T23:44:53Z

Ah I'm reminded why I prefer to amend and force-push when working on PR. Rebasing this branch on master is a huge mess

elalish · 2023-12-20T08:10:10Z

No need to bother with rebasing - just use a regular merge. That's why we squash.

elalish · 2023-12-20T08:14:38Z

bindings/python/docstrings.inl

+#pragma once
+#include <string>
+namespace manifold_docs {
+const auto cross_section__area_f5458809be32 = R"___(Return the total area covered by complex polygons making up the


Is this file generated by your script? Would be great to have some documentation about how the system works and the right way to maintain it. Looks promising though!

Generated a comment in the file, and added comments in the gen_docs.py script.

elalish · 2023-12-21T08:52:28Z

This is looking great - is it ready for review?

zalo · 2023-12-21T09:03:56Z

Is the “_1”, “_2” overload handling mechanism still in there; how does it work? (I would be kind of sad if callers had to disambiguate overloads by appending the number of the overload to the function name… OpenCascade.js eventually went this direction for typescript annotations)

If that’s the case, perhaps a lesser evil would be marking non-intersecting args as optional?

pca006132 · 2023-12-21T11:07:47Z

I think the auto doc is just for documentation, it does not affect overload resolution in the runtime. And it is using the argument names as suffices to disambiguate different overloads, e.g. manifold__manifold vs manifold__manifold__mesh_gl__property_tolerance.

wrongbad · 2023-12-21T13:14:49Z

Yes I think it's ready for review.

And yes the _1 is gone and these overload names don't affect API, only binding sources for maintainers.

elalish

This looks great to me, thank you! So the CI builds these docs automatically and they'll get packaged up into our Python releases? How do we test them?

elalish · 2023-12-21T19:33:00Z

bindings/python/gen_docs.py

+
+# we don't handle inline functions in classes properly
+# so instead just white-list functions we want
+def select_functions(s):


Should we add something in a README somewhere that when a new non-member function is added to the API it needs to also be added to this list?

elalish · 2023-12-21T19:34:18Z

bindings/python/gen_docs.py

+
+
+collect(f"{base}/src/manifold/src/manifold.cpp", lambda s: method_re.search(s))
+collect(f"{base}/src/manifold/src/constructors.cpp", lambda s: method_re.search(s))


Perhaps also add to a README that when new files are added with API documentation, they need to also be added as a collect line?

elalish · 2023-12-21T20:14:41Z

bindings/python/gen_docs.py

+
+
+def method_key(name):
+    name = re.sub("\+", "_plus", name)


We might need to add a substitution of MeshGL to mesh, since several docs refer to this data structure by its C++ name.

wrongbad · 2023-12-22T02:10:06Z

@elalish What specifically are you hoping to test? At least right now empty doc-strings are omitted, which would result in compile errors if a code change caused one of them to get lost somehow. But other than that, I'm not sure automated testing can know if a doc-string is "correct".

I guess we could also run the nanobind-stubgen script on the bindings and check that the param names match the function signature.

elalish · 2023-12-22T02:37:50Z

Well, it doesn't have to be automation. I just feel like before I publish a release I'd like to be able to see the python docs in context and verify they look right. More of some README steps; maybe run the script and then hover over some python in VSCode and see if the doc string looks right? I just want to make sure I'm looking at the most recent version and such.

zalo · 2023-12-22T03:40:54Z

If all it takes to build locally is to just run CMake on this branch, then, I think I have a few examples:

Here's an example of Github Copilot giving suggestions based on context alongside the docs:

Also, it seems like sphere(), cube(), and hull_points() don't have docs... There are probably more missing...
Which is weird, because I see them here, so I know the script ran...
docstrings.inl.txt

pca006132 · 2023-12-22T05:04:05Z

I think for some language servers you need the stub file.

wrongbad · 2023-12-22T22:02:45Z

For basic human QA of python docs, I would recommend the following:

pip install nanobind-stubgen
# in manifold repo root
pip install . 
nanobind-stubgen manifold3d

Open manifold3d.pyi and read it.

elalish · 2023-12-23T10:13:06Z

Thanks @wrongbad, but nanobind-stubgen manifold3d gave me WARNING:root:Function is not valid python code: Initialize self. See help(type(self)) for accurate signature.

Would you mind adding these directions to your README? I'm guessing there's a step missing, or my python setup is weird somehow. @zalo does it work for you now? Would also be great if you could paste manifold3d.pyi into a comment here so others can review it easily.

pca006132 · 2023-12-23T10:25:17Z

I also got the same warning, it seems that it is related to enum codegen, which I guess is complaining the lack of self argument in __init__.

class JoinType:
    """
    <attribute '__doc__' of 'JoinType' objects>
    """

    @entries: dict
    
    Miter: JoinType
    
    Round: JoinType
    
    Square: JoinType
    
    def __init__(*args, **kwargs):
        """
        Initialize self.  See help(type(self)) for accurate signature.
        """
        ...

There are also other issues, copying (and updating) my previous comment to here:

Does not annotate static methods.
Does not annotate every overloaded method. I think it does not annotate the first one and jedi is not happy about this.
Cannot figure out the type for things like Floatx3
If the return type is a tuple, e.g. tuple with 6 floats, it can only infer tuple as the return type.

pca006132 · 2023-12-23T10:28:52Z

I think for now we can use a postprocessor to fix some of the issues, and open a PR in the upstream to make the real fix later.

wrongbad · 2023-12-23T13:16:41Z

Yeah I get the warnings too but it seems non-critical. I updated the readme and included a simpler option as well:

pip install .
python -c 'import manifold3d; help(manifold3d)'

elalish · 2023-12-24T09:06:48Z

@pca006132 It seems like most of what you're referring to is working for me, unless I'm not understanding this right:

overloads:

    class Manifold(builtins.object)
     |  Methods defined here:
     |  
     |  __add__(...)
     |      __add__(self, arg: manifold3d.Manifold, /) -> manifold3d.Manifold
     |      
     |      Shorthand for Boolean Union.
     |  
     |  __init__(...)
     |      __init__(self) -> None
     |      __init__(self, mesh: manifold3d.Mesh, property_tolerance: list[float] = []) -> None
     |      
     |      Overloaded function.
     |      
     |      1. ``__init__(self) -> None``
     |      
     |      Construct an empty Manifold.
     |      
     |      2. ``__init__(self, mesh: manifold3d.Mesh, property_tolerance: list[float] = []) -> None
``
     |      
     |      Convert a Mesh into a Manifold, retaining its properties and merging only
     |      the positions according to the merge vectors. Will return an empty Manifold
     |      and set an Error Status if the result is not an oriented 2-manifold. Will
     |      collapse degenerate triangles and unnecessary vertices.
     |      All fields are read, making this structure suitable for a lossless round-trip
     |      of data from GetMesh. For multi-material input, use ReserveIDs to set a
     |      unique originalID for each material, and sort the materials into triangle
     |      runs.
     |      :param mesh: The input Mesh.
     |      :param property_tolerance: A vector of precision values for each property
     |      beyond position. If specified, the propertyTolerance vector must have size =
     |      numProp - 3. This is the amount of interpolation error allowed before two
     |      neighboring triangles are considered to be on a property boundary edge.
     |      Property boundary edges will be retained across operations even if the
     |      triangles are coplanar. Defaults to 1e-5, which works well for most
     |      properties in the [-1, 1] range.

static methods:

     |  Static methods defined here:
     |  
     |  __new__(*args, **kwargs) from nanobind.nb_type_0
     |      Create and return a new object.  See help(type) for accurate signature.
     |  
     |  ----------------------------------------------------------------------
     |  Data and other attributes defined here:
     |  
     |  batch_hull = <nanobind.nb_func object>
     |      batch_hull(manifolds: list[manifold3d.Manifold]) -> manifold3d.Manifold
     |      
     |      Compute the convex hull enveloping a set of manifolds.
     |      :param manifolds: A vector of manifolds over which to compute a convex hull.

This is looking pretty decent to me - @pca006132 should we merge this, or is there some kind of regression you see?

pca006132 · 2023-12-24T09:15:20Z

ok, maybe it is a version problem on my side, I installed it earlier. yeah we can merge this.

elalish

I'm going to assume this is all good - it certainly cleans up our code nicely. I wish I knew Python well enough to feel some confidence in verifying it, but I'll assume our users will file issues if they notice problems. Thank you!

* python autogen docstrings * use function signature hashes in docstring key * remove sys.path debug code * docstrings use param names * autogen docstrings during cmake * document gen_docs.py * more readme, remap mesh_gl references * readme about verify python docstrings, autogen depend on sources

zalo reviewed Dec 19, 2023

View reviewed changes

bindings/python/examples/gyroid_module.py Outdated Show resolved Hide resolved

8000

wrongbad added 3 commits December 19, 2023 18:48

python autogen docstrings

fc1e9a0

use function signature hashes in docstring key

14baa55

remove sys.path debug code

9e97798

wrongbad force-pushed the python_auto_doc branch from 69ef783 to 9e97798 Compare December 19, 2023 23:48

elalish reviewed Dec 20, 2023

View reviewed changes

wrongbad added 2 commits December 20, 2023 18:13

docstrings use param names

2f36394

autogen docstrings during cmake

72ab01d

document gen_docs.py

99f0c30

elalish reviewed Dec 21, 2023

View reviewed changes

more readme, remap mesh_gl references

ce63a83

readme about verify python docstrings, autogen depend on sources

50ab9f7

elalish approved these changes Dec 26, 2023

View reviewed changes

elalish merged commit 54fa822 into elalish:master Dec 26, 2023

elalish mentioned this pull request Jan 7, 2024

v2.3 #685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python auto doc #665

Python auto doc #665



		collect(f"{base}/src/manifold/src/manifold.cpp", lambda s: method_re.search(s))
		collect(f"{base}/src/manifold/src/constructors.cpp", lambda s: method_re.search(s))

Python auto doc #665

Python auto doc #665

Conversation

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment