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

Support for ros launch #298

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Support for ros launch #298

merged 3 commits into from
Apr 18, 2024

Conversation

adityapande-1995
Copy link
Collaborator

@adityapande-1995 adityapande-1995 commented Aug 21, 2023

This PR aims to add support for ros launch using a new bazel rule. It is based off of the ros_py_binary rule.

This is a bit tricky as roslaunch has the power to execute arbitrary nodes, executables and a whole bunch of actions that rely on paths and environment being set accordingly.

Design

I'm piggybacking off the ros_py_binary() rule here. That rule makes sure my environment is set up properly (rmw, index paths, etc) so that a ros node can be run in a python process. If I run a subprocess here, using the ros2 CLI, I inherit the environment for free, and don't need to worry about separate yaml/xml/python parsing.

Problems and limitations

EDIT(eric): See #344

Example

See README updates in the PR

cc @tervay-bdai


This change is Reviewable

@adityapande-1995 adityapande-1995 changed the title WIP: Support for ros launch with a new rule WIP: Support for ros launch Aug 24, 2023
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Aug 31, 2023

Got a new launch action implemented ! ExecuteBazelTarget() in python and <execute_bazel_target/> in xml style. The utils file might need some refactoring with regards to the pip install action.

Getting it to work in xml required a workaround. The entry point has to be registered with the group launch.frontends and setup() seems to be the only way to do it. This required packaging roslaunch_utils file into a package, and pip installing it.

Open to suggestions on refactoring, this is just a minimal viable product. cc @tervay-bdai @IanTheEngineer @calderpg-tri

@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Sep 18, 2023

Improvement suggestion :

In order for ExecuteAction() to work with xml and yaml, there needs to be an entry point registered, with the tag launch.frontends. Usually this is done with setuptools.setup() , which is invoked when we do a pip install . on some package. Currently, I'm doing this using a os.system('pip install /path/to/action'), in roslaunch_utils.py. This PR is functional, but I hope this can be made more bazel friendly. This line seems weird to me, and could be replaced with a better solution :

os.system("pip install ../bazel_ros2_rules/ros2/tools/roslaunch_util >/dev/null 2>&1")

I assume pip_install or pip_parse are meant to do this in bazel.

I'm unable to use pip_install() rule to install a local python package (i.e. roslaunch_utils in this case) using pip. Is there a suggestion on if this is possible ? Started a discussion here : https://github.com/bazelbuild/bazel/discussions/19557

Otherwise, is it possible to add a custom entry point in bazel ?

@adityapande-1995 adityapande-1995 marked this pull request as ready for review September 18, 2023 06:19
@adityapande-1995 adityapande-1995 changed the title WIP: Support for ros launch Support for ros launch Sep 18, 2023
@adityapande-1995
Copy link
Collaborator Author

adityapande-1995 commented Sep 25, 2023

Update : pip_install() is the wrong approach here, there is no way to use a local package using that rule to invoke setuptools (to have a custom entrypoint). Open for review !

@EricCousineau-TRI
Copy link
Collaborator

@IanTheEngineer Do you have time to review / land this PR on Mon / Tues?
If not, I can help to do so.

@ali-bdai
Copy link

@EricCousineau-TRI Let me know if there's anything I can do to expedite this. Thanks!

@adityapande-1995
Copy link
Collaborator Author

Rebased on main, open for review.

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 10 files at r1, 1 of 1 files at r2, 1 of 1 files at r4.
Reviewable status: 7 of 10 files reviewed, 7 unresolved discussions (waiting on @adeeb10abbas and @adityapande-1995)

a discussion (no related file):
There is no documentation in this PR as to how to confirm that Bazel-ified launching works.

Can you take your overview comments, and place them in code in this PR?



bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 39 at r4 (raw file):

    # Required to get xml launch working, as we need to register an entry
    # point for launch extensions.
    os.system("pip install ../bazel_ros2_rules/ros2/tools/roslaunch_util >/dev/null 2>&1")

pip install should not be used.

Why is it needed here? What fails if you do not do this?


bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 46 at r4 (raw file):

Previously, adityapande-1995 (Aditya Pande) wrote…

Can use bazel runfiles here, to extract the path.

Yes, should certainly use runfiles.


bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 49 at r4 (raw file):

    action = "launch"
    # TODO : Is there a better way to locate the launch file exactly ?
    launch_file = find_rel_path(launch_file_name, os.getcwd())

Can you reference what test might fail if you don't have this search functionality?

What is the normal mechanism by which ros2 launch works when locating a binary? (and by that token, ros2 run)
Does it just find whichever binary first?

(Can you cite the code where this happens?)


ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 243 at r4 (raw file):

        ":eg_listener",
    ],
)

This should have a test showing that our Bazel spelling of ros2 launch works.


ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 243 at r4 (raw file):

        ":eg_listener",
    ],
)

nit Ideally, with our own provided AMENT_PREFIX_PATH, we should be able to call ros2 launch itself on our own targets.

Consider adding a test to this extent, or a TODO to that effect.

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 10 files reviewed, 7 unresolved discussions (waiting on @adeeb10abbas and @adityapande-1995)


bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 39 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

pip install should not be used.

Why is it needed here? What fails if you do not do this?

Yeah, this def. shouldn't be used. It polluted my system, s.t. ros2 launch will try to pull inros2launch_util even outside of Bazel.

I had to fix my system by deleting the following:

rm -rf ~/.local/lib/python3.10/site-packages/roslaunch_util*

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Dismissed @adeeb10abbas from a discussion.
Reviewable status: 0 of 18 files reviewed, 1 unresolved discussion (waiting on @adeeb10abbas and @adityapande-1995)


ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 243 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This should have a test showing that our Bazel spelling of ros2 launch works.

Done.


ros2_example_bazel_installed/ros2_example_apps/BUILD.bazel line 243 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Ideally, with our own provided AMENT_PREFIX_PATH, we should be able to call ros2 launch itself on our own targets.

Consider adding a test to this extent, or a TODO to that effect.

Done. Added TODO.


bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 39 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yeah, this def. shouldn't be used. It polluted my system, s.t. ros2 launch will try to pull inros2launch_util even outside of Bazel.

I had to fix my system by deleting the following:

rm -rf ~/.local/lib/python3.10/site-packages/roslaunch_util*

Done. Removed anything trying to load / install extensions.


bazel_ros2_rules/ros2/tools/roslaunch_util/roslaunch_util.py line 49 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you reference what test might fail if you don't have this search functionality?

What is the normal mechanism by which ros2 launch works when locating a binary? (and by that token, ros2 run)
Does it just find whichever binary first?

(Can you cite the code where this happens?)

Done. Removed in lieu of more explicit approach.

@EricCousineau-TRI
Copy link
Collaborator

@ali-bdai can you review this and see if it meets your needs?

If you need something more, would you be up for taking over this PR?

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 19 files reviewed, all discussions resolved (waiting on @adeeb10abbas)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

There is no documentation in this PR as to how to confirm that Bazel-ified launching works.

Can you take your overview comments, and place them in code in this PR?

Done.


@adeeb10abbas
Copy link
Member

@ali-bdai can you review this and see if it meets your needs?

If you need something more, would you be up for taking over this PR?

LGTM, I think this should get the initial phase of what I am trying to do, thanks a ton!

Yep, I can take on the next one(s) to keep this going. This is great. :D

Copy link
Member

@adeeb10abbas adeeb10abbas left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 19 files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)

Copy link
Member

@IanTheEngineer IanTheEngineer left a comment

Choose a reason for hiding this comment

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

:LGTM: as well. I appreciate the iterations you've gone through here, Aditya and your most recent updates Eric. This all looks reasonable to me. I created an issue to track the outlined limitations of this feature:

#344

@EricCousineau-TRI and @ali-bdai , please feel free to add your thoughts there for future improvements.

Reviewed 9 of 18 files at r5, 5 of 8 files at r7, 3 of 3 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @adityapande-1995)

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 18 files at r5, 5 of 8 files at r7, 3 of 3 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @adityapande-1995)

@EricCousineau-TRI
Copy link
Collaborator

Unable to resolve this convos via "Unresolved conversations ... View" link:
image

They same the commits no longer exit:
https://github.com/RobotLocomotion/drake-ros/pull/298/files/a87e1fc3483134005a6777067074bc848202c7f2#r1541968867
https://github.com/RobotLocomotion/drake-ros/pull/298/files/4093d9c7c0761c378e19ba0a3bf0dcfb510279b5#r1518646074

For this reason, we should avoid making GitHub review comments, and stick to using Reviewable.

@EricCousineau-TRI EricCousineau-TRI merged commit 955245e into main Apr 18, 2024
6 checks passed
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.

5 participants