Add support for python-only projects

[aprotyas]

Previously, rosdoc2 would not generate any documentation for projects without doxygen - more concretely, projects with the ament_python build type - as reported in #19.

This PR attempts to resolve that issue. In offline discussions with @clalancette and @wjwwood, we decided that rosdoc2 can exhibit different behavior for different build types. See this comment for an outline of said behaviors.

Specifically, this PR:

• Adds a mandatory build_type configuration option, which defaults to the build_type obtained from the package.xml file. The set of available build_types is [ament_cmake, cmake, ament_python].
• Change behavior of the doxygen_builder and breathe/exhale extensions for ament_python build types: For this build type, doxygen does not need to be invoked whatsoever. Similarly, the breathe/exhale extensions to sphinx need not be registered since there is no doxygen-generated tag file to process.
• Create toctree entries for the default index.rst specific to each build type.
• Add an optional python_source configuration option. This option is represented as a path relative to the package's package.xml file, and should really only be used for packages that do not follow the standard Python package layout. This defaults to [package_name].
• Execute the sphinx-apidoc tool for ament_python build types so that documentation for Python modules can be automatically generated before sphinx-build is invoked.

Signed-off-by: Abrar Rahman Protyasha [email protected]

[aprotyas]

Pinging @clalancette and @wjwwood for a review.

[wjwwood]

I'm not sure about this mixed setting. What I was imagining is a default behavior for ament_cmake (+ doxygen/exhale/breathe, - sphinx-apidoc), a different one for ament_python (- doxygen/exhale/breathe, + sphinx-apidoc), and nothing special for all other package build types, but any of them could manually enable/disable things.

So if an ament_cmake package wanted to build python API docs too, then they would just enable that explicitly in the rosdoc2_settings dict, rather than change their type to mixed.

I can see the advantage to more "configuration templates", but it just feels like overkill at the moment, since the 90% case will be either ament_cmake with only C/C++ or ament_python with only Python. Everyone else in the meantime would need some special configurations, but if we see a common pattern emerging, then we could add a new "doc build type". As it stands, do you know of any packages that would select mixed right now?

Another thing to consider is packages that generate messages, but since we're not building first at the moment, that's kind of depending on that. We may have to change the logic more when that happens because right now all packages that have messages need to be ament_cmake but also will generate Python code for those messages. But again, I think we should defer that concern for now.

[clalancette]

I'm not sure about this mixed setting. What I was imagining is a default behavior for ament_cmake (+ doxygen/exhale/breathe, - sphinx-apidoc), a different one for ament_python (- doxygen/exhale/breathe, + sphinx-apidoc), and nothing special for all other package build types, but any of them could manually enable/disable things.

Yeah, agreed on that. The other problem with 'mixed' is that if we add another documentation type (say for ament_java packages), then it is no longer clear what 'mixed' refers to. Let's just go for the ament_cmake/ament_python split for now, and we can incrementally add other types as needed. And anyone who needs to do more than one thing in their package can manually configure it.

[aprotyas]

It seems like the mixed setting adds more ambiguity than it removes, particularly when considering the existence of other build types (such as ament_java). I can remove that from this PR, instead maintaining an ament_cmake/ament_python split only.

As it stands, do you know of any packages that would select mixed right now?

The only such package I can think of off the top of my head is message_filters.

So if an ament_cmake package wanted to build python API docs too, then they would just enable that explicitly in the rosdoc2_settings dict, rather than change their type to mixed.

By "enable that explicitly", do you mean there should be keys run_sphinx_apidoc and run_doxygen (or generate_py_docs/generate_cpp_docs) which we should account for when determining default behavior for a build type? For example, for an ament_cmake package with run_sphinx_apidoc set to true, sphinx-apidoc will be excecuted?

[aprotyas]

Regardless of the mixed setting, I've seen some packages that do not house the package source under the {package_name} directory - for example, tf2_ros_py's source is placed under tf2_ros. I added the optional python_source setting in the rosdoc2_settings dict for situations like that (check 6612754). Should this stay as is? Is there a better name (or a better place) for such an option?

[aprotyas]

I've dropped the mixed build type setting in 2977f35. In fact, the build_type information is no longer configurable (d5b4da9), since I've added the optional settings run_doxygen and run_sphinx_apidoc (5feb103 and d467c73 respectively) which allow overriding of expected/default behaviors for a given build type. More explicitly, setting those options to true allows their namesake tool to be invoked regardless of build type - so, an ament_cmake package could generate Python API documentation if it set run_sphinx_apidoc to true. Run rosdoc2 on ros2/message_filters:aprotyas/rosdoc2 for one such demonstration.

I've also patched up the toctree entries to distinguish between Python/C++ API headings in e43f3ab, just a small QOL thing.

Thoughts?

[clalancette]

I've left a few minor things to fix up. Mostly I want to make sure that this will continue to work if a new documentation type is added.

Separately, I tried running this against 'launch', and it did indeed work (which is great). But I got quite a few errors when running, of the form:

WARNING: autodoc: failed to import module 'substitutions.this_launch_file' from module 'launch'; the following exception was raised:
No module named 'launch'

[aprotyas]

Thank you for the review again @clalancette, I've committed some of the requested changes and addressed the feedback in commits e188b2d through ef8840e.

---

On the parallel discussion about running against the launch package. I just tried to do so locally, and it did work. I too got a bunch of errors but nothing along the lines of "failed to import module". The errors that I'm seeing are more problems in .rst markup within launch and duplicate references to some symbols, here's a snippet of the warnings:

Short snippet of the warnings from running rosdoc2 on launch

/home/abrar/ros2_rolling/build/launch/launch/event_handlers/event_named.py:docstring of launch.event_handlers.event_named:1: WARNING: duplicate object description of launch.event_handlers.event_named, other instance in launch.event_handlers, use :noindex: for one of them
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.extract_type:12: WARNING: Unexpected indentation.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.is_substitution:6: WARNING: Unexpected indentation.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.is_typing_list:4: WARNING: Unexpected section title.

Examples
--------
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:7: WARNING: Title underline too short.

Example:
-------
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:7: WARNING: Unexpected section title.

Example:
-------
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:10: WARNING: Unexpected indentation.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:8: WARNING: Inline literal start-string without end-string.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:8: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:17: WARNING: Block quote ends without a blank line; unexpected unindent.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:17: WARNING: Inline literal start-string without end-string.
/home/abrar/ros2_rolling/build/launch/launch/utilities/type_utils.py:docstring of launch.utilities.type_utils.normalize_typed_substitution:17: WARNING: Inline interpreted text or phrase reference start-string without end-string.
looking for now-outdated files... none found
pickling environment... done
checking consistency... /home/abrar/Documents/open_robotics/code/ros2_humble/src/launch/launch/doc/source/modules.rst: WARNING: document isn't included in any toctree
done
preparing documents... done
writing output... [100%] modules                                                               
/home/abrar/ros2_rolling/build/launch/launch/launch_description_sources/any_launch_file_utilities.py:docstring of launch.launch_description_sources.any_launch_file_utilities.get_launch_description_from_any_launch_file:: WARNING: more than one target found for cross-reference 'InvalidLaunchFileError': launch.InvalidLaunchFileError, launch.invalid_launch_file_error.InvalidLaunchFileError, launch.launch_description_sources.InvalidLaunchFileError

Regardless, when I was working on this PR I did consider multiple ways to mitigate errors like "can't find module", because sphinx-apidoc requires all modules to be in the system path. I settled on invoking sphinx-apidoc with the -a flag, which appends module_path to sys.path. I'm not sure why you got those warnings @clalancette. Any clues/thoughts?

EDIT:
Never mind, it looks like launch exists in my sys.path because it's been sourced from my workspace's underlay. If I remove it from the underlay, I get the same warning that you do. It looks like sphinx-apidoc only uses the -a flag when it is used to generate a whole project, so that's out of the question.

One option to mitigate this is through manually calling sys.path.insert(0, os.path.abspath('path/to/module')) for all modules returned by a setuptools.find_package('path/to/module'). Thoughts?

[wjwwood]

One option to mitigate this is through manually calling sys.path.insert(0, os.path.abspath('path/to/module')) for all modules returned by a setuptools.find_package('path/to/module'). Thoughts?

I was going to suggest this a while ago but got caught up in a review too.

It's in the sphinx-autodoc examples that you may need to add the local module directories to the sys.path for autodoc to work.

[aprotyas]

Here's what's changed since the latest round of feedback:

• The exhale/breathe settings are unconditionally checked now (previously, they were only checked if the build type was ament_cmake or cmake). The only difference is that the default value for the enable_<breathe/exhale> settings is not always True anymore. The default value is only True if the user has enabled always_run_doxygen or if the build type is ament_cmake or cmake. (81f9474)
• Formatting changes within the builder classes: more legible indentation of if-conditions, limiting line length, etc. (3379546, 8d61f5b, 1d7e75d, 51e11ed)
• Made the generated conf.py file less "cryptic" for the user, i.e. format mapped template variables into local variables before using them. (f2415cf)
• Initialized some rosdoc2 settings being propagated through the BuildContext class with reasonable default values. (0642f05)
• Inserted the package source directory into sys.path and provided sphinx-autodoc a list of exec_depends it could patch were they not present in sys.path. More about this below. (cecccee, eef02f1)
• Fixed up logic to check that the correct package source directory was identified. (final patch in 5d84071)

---

One option to mitigate this is through manually calling sys.path.insert(0, os.path.abspath('path/to/module')) for all modules returned by a setuptools.find_package('path/to/module'). Thoughts?

I was going to suggest this a while ago but got caught up in a review too.

It's in the sphinx-autodoc examples that you may need to add the local module directories to the sys.path for autodoc to work.

Yes, you're right. sphinx-autodoc actually has to import modules and hence requires the module source directories in sys.path.

As I said in #28 (comment), I wrongly assumed the -a flag used to invoke sphinx-apidoc would insert said directories into sys.path. There are two problems here:

• That flag only works if you're using sphinx-apidoc to generate a full sphinx project, which we are not.
• We cannot actually insert these directories into sys.path at the SphinxBuilder class layer, because of the scope of this change. When sphinx-build is invoked - and consequently, when sphinx-autodoc runs - it does not seem to find the modifications made to sys.path.

The solution I've settled on is:

• Provide the package source directory as a template variable to the rosdoc2_wrapping_conf_py_template. In this template, if the enable_autodoc setting is True, the package source directory's parent is inserted to sys.path. i.e. sys.path.insert(0, os.path.dirname('{package_src_directory}').
• Parallel to the above, provide the list of exec_depends of the package being processed as a template variable to the rosdoc2_wrapping_conf_py_template. Again, if enable_autodoc is True, this list of exec_depends is assigned to autodoc_mock_imports. Since autodoc imports every module in a file - it will invariably attempt to import that package's exec_depends too. Since it is possible for those dependencies to not be in sys.path, autodoc will mock any modules that it fails to import. This allows autodoc to build till completion, basically.

Thoughts about the solution? I could be wrong about my assessment of We cannot actually insert these directories into sys.path at the SphinxBuilder class layer, but I couldn't get anything to work that way. Let me know if this solution needs modification.

---

I believe the PR has addressed most of the feedback provided so far. Thoughts?

[clalancette]

* Provide the package source directory as a template variable to the `rosdoc2_wrapping_conf_py_template`. In this template, if the `enable_autodoc` setting is True, the package source directory's parent is inserted to `sys.path`. i.e. `sys.path.insert(0, os.path.dirname('{package_src_directory}')`.

One important thing we are going to have to keep in mind with this change is the buildfarm. In particular, the current rosdoc2 jobs do not install dependencies or source the ROS environment while building the documentation. In order for this to work, we are going to have to change the buildfarm to do both of those things, otherwise the whole spinx-autodoc thing isn't going to be able to find the pieces of the environment that it expects.

[aprotyas]

I don't think anything will have to be changed in the buildfarm for sphinx-autodoc to work though. The dependencies will all be patched by sphinx-autodoc, and only the source directory of the package in question gets appended (temporarily) to sys.path.

[wjwwood]

Yeah, my hope is that autodoc just needs to include python modules that are local to the package, but I haven't tested that.

---

All the rest of what you mentioned makes sense to me @aprotyas. I'll do another code review.

[ivanpauno]

Related to my last comment, the exec() line to include the user conf.py seems to be problematic if the original file had some relative files (depending where you run rosdoc2).

e.g. if you do things like https://docs.readthedocs.io/en/stable/guides/adding-custom-css.html.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-08-19/22008/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1

[wjwwood]

Sure. If you look at a section of sros2's init.py file:

That's a great example of something that could either be deferred to the first time it is used, or put into a function, to avoid the issue. For example, it could be this instead:

_xml_cache_path = None


def _get_xml_cache_path():
    global _xml_cache_path
    if _xml_cache_path is None:
        _xml_cache_path = urllib.parse.urljoin(
            'file:',
            urllib.request.pathname2url(
                os.path.join(
                    ament_index_python.get_package_share_directory('sros2'),
                    'xml_cache',
                    'xhtml-cache.xml'
                )
            )
        )
    return _xml_cache_path

So I think mocking is ok, but we just need to fix the issues that are coming up. Long term we could avoid that when we have full builds, but we don't have that right now. So I'd say add back the mocking and fix these issues maybe.

[aprotyas]

So I think mocking is ok, but we just need to fix the issues that are coming up. Long term we could avoid that when we have full builds, but we don't have that right now. So I'd say add back the mocking and fix these issues maybe.

I've thought of this approach, but this pushes the problem to users, i.e. if we can't build sros2/launch_ros/pkg_foo documentation with rosdoc2, let's see if there are ways to address that in sros2/launch_ros/pkg_foo. Each package in question needs to come up with a specific fix to its "avoid side effects because my dependencies may be mocked" issue. I don't know if we want to do that, especially because we can already afford full builds (they work for both ament_cmake and ament_python packages)

[rkent]

As @clalancette said, this PR really needs to get landed and issues dealt with in followup.

That being said, in the issue of mocking imports, I'm not sure what "# Note: autodoc only mocks up those modules that it actually cannot locate in PATH" means. The issue in the examples is launch and even if the user has sourced ROS2 so that launch can be imported (which is how I interpret as 'in PATH') that mock still occurs.

For a user who is running rosdoc2 in an environment where everything they need for their package is already installed, the issue can be fixed in rosdoc2 by importing the modules in conf.py. That is, modify rosdoc2_wrapping_conf_py_template to include something like this:

    import importlib
    for item in autodoc_mock_imports:
        try:
            importlib.import_module(item)
        except:
            pass

Or alternatively just stop doing the mock.

This would allow rosdoc2 to run successfully locally, but then fail when run in an infrastructure environment that produces common documentation. You could always install modules when running rodoc2, but that could be always slow. Perhaps you could add a runtime option to rosdoc2 to install modules prior to running that would be used in the infrastructure setting.

Anyway this discussion belongs in a separate issue after this patch has landed.

[aprotyas]

I'm not sure what "# Note: autodoc only mocks up those modules that it actually cannot locate in PATH" means. The issue in the examples is launch and even if the user has sourced ROS2 so that launch can be imported (which is how I interpret as 'in PATH') that mock still occurs.

Your interpretation is correct, and my comment there is wrong - I retracted it in 56a7908.

Or alternatively just stop doing the mock.

I think this recommendation is reasonable for packages that can actually be imported. From 56a7908:

    pkgs_to_mock = []
    import importlib
    for exec_depend in {exec_depends}:
        try:
            importlib.import_module(exec_depend)
        except ImportError:
            pkgs_to_mock.append(exec_depend)
    autodoc_mock_imports = pkgs_to_mock

I'm not fully clear on rosdoc2's expectations regarding the presence of package dependencies on the system, so I haven't dropped mocking outright. Note that the autodoc extension itself is still needed because some generated rst files contain autodoc directives.

Again, let's defer that conversation to after this PR has landed.

---

@clalancette I think this PR is now ready to land. I've tested that the package builds to completion with launch, launch_testing, launch_pytest, launch_xml, launch_yaml, launch_ros, ros2cli, ros2launch, sros2. I've also checked that rosdoc2 builds for C/C++ packages like rclcpp, rcpputils, rcl, rmw behave the same as before.

[rkent]

What will it take to get this landed? I'd like to do some work on rosdoc2, but I really cannot make progress as long as this PR is outstanding.

[aprotyas]

This PR is ready to land from my end too.

[hwadiadinkra]

Why is this still open? Can this be merged already? Really appreciate it, thank you.

[hwadiadinkra]

What will it take to get this landed? I'd like to do some work on rosdoc2, but I really cannot make progress as long as this PR is outstanding.

I think it's best to copy the 5 files changed and implement the fix locally. That way you do not have to wait for the ROS overlords to finally decide to bless aprotyas great work.

[Yadunund]

@clalancette I had a chance to test this PR against launch_ros/ros2launch which is an ament_python package and rclpy which is an ament_cmake package with python APIs to document.

No problems with ros2launch.

With rclpy, I had to add a rosdoc2.yaml along with a <rosdoc2> export tag in the package.xml. The yaml explictly disables doxygen generation which will execute by default else. However another issue exists with such packages where the sphinx_builder script will assume doxygen was invoked given the logic introduced here. As a result, it attempts to invoke breathe and exhale which will result in an error. That rosdoc2_settings dict is defined in the same script and it seems like the default entries do not get overwritten/updated by the key-values specified in the rosdoc2.yaml. So in the end to generate the doc for this pkg I had to explictly set enable_breathe and enable_exhale to False within the script. Also tested using the conf.py file already in rclpy by providing a sphinx_sourcedir arg in the rosdoc2.yaml file.

I think it should be possible to update that rosdoc2_settings dict with values from the rosdoc2.yaml either by injecting suitable entires into the template_variables from the BuildContext or by updating the implementation of generate_package_toc_entry.
I'm happy to work on this in a follow up PR if we want to merge this first.

[clalancette]

Just as an update on the current status here:

After discussion, we discovered that generation here is only working because @Yadunund had sourced the workspace before building. While that is one path forward, as it stands the way we generate documentation is in a standalone way. Thus, this PR needs more work to be able to generate standalone, otherwise we get lots of errors like:

WARNING: autodoc: failed to import module 'launch'; the following exception was raised:
No module named 'launch'

[rkent]

A few months ago, I did a bit of work on rosdoc2. My repo at https://github.com/rkent/rosdoc2 explored various approaches. I wanted to start to incorporate things in the main rosdoc2. But no progress could be made until the current patch is landed. Requests from several of us to get it landed went unanswered. So I lost interest, and have since moved on to other projects.

One of the last things that I did was a branch that added a basic unit test framework, rkent/add-tests, which was to be my suggestion of the minimum changes before suggesting some more radical changes. As part of that, I fixed some obvious issues that the tests revealed, including the issue in @clalancette's comment.

This commit seemed to make the issue go away:

rkent@3fb7141

Essentially the fix that I proposed was to add this line to the default conf.py:

sys.path.insert(0, os.path.abspath(os.path.join('{package_src_directory}', '..')))

So, where to go from here? My preference would be to land @aprotyas's patch as-is, and fix issues in subsequent patches. There are other issues with the patch, but it is hard to address them with the current patch unlanded behind a 100 comment thread. It would be much easier to address them with smaller patches, backed up by a basic unit test framework like rkent/add-tests. IIRC rosdoc2 is hardly used in production (has that changed?) so the downsides of just getting it landed are minimal.

What this patch really needs is for someone with the power to review and land patches to either land this patch as-is, or specify the minimum needed along with a commitment to review and land in a timely fashion.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-02-16/29927/1

[Yadunund]

@rkent I can understand your frustration. Just want to point out that rosdoc2 is actively used in production- we run document generation jobs on our build farms using this tool. Hence, the caution with merging big changes that may potentially result in build failures across hundreds of packages.

Having said that, i'm keen to get rosdoc2 operational for python packages. I've created this checklist ticket to delineate the steps needed for the same. One of the steps is to get your commit patched in so kindly keep an eye out for a PR request once we merge this PR in. There are other changes also needed as listed in the ticket.

My plan is to do one final check that the proposed changes do not break any documentation generation of ament_cmake packages before working together with the maintainers to get the proposed changes in. Stay tuned!

[Yadunund]

LGTM. Will be opening a follow-up PR with some other changes.

stale

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/growing-issue-with-ros-documentation/36075/48