Skip to content
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.

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

List meta dependencies in meta packages #114

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rkent
Copy link
Contributor

@rkent rkent commented Apr 18, 2024

This one might be a little more controversial, but generally meta packages have little documentation. The only thing interesting is the list of packages, so we show them. I assume that no package.build_depends but existing package.exec_depends defines a meta package. Is that correct?

I changed the default --base-url value because I test existing packages with simply rosdoc2 build -p . and I wanted real meta packages to have links that could be tested. I tried it with 'soccer_interfaces' and it worked OK.

@clalancette
Copy link
Contributor

I assume that no package.build_depends but existing package.exec_depends defines a meta package. Is that correct?

Unfortunately, I don't think we can make that assumption. Pure python packages, for instance, have no build_depends but exec_depends. Further, I don't even think we can use build_type as ament_cmake to distinguish this case, as it is possible to have pure python packages that are installed via CMake.

We do have an existing tag called metapackage that packages can put in their export section. But we do not enforce that flag is there, and in point of fact none of the core ROS 2 metapackages actually define that flag currently.

@tfoote
Copy link
Member

tfoote commented Apr 18, 2024

We have basically deprecated the metapackage as a formal designation. I think that the approach that we should look for on this is that the the full package.xml content is easily discoverable and viewable, independent of whether it's a package or a pseudo metapackge. The dependency list is just one element of that. The links added in #113 and the description in #112 are all getting the elements into the docs. Putting them all into a nice cohesive box that's moderately universal will be quite powerful. It could even be submitted to github to be rendered if we can keep it standalone in generating an rst or html block. And it would also be great if others who write a custom rst index etc can just do a directive like .. include_package_xml(./package.xml) kind of operation. And we can have that in the default template.

@rkent
Copy link
Contributor Author

rkent commented Apr 18, 2024

I have no problems including package.xml in the rosdoc2 output. I'd probably put in under "Standard Documents". I'll do a patch for that.

But I would still maintain that, for meta packages, it is useful to have a list of the included packages that can be easily clicked on to get to their documentation. Let me investigate a little more the possible ways that this could be detected, including looking at actual packages in use.

If there was a way to include this for most meta packages, even if it did not hit all of them, it could still be useful.

@rkent
Copy link
Contributor Author

rkent commented Apr 19, 2024

I did a review of packages that might be claiming to be meta packages (looking for 'meta' in package.description.lower(), and also matching "no package.build_depends but existing package.exec_depends". The additional common characteristic: all legitimate meta packages have no subdirectories, except 1: moveit also has a scripts directory. I'm going to look at packages that match that, but WITHOUT 'meta' in their description, to see how that looks.

@rkent
Copy link
Contributor Author

rkent commented Apr 19, 2024

Now reviewing a sampling of packages that do not have 'meta' in the description, but have no subdirectories (except perhaps 'doc'). The vast majority are legitimate meta packages (that is, a list of related packages that are part of a collection maintained by a single entity). A few are not meta packages of a larger collection, but include other packages. Examples:

vision_opencv: lists cv_bridge and image_geometry "Packages for interfacing ROS2 with OpenCV"

rosidl_runtime: action_msgs, service_msgs

moveit_resources: lists 5 moveit_resources_* packages, but also adds joint_state_publisher and robot_state_publisher

I'm going to modify the PR to match this plan.

@rkent
Copy link
Contributor Author

rkent commented Apr 19, 2024

Here we go with the subdirectory test. What's the downside to this? Maybe a handful of packages have their dependencies listed as a "meta package" when they are not true metapackages (if that even is well defined). For the vast majority of true metapackages, this adds useful documentation that should have links that can be followed by clicking.

@tfoote
Copy link
Member

tfoote commented Apr 19, 2024

I agree that this information would be useful. But I think that we shouldn't actually limit this to "meta" packages. ANd we're actively working to avoid using that as another official term. The dependencies are valuable to list and link for all packages not just meta-packages. We should think about how do we effectively render all the metadata about all the packages at in one compact format that is consistent.

One example where we're doing this that's been very successful is on the ROS wiki like this: https://wiki.ros.org/tf2

image

And we also have this information and more on ROS Index aka: https://index.ros.org/p/tf2/#rolling-overview

We can't currently get the full richness here because of the need for extra index information. But I think that we should look at creating a more homogeneous way to integrate this extra information. (Things like the reverse dependencies, and knowing if the links are alive, and the CI jobs. A lot of that information comes from the rosdistro.) And the version selector for generated docs, like our core docs and on the wiki and on index.ros.org would be highly valuable.

image

Rendering the raw xml like in #115 is a good first step. But lets do it in a way that's valid for all packages instead of specifically making up a heuristic for a meta package.

@rkent
Copy link
Contributor Author

rkent commented Apr 20, 2024

I count 62 packages in recent rolling that meet this definition of a meta package. The vast majority of those have no documentation except for the package.xml file. The vast majority of these are also clearly intended to be a meta package, in the sense that they are a collection of related packages, managed by a single entity, that can be deployed as a group.

So y'all may be avoiding the concept or name "metapackage" but it appears to me that this is a concept widely in use in existing packages. And I would argue that it is fundamentally different from other types of packages, where the dependencies are really more about what you need to make the package work, and after it is working (which is mostly a build issue), the package user doesn't really care about what the dependencies are.

rosindex has access to the distro, while so far rosdoc2 does not. The info in the distro would be valuable, but it requires an internet access step that so far rosdoc2 has avoided. But it might be needed in the future. We should probably discuss that issue independently of this meta dependency issue.

If you really don't like the word "meta" perhaps we could show execution dependencies when they exist. But I fear that would get confusing because people have been using the tag that mixes issues.

I don't really have the standing or deep knowledge to push this perspective too strongly, but from what I have seen I think this should move forward. Reconsider but if you (or y'all) still don't like it, we can drop it.

@clalancette
Copy link
Contributor

I don't really have the standing or deep knowledge to push this perspective too strongly, but from what I have seen I think this should move forward. Reconsider but if you (or y'all) still don't like it, we can drop it.

So I think my big problem with the implementation here is that it is based on heuristics. And given the thousands of packages that we have, those heuristics are inevitably going to be wrong for some subset of packages.

However, I do think that two good ideas have come out of this discussion: one is that we should show the dependencies of packages, and one is that metapackages are indeed a different "kind" of package.

For the first one, we have all of the metadata we need from the package.xml to display dependencies. So I think we can just make a PR to add that information; mostly it is a problem on how to display it.

For the second one, I think that we should allow the user to opt-in to documenting a package as a metapackage. That is, we add an option into rosdoc2.yaml (or similar) where a user can explicitly say that package so-and-so is a metapackage, and then we do special handling for it. That way we will never inadvertently call something a meta-package when it isn't one, and users always have a way to "opt-out" by not including the metapackage flag in the configuration. The downside here of course is that it isn't "automatic", and may be somewhat hard for users to discover, but I think that is preferable to heuristics.

Thoughts?

@rkent
Copy link
Contributor Author

rkent commented Apr 22, 2024

@clalancette Thanks for the input. I started to add some verbal comments, but it would be just as easy to prepare a revised PR that tries to respond to your concerns. I'll do that in the next couple of days.

@rkent
Copy link
Contributor Author

rkent commented Apr 26, 2024

The last branch push gives a version ready for re-review. This represents what I hope is an acceptable compromise, incorporating the previous comments.

Changes from before:

  • rosdoc2.yaml now has an option show_exec_dep which can be used to specify whether exec_depend should show.
  • If shown, the description does not use the term 'meta package' as before, but just calls them Execution Dependencies.

The heuristics established earlier are used if that value is not set.

With these changes, the downside of a false positive in show_exec_dep is:

  • execution dependencies are shown (but I would argue are still useful in a minimal package that would pass the heuristics, even if this is not a proper meta package).
  • those dependencies will link to the base_url, assuming they are packages and not rosdep values. To distinguish between these would require rosdoc2 load metadata from the internet, which is a barrier we have not yet crossed (though might someday).

The downside of a false negative is that the author would need to add an explicit entry in rosdoc2.yaml to show the exec_depend

The upside of the heuristics is that the vast majority of meta packages will now have some useful documentation shown, and not the virtually empty documentation as is currently shown. I doubt many existing packages would go back and explicitly add this new rosdoc2.yaml value if the heuristics were not present.

One question is, why only exec_depend and not all depend, and for all packages? For meta packages, they typically also have a dependency on ament_cmake which is not really useful to show. For all packages, I am not convinced there is value to showing them here in rosdoc2, as that is not useful information for most package users except in meta packages. Also rosindex already does a good job of showing that, with proper links since they have access to internet metadata, and you can see this in the package.xml listing that is now shown.

Anyway, this is ready for comments from @clalancette and @tfoote

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

One question is, why only exec_depend and not all depend, and for all packages? For meta packages, they typically also have a dependency on ament_cmake which is not really useful to show. For all packages, I am not convinced there is value to showing them here in rosdoc2, as that is not useful information for most package users except in meta packages. Also rosindex already does a good job of showing that, with proper links since they have access to internet metadata, and you can see this in the package.xml listing that is now shown.

I agree that rosindex does generally do a good job of showing the full dependency tree. But more cross references more easily are basically always going to provide a better experience for people. If someone can get from the docs of one package to the docs of another package in 2 clicks (package root, -> dependency) that's a much better experience than having to copy the package name, type it into ros index, click the version your want then click the API Docs link. They're unlikely to do that all. They're more likely to go to the url bar and type in the dependency name at the top replacing the current package. We want to make it easier than that. Which is why the Package Home => Dependency list is what we should do for all dependencies.

The capabilities of the index are great, but as we flesh out docs.ros.org and it becomes more complete we may be able to actually integrate all of those capabilities into docs.ros.org and keep people in the same ecosystem.

To that end lets start with just including all the exec_depends unconditionally. There's plenty of space and we can optimize it's layout in the future to be more compact. Maybe a table layout with the other dependency types.

Then we don't have to have heuristics or extra options or parameters. Maybe one option to create the dependency display, that defaults to on, but has a parameter to disable.

Then it's very clean and everyone gets the benefits by defaut.

For the rosdep vs dependency ambiguity we need to solve that anyway. We should be able to do that in a rosdistro python package dependency where you can resolve the distro, or local path one or else it's a rosdep. All the command line tools do this already.

Execution Dependencies of this package
--------------------------------------
{% for dependency in package.exec_depends -%}
* `{{ dependency }} <{{ base_url }}/{{ dependency }}>`_
Copy link
Member

Choose a reason for hiding this comment

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

Injecting the base_url here means that the inter package content jumps out of the local tree of a local build tree, or a mirror etc. I think everything else generally uses relative paths and thus is easily testable in the local location and relocatable. Overall using base_url makes sense, but maybe having the default be "." such that everything is relative. Otherwise we really need to flesh out the documentation and deployment stories. It's a challenge to have to test all the docs at one URL or file path. But then have to rebuild them to deploy. Before this the only locations using the base_url are the doxygen tags and sphinx inventory files which actually might be helpful for leveraging cross referencing. But also might have issues if we get to supporting them locally for cross referncing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I'll modify this according to your comments, but it might take a few days because this is a busy week for me.

@mikeferguson
Copy link

I just saw this PR and read some of the feedback (but certainly not all) - but just wanted to note that I sort of manually did this for image_pipeline - https://docs.ros.org/en/rolling/p/image_pipeline/index.html - the side navigation was updated to jump to the individual packages (in a fairly hacky way I think), and the shared tutorials are in that package as they were on the old wiki

@rkent
Copy link
Contributor Author

rkent commented Aug 6, 2024

@mikeferguson Thanks for pointing out your work to me. Your work, particularly the 'fairly hacky way', is actually fairly relevant to the issues I am facing with #137 I'm going to further comment there if you are interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants