aprotyas
commented
2 years ago Pinging @clalancette and @wjwwood for a review.
Closed aprotyas closed 1 year ago
aprotyas
commented
2 years ago Pinging @clalancette and @wjwwood for a review.
clalancette
commented
2 years ago I'm not sure about this
mixedsetting. What I was imagining is a default behavior forament_cmake(+ doxygen/exhale/breathe, - sphinx-apidoc), a different one forament_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
commented
2 years ago 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
mixedright now?
The only such package I can think of off the top of my head is message_filters.
So if an
ament_cmakepackage wanted to build python API docs too, then they would just enable that explicitly in therosdoc2_settingsdict, rather than change their type tomixed.
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
commented
2 years ago 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
commented
2 years ago 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?
aprotyas
commented
2 years ago 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:
rosdoc2 on launchRegardless, 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
commented
2 years ago 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
commented
2 years ago Here's what's changed since the latest round of feedback:
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)conf.py file less "cryptic" for the user, i.e. format mapped template variables into local variables before using them. (f2415cf)rosdoc2 settings being propagated through the BuildContext class with reasonable default values. (0642f05)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)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.pathfor 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 https://github.com/ros-infrastructure/rosdoc2/pull/28#issuecomment-897026854, I wrongly assumed the -a flag used to invoke sphinx-apidoc would insert said directories into sys.path. There are two problems here:
sphinx-apidoc to generate a full sphinx project, which we are not.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:
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}').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
commented
2 years ago * 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
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago 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
ros-discourse
commented
2 years ago 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-11-18/23209/1
aprotyas
commented
2 years ago Lest this PR is mistook as stale, it's still on my radar and I plan to pick it back up in a couple of weeks once my semester is complete. Thanks for the additional feedback @rkent!
rkent
commented
2 years ago Here are possible fixes for my comments, basically moving some of the setup earlier in the code: https://github.com/rkent/rosdoc2/tree/rkjfixes
rkent
commented
2 years ago It's very hard to make progress in rosdoc2 until this PR is landed. I've done a rebase to main so I could continue with my demo in my fork. What can I do to help move this forward?
aprotyas
commented
2 years ago It's very hard to make progress in rosdoc2 until this PR is landed. I've done a rebase to main so I could continue with my demo in my fork. What can I do to help move this forward?
@rkent Thanks for the nudge! I've cherry-picked your commit from https://github.com/rkent/rosdoc2/tree/rkjfixes to resolve issues with referencing paths before accessing them.
aprotyas
commented
2 years ago Trying to get this PR rolling again.
Updates:
{package_name} generated/index entry only works for packages where Doxygen is invoked. (https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/fa97f79c93e7000d0587da829886041b234329ea)Outstanding concerns:
rosdoc2 is run. @ivanpauno pointed out that if a package does anything with files local to its conf.py script, it may cause issues when it is processed with exec.
https://github.com/ros-infrastructure/rosdoc2/blob/81f94745eb058634af1ef6d52631217783ce9a16/rosdoc2/verbs/build/builders/sphinx_builder.py#L56At this point, I'm putting out a request for reviews/comments/testing. FYI @clalancette @wjwwood @ivanpauno @hidmic @rkent
ros-discourse
commented
2 years ago This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/ros-2-tsc-meeting-january-20th-2022/23986/1
aprotyas
commented
2 years ago @clalancette I addressed the feedback in https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/7d0b4fd4dff3ca0c92f8364ab39b47625f944dbf and https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/fb0eab5d53133b0a0d71a59edb7213a36c6fb522. Thanks for the review.
It would be great if you could try to look into those, and maybe suggest a way forward for those packages. I've also left some (minor) comments inline
The problem
sphinx_apidocis invoked, which uses theautodocextension for automatic generation of Sphinx rst sources.autodocrequires that all depended packages be importable at runtime. This is not ideal for document generation because users would need all exec dependencies installed and on their system path when generating documents.- To deal with the restriction in (2), imported modules can be mocked up by populating the
autodoc_mock_imports, from https://github.com/aprotyas/rosdoc2/blob/fb0eab5d53133b0a0d71a59edb7213a36c6fb522/rosdoc2/verbs/build/builders/sphinx_builder.py#L71-L78:if rosdoc2_settings.get('enable_autodoc', True): print('[rosdoc2] enabling autodoc', file=sys.stderr) extensions.append('sphinx.ext.autodoc') # Provide all runtime dependencies to be mocked up # Note: `autodoc` only mocks up those modules that it actually cannot locate in PATH autodoc_mock_imports = {exec_depends} # Add the package directory to PATH, so that `sphinx-autodoc` can import it sys.path.insert(0, os.path.dirname('{package_src_directory}'))autodocalso imports the module to be documented. When doing so, it executes all side-effects said module has on an import: i.e. executes the current module's__init__.pyfile.- For both of the failed builds in
sros2andlaunch_ros, importing the module executes code which uses some object that has already been mocked in (3). As such,importlib.import_modulethrows an exception since it doesn't know, say, thatSomeSubstitutionsType_types_tupleandtuplecan be added with the+operand.
I've verified that this is the case because NOT mocking launch in the case of launch_ros fixed this issue locally. But, this is not a real fix because it brings us back to the restrictions in (2).
autodoc's documentation issues a warning to this effect too:
Warning autodoc imports the modules to be documented. If any modules have side effects on import, these will be executed by autodoc when sphinx-build is run.
If you document scripts (as opposed to library modules), make sure their main routine is protected by a if
__name__ == '__main__'condition.
But, I don't think the proposed solution works for the packages in question, given what the __init__.py file of these packages is trying to accomplish.
As it stands I don't have a reasonable way forward here. Not mocking dependencies is not a useful compromise and neither is protecting import routines by a __name__ == '__main__' condition. Any ideas?
wjwwood
commented
2 years ago autodoc requires that all depended packages be importable at runtime. This is not ideal for document generation because users would need all exec dependencies installed and on their system path when generating documents.
I mean, we need the "full build" doc jobs eventually (for C++ packages and message packages) where you would install dependencies and build the package before running documentation. So eventually we want that anyway.
For both of the failed builds in sros2 and launch_ros, importing the module executes code which uses some object that has already been mocked in (3). As such, importlib.import_module throws an exception since it doesn't know, say, that SomeSubstitutionsType_types_tuple and tuple can be added with the + operand.
Can you give a concrete example of this? I can't think of any examples from memory where this would be an issue. It must some globals or something.
aprotyas
commented
2 years ago I mean, we need the "full build" doc jobs eventually (for C++ packages and message packages) where you would install dependencies and build the package before running documentation. So eventually we want that anyway.
Right. Thinking about this again, there's no need to mock dependencies if we hold the condition that all dependencies are built/installed and rosdoc2 is invoked after sourcing an appropriate underlay workspace.
In fact, I just tried building documentation for sros2 and launch_ros under the "full build" conditions, and they seem to work perfectly - discounting some sphinx tag errors.
Can you give a concrete example of this? I can't think of any examples from memory where this would be an issue. It must some globals or something.
Sure. If you look at a section of sros2's __init__.py file:
_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'
)
)
)If the dependencies for sros2 are mocked up, then os.path.join does not actually get a path from ament_index_python.get_package_share_directory (since this method has been mocked). Well, if I ignore my (arbitrary) need to mock dependencies, this isn't a problem anymore.
Thank you for the suggestion @wjwwood. I think the way forward is to just not mock dependencies (https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/b529001bae3670ded1e8fb7c5824009c29b58a3b), instead requiring that all dependencies are built/installed and rosdoc2 is invoked after sourcing an appropriate underlay workspace. Should this expectation be documented somewhere?
wjwwood
commented
2 years ago 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_pathSo 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
commented
2 years ago 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
commented
2 years ago 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:
passOr 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
commented
2 years ago I'm not sure what "# Note:
autodoconly mocks up those modules that it actually cannot locate in PATH" means. The issue in the examples islaunchand even if the user has sourced ROS2 so thatlaunchcan 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 https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/56a79083ab4163b705872c76e08b7ddfa76687c6.
Or alternatively just stop doing the mock.
I think this recommendation is reasonable for packages that can actually be imported. From https://github.com/ros-infrastructure/rosdoc2/pull/28/commits/56a79083ab4163b705872c76e08b7ddfa76687c6:
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_mockI'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
commented
2 years ago 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
commented
2 years ago This PR is ready to land from my end too.
hwadiadinkra
commented
1 year ago Why is this still open? Can this be merged already? Really appreciate it, thank you.
hwadiadinkra
commented
1 year ago 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.
clalancette
commented
1 year ago 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'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:
https://github.com/rkent/rosdoc2/commit/3fb714120e594e80f58d8fe45658f369368fd13d
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
commented
1 year ago 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
commented
1 year ago @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!
ros-discourse
commented
4 months ago 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
Previously,
rosdoc2would not generate any documentation for projects without doxygen - more concretely, projects with theament_pythonbuild type - as reported in https://github.com/ros-infrastructure/rosdoc2/issues/19.This PR attempts to resolve that issue. In offline discussions with @clalancette and @wjwwood, we decided that
rosdoc2can exhibit different behavior for different build types. See this comment for an outline of said behaviors.Specifically, this PR:
build_typeconfiguration option, which defaults to thebuild_typeobtained from the package.xml file. The set of availablebuild_types is [ament_cmake,cmake,ament_python].doxygen_builderandbreathe/exhaleextensions forament_pythonbuild types: For this build type,doxygendoes not need to be invoked whatsoever. Similarly, thebreathe/exhaleextensions tosphinxneed not be registered since there is nodoxygen-generated tag file to process.toctreeentries for the defaultindex.rstspecific to each build type.python_sourceconfiguration option. This option is represented as a path relative to the package'spackage.xmlfile, and should really only be used for packages that do not follow the standard Python package layout. This defaults to[package_name].sphinx-apidoctool forament_pythonbuild types so that documentation for Python modules can be automatically generated beforesphinx-buildis invoked.Signed-off-by: Abrar Rahman Protyasha abrar@openrobotics.org