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

Drop support for ROS 2 Iron #981

Merged
merged 2 commits into from
Dec 25, 2024
Merged

Drop support for ROS 2 Iron #981

merged 2 commits into from
Dec 25, 2024

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Dec 18, 2024

Public API Changes
None

Description
This PR removes support for ROS 2 Iron now that it's EOL.

Additionally, this lets us re-enable some unit tests that were previously failing on Iron.

@sea-bass
Copy link
Contributor Author

sea-bass commented Dec 18, 2024

@MatthijsBurgh or @bjsowa -- can you go into the repo settings and remove the Iron check as required on the ros2 branch? Thanks!

@bjsowa
Copy link
Collaborator

bjsowa commented Dec 18, 2024

@MatthijsBurgh or @bjsowa -- can you go into the repo settings and remove the Iron check as required on the ros2 branch? Thanks!

I don't have admin access to this repo so I'm unable to do this.

@@ -25,7 +25,7 @@
class TestActionCapabilities(unittest.TestCase):
def setUp(self):
rclpy.init()
self.executor = SingleThreadedExecutor()
self.executor = MultiThreadedExecutor()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Contributor Author

@sea-bass sea-bass Dec 18, 2024

Choose a reason for hiding this comment

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

The tests that were previously skipped seem to have needed this. I was getting some flaky behavior without this for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have very bad experiences with the MultiThreadedExecutor, see ros2/rclpy#1223. So we need to make sure we use CycloneDDS to limit the issues. (Or keep using the SingleThreadedExecutor)

Copy link
Collaborator

Choose a reason for hiding this comment

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

They work fine on my end with single threaded executor

Copy link
Contributor Author

@sea-bass sea-bass Dec 18, 2024

Choose a reason for hiding this comment

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

Alright -- I've put it back into SingleThreaded EDIT: #981 (comment)

That rclpy is issue is very concerning! wow

@sea-bass sea-bass force-pushed the remove-iron branch 2 times, most recently from b70b270 to a831604 Compare December 18, 2024 13:11
@sea-bass sea-bass requested a review from bjsowa December 18, 2024 13:13
@sea-bass
Copy link
Contributor Author

sea-bass commented Dec 18, 2024

Yeah, see the test was flaky on one of the 4 runs with SingleThreadedExecutor.

I can either keep that test disabled, or go to MultiThreadedExecutor. I think for a unit test, it might be OK to use these executors, so long as we don't put it in the main source code?

@bjsowa
Copy link
Collaborator

bjsowa commented Dec 18, 2024

I think for a unit test, it might be OK to use these executors, so long as we don't put it in the main source code?

That's fine by me

@bjsowa
Copy link
Collaborator

bjsowa commented Dec 21, 2024

@MatthijsBurgh or @bjsowa -- can you go into the repo settings and remove the Iron check as required on the ros2 branch? Thanks!

@MatthijsBurgh Could you please do it?

@MatthijsBurgh
Copy link
Contributor

Done

@sea-bass sea-bass merged commit 25fe570 into ros2 Dec 25, 2024
12 checks passed
@sea-bass sea-bass deleted the remove-iron branch December 25, 2024 14:19
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.

3 participants