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.

Sign up for GitHub

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

Jump to bottom

Add example for migrating a Python package from ROS 1 to 2 #4780

Merged
merged 22 commits into from
Oct 15, 2024
Merged

Add example for migrating a Python package from ROS 1 to 2 #4780

merged 22 commits into from
Oct 15, 2024

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Oct 1, 2024 •
edited
Loading

This adds a guide walking someone through migrating a ROS 1 Python package to ROS 2. I assumed the reader is familiar with ROS 1 and Python, but is not familiar with ROS 2's Python APIs or best practices.

@sloretz sloretz self-assigned this Oct 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

HTML artifacts: https://github.com/ros2/ros2_documentation/actions/runs/11353745523/artifacts/2060170324.

To view the resulting site:

  1. Click on the above link to download the artifacts archive
  2. Extract it
  3. Open html-artifacts-4780/index.html in your favorite browser

@sloretz sloretz changed the title Add tutorial for migrating a Python package from ROS 1 to 2 Add example for migrating a Python package from ROS 1 to 2 Oct 1, 2024
@sloretz
Add tutorial for migrating a Python package from ROS 1 to 2
b4e5742 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Newline at end of file
278bed2 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz force-pushed the sloretz__ROS-1-to-2_python_tutorial branch from c367d2e to 278bed2 Compare October 1, 2024 21:32
@sloretz
Tutorial -> Example
de72497 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Tutorial -> Example
338470d 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@kscottz
Copy link
Collaborator

kscottz commented Oct 4, 2024

This is a great start, what I would recommend we do this in a step by step fashion. For example, first show the package.xml upgrade process, then move on the next file. This approach would keep the before and after files right next to each other.

@sloretz
More progress on migration guide
4607856 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
More section headings
9b77568 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Misc progress
50f1ef4 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
A little more progress
6157489 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Plan out headings and reduce depth of headings
ab4e452 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Uppercase Python
a06cbfc 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Misc additions
be0ff1d 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Rate
ff2727d 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Complete rough draft
1678c27 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Polish first draft
aa0c6ce 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz marked this pull request as ready for review October 14, 2024 18:53
@sloretz sloretz requested review from kscottz and removed request for clalancette October 14, 2024 18:54
@sloretz
Copy link
Contributor Author

sloretz commented Oct 14, 2024

@kscottz @clalancette this guide is ready for review!

clalancette
clalancette previously requested changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This is a fantastic document. I've left a few things to fix, hopefully they aren't too onerous.

source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst


def main():
rclpy.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

For the rolling branch only, I'm going to suggest that we switch to the with rclpy.init(): context manager, since we have switched all of our other examples to that. But do note that when we backport this to other distributions, we are going to have to go back to the "regular" rclpy.init().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in a separate PR #4813. My thinking is we can merge this one, backport-all, and then merge #4813 just to rolling.

kscottz
kscottz reviewed Oct 14, 2024
View reviewed changes
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst
:depth: 2
:local:

Prerequisites
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just did a first pass read on this while I had a minute.

One suggestion, which you are free to ignore, is that if you wrote an index / overview for this document it would effectively be a check list that readers could work through systematically.

We can certainly come back to this once the initial PR is complete and add it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will skip this one for now to conserve time

@sloretz
example.com email
bdb3cdc 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
sloretz and others added 4 commits October 14, 2024 18:18
@sloretz
Explain what the package marker file does
f33240c 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz @clalancette
Apply suggestions from code review
8a1445b 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Give reason for colcon workspace and capitalize Colcon
3704f9b 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
Add tabs for OSX and Windows
f6f6602 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz mentioned this pull request Oct 15, 2024
Merged
@sloretz
Typo and unused import removal
bf210bb 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz dismissed clalancette’s stale review October 15, 2024 01:59

Comments addressed

@sloretz
Copy link
Contributor Author

sloretz commented Oct 15, 2024

@clalancette @kscottz all feedback addressed. PTAL 🙇

fujitatomoya
fujitatomoya approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

i think this is great enhancement for migration process. lgtm.

source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
clalancette
clalancette previously requested changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few more minor things to fix, then I think this is ready to go!

source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
source/How-To-Guides/Migrating-from-ROS1/Migrating-Python-Package-Example.rst Outdated Show resolved Hide resolved
@clalancette clalancette added the backport-all backport at reviewers discretion; from rolling to all versions label Oct 15, 2024
sloretz and others added 2 commits October 15, 2024 13:13
@sloretz @clalancette
More example.com uses
08b4f3a 
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz
ros2 topic echo /chatter
7899861 
Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
@sloretz sloretz dismissed clalancette’s stale review October 15, 2024 20:20

Feedback addressed

clalancette
clalancette approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good! Will go ahead and merge this and backport, then merge in #4813 only on Rolling.

@clalancette clalancette merged commit d9391f3 into rolling Oct 15, 2024
4 checks passed
@clalancette clalancette deleted the sloretz__ROS-1-to-2_python_tutorial branch October 15, 2024 20:43
mergify bot pushed a commit that referenced this pull request Oct 15, 2024
@sloretz @mergify
Add example for migrating a Python package from ROS 1 to 2 (#4780)
002d770 
* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)
@mergify mergify bot mentioned this pull request Oct 15, 2024
Merged
mergify bot pushed a commit that referenced this pull request Oct 15, 2024
@sloretz @mergify
Add example for migrating a Python package from ROS 1 to 2 (#4780)
a8c1831 
* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)
@mergify mergify bot mentioned this pull request Oct 15, 2024
Merged
mergify bot pushed a commit that referenced this pull request Oct 15, 2024
@sloretz @mergify
Add example for migrating a Python package from ROS 1 to 2 (#4780)
8bc8ea0 
* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)
@mergify mergify bot mentioned this pull request Oct 15, 2024
Merged
clalancette pushed a commit that referenced this pull request Oct 15, 2024
@mergify @sloretz
Add example for migrating a Python package from ROS 1 to 2 (#4780) (#…
9cd9f9f 
…4815)

* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)

Co-authored-by: Shane Loretz <shane.loretz@gmail.com>
clalancette pushed a commit that referenced this pull request Oct 15, 2024
@mergify @sloretz
Add example for migrating a Python package from ROS 1 to 2 (#4780) (#…
dc81eee 
…4816)

* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)

Co-authored-by: Shane Loretz <shane.loretz@gmail.com>
clalancette pushed a commit that referenced this pull request Oct 15, 2024
@mergify @sloretz
Add example for migrating a Python package from ROS 1 to 2 (#4780) (#…
a19dbfc 
…4817)

* Add tutorial for migrating a Python package from ROS 1 to 2

Signed-off-by: Shane Loretz <shane.loretz@gmail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
(cherry picked from commit d9391f3)

Co-authored-by: Shane Loretz <shane.loretz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kscottz kscottz kscottz left review comments

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@clalancette clalancette clalancette approved these changes

@audrow audrow Awaiting requested review from audrow audrow is a code owner

Assignees

@sloretz sloretz

Labels
backport-all backport at reviewers discretion; from rolling to all versions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sloretz @kscottz @clalancette @fujitatomoya