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

[ament_index] Should fix / simplify injection of per-package AMENT_PREFIX_PATH #270

Open
EricCousineau-TRI opened this issue Mar 31, 2023 · 8 comments

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Mar 31, 2023

I don't understand why we inject per-package AMENT_PREFIX_PATH.
Can we simplify and report as few paths as possible, if ament_index recurses? (We know Drake's PackageMap does)

For example, in #269, I only depend on the data for rviz_default_plugins:

ros_py_test(
name = "parse_ros_model_test",
srcs = ["test/parse_ros_model_test.py"],
main = "test/parse_ros_model_test.py",
# TODO(eric.cousineau): Use a non-test resource and enable this test.
tags = ["manual"],
data = ["@ros2//:rviz_default_plugins_share"],
deps = ["@drake//bindings/pydrake"],
)

However, if I print out os.environ["AMENT_PREFIX_PATH"], I see all of the packages listed.
For PackageMap, I see the following slew of errors:

$ bazel run //:parse_ros_model_test
<313 lines of similar errors>
WARNING  drake:None:0 Unable to open directory: {runfiles}/ros2/archive/ament_clang_tidy
WARNING  drake:None:0 Unable to open directory: {runfiles}/ros2/archive/ament_clang_format

Assigning @sloretz for disposition, \cc @adityapande-1995

FYI @adeeb10abbas @jwnimmer-tri

@EricCousineau-TRI EricCousineau-TRI changed the title [ament_index] Shoudl fix / simplify injection of per-package AMENT_PREFIX_PATH [ament_index] Should fix / simplify injection of per-package AMENT_PREFIX_PATH Mar 31, 2023
@EricCousineau-TRI
Copy link
Collaborator Author

Shane, can you provide a suggestion for the simplest route to success? Is there a way we can be more selective with what paths we provide?

Once you provide that, either Aditya or I can execute on it.

@sloretz
Copy link
Collaborator

sloretz commented Mar 31, 2023

Shane, can you provide a suggestion for the simplest route to success?

I'd be happy to!

The archive we're using at the moment is an isolated workspace. This means every package has a unique install location (usually install/<package name>), and so each has its own entry in AMENT_PREFIX_PATH.

The only way to get a single AMENT_PREFIX_PATH for all packages is to use a merged workspace. For example, /opt/ros/<distro> is a merged workspace, so using debian packages instead of an archive would mean there was only a single value in AMENT_PREFIX_PATH. It would also be possible to manually build an archive that uses a merged workspace instead of an isolated one (colcon build --merge-install).

Can we simplify and report as few paths as possible, [...] For example, in [...] I only depend on the data for rviz_default_plugins:

Right now the generated distro.bzl file puts all of the paths into a single variable for the dload shim to use. It might be possible to make this a per target thing (where the rules have an AmentIndex provider like the hydroelastic collisions PR uses).

However, I'm not aware of any performance issues with regards to long values of AMENT_PREFIX_PATH (aside from increasing length of time it takes to source a workspace when using some shells, like Windows cmd.exe). Uploading a merged workspace to Girder seems to me like it would be less work for a better result.

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Apr 12, 2023

Constraining the archive type does sound like an easier solution, but my ideal is that we need not constrain things this way. I think going the AmentIndex provider route might be better, but can be convinced others.

However, given that #133 landed, maybe it's not too hard?

@sloretz
Copy link
Collaborator

sloretz commented Apr 13, 2023

Constraining the archive type does sound like an easier solution, but my ideal is that we need not constrain things this way.

Instead of thinking this as a constraint on the archive type, I think it would be fair to call it a choice of the type of workspace Drake-ROS uses by default. I think users are already free to use either a merged or isolated workspace with Drake-ROS today.

I think going the AmentIndex provider route might be better, but can be convinced others.

Making each ROS package in the generated BUILD.bazel have an AmentIndex would lead to fewer entries in AMENT_PREFIX_PATH when using an Isolated ROS workspace as each target would have only the entries it needs, but how much better does it need to be to justify the cost of developing it?

If the metric for "better" is runtime performance, I'm not aware of any significant performance degradation with long values of AMENT_PREFIX_PATH. If human readability is the metric, anything that uses Drake-ROS is probably going to depend on @ros2//:rclcpp_cc directly or indirectly. Running colcon list --packages-up-to rclcpp | wc -l on my ROS 2 workspace returns 125 packages. My ROS workspace has 354 packages in total. An AMENT_PREFIX_PATH with 125 entries instead of 354 entries is still going to be hard for a human to read.

@EricCousineau-TRI
Copy link
Collaborator Author

If the workspace is merged and should effectively resolve to a shorter AMENT_PREFIX_PATH, I think that'd be a better option?
And my desire is that using AmentIndex provider should be able to easily recognize that, and/or it should be recognized via shim mechanisms.

At this point, the maximal case (depending on all packages) should yield a path list whose size is <= what we currently have, at which point there should only be gains for this approach.

Does that make sense?

@sloretz
Copy link
Collaborator

sloretz commented Apr 14, 2023

If the workspace is merged and should effectively resolve to a shorter AMENT_PREFIX_PATH, I think that'd be a better option?

Yes, I think using a merged workspace is the best option for getting shorter values of AMENT_PREFIX_PATH

And my desire is that using AmentIndex provider should be able to easily recognize that, and/or it should be recognized via shim mechanisms.

I don't understand. What is it the AmentIndex provider should recognize?

@EricCousineau-TRI
Copy link
Collaborator Author

Ah, sorry - meant that AmentIndex should be able to deduplicate any paths that are listed twice, whether it be for a merged workspace or an isolated workspace.

@sloretz
Copy link
Collaborator

sloretz commented Apr 15, 2023

Ah, sorry - meant that AmentIndex should be able to deduplicate any paths that are listed twice, whether it be for a merged workspace or an isolated workspace.

Ah, are you saying that if there are two AmentIndex providers that point to the same prefix (say we have a target depending on two calls ament_index_share_files twice with the same prefix), then we should de-duplicate those paths before passing them to a dload shim?

If so, maybe we could do that by making this a set (or is depset the right bazel datastructure?):

@sloretz sloretz removed their assignment Feb 16, 2024
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

No branches or pull requests

3 participants