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

Add API samples for Python #1253

Merged
merged 4 commits into from
Apr 1, 2023
Merged

Conversation

DLu
Copy link
Contributor

@DLu DLu commented Mar 1, 2023

In lieu of @gbiggs fixing #887, I cherry-picked the commit here and then renamed things based on @MichaelOrlov 's comment

@DLu DLu requested a review from a team as a code owner March 1, 2023 20:06
@DLu DLu requested review from gbiggs and hidmic and removed request for a team March 1, 2023 20:06
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@DLu Thank you for moving forward with this PR.
Could you please fix linter errors

Error Message
AssertionError: Found 3 code style errors / warnings:
  ./rosbag2_examples_py/data_generator_node.py:8:1: E302 expected 2 blank lines, found 1
  ./rosbag2_examples_py/data_generator_executable.py:34:1: E305 expected 2 blank lines after class or function definition, found 1
  ./rosbag2_examples_py/simple_bag_recorder.py:8:1: E302 expected 2 blank lines, found 1
assert 1 == 0

And add copyright to the python files with the same template as in

// Copyright 2021 Open Source Robotics Foundation
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@DLu Thanks for PR.
LGTM.

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/758e2c906fcfb97e3e7be59b516d99cc/raw/fe8d10c5fe162ccc871b2f1694188c8e7e2b8846/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_examples_py
TEST args: --packages-above rosbag2_examples_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11619

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

@DLu Can you please rebase your branch on top of latest rolling?
CI fails due to absent fixes from #1257

@DLu
Copy link
Contributor Author

DLu commented Mar 21, 2023

@MichaelOrlov Ready to go!

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/758e2c906fcfb97e3e7be59b516d99cc/raw/fe8d10c5fe162ccc871b2f1694188c8e7e2b8846/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_examples_py
TEST args: --packages-above rosbag2_examples_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11645

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

@DLu CI has warnings. Could you please address them?

Error Message
AssertionError: Found 5 code style errors / warnings:
  ./rosbag2_examples_py/data_generator_node.py:17:1: I100 Import statements are in the wrong order. 'from example_interfaces.msg import Int32' should be before 'from rclpy.serialization import serialize_message'
  ./rosbag2_examples_py/data_generator_node.py:23:1: CNL100 Class definition does not have a new line.
  ./rosbag2_examples_py/simple_bag_recorder.py:19:1: I100 Import statements are in the wrong order. 'import rosbag2_py' should be before 'from std_msgs.msg import String'
  ./rosbag2_examples_py/simple_bag_recorder.py:23:1: CNL100 Class definition does not have a new line.
  ./rosbag2_examples_py/data_generator_executable.py:17:1: I100 Import statements are in the wrong order. 'from example_interfaces.msg import Int32' should be before 'from rclpy.serialization import serialize_message'
assert 1 == 0

@DLu
Copy link
Contributor Author

DLu commented Mar 29, 2023

Two things:

  1. I'm not familiar with this new style of CI. Why isn't this a github hook?
  2. Is there a specific invocation I can use to generate the code style warnings above? They didn't show up with pytest and there's no pre-commit settings.

gbiggs and others added 4 commits March 29, 2023 11:53
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: David V. Lu <davidvlu@gmail.com>
Signed-off-by: David V. Lu <davidvlu@gmail.com>
Signed-off-by: David V. Lu <davidvlu@gmail.com>
@clalancette
Copy link
Contributor

  1. I'm not familiar with this new style of CI. Why isn't this a github hook?

Just to be clear, this isn't new at all. This has been the CI practice for ROS 2 for approximately 7 years. There are definitely arguments to be made that this should be a GitHub hook, but doing it this way means that OSRF is ultimately in control of running CI. Regardless, this isn't the place to debate this.

  • Is there a specific invocation I can use to generate the code style warnings above? They didn't show up with pytest and there's no pre-commit settings.

If you follow the instructions on http://docs.ros.org/en/rolling/Installation/Alternatives/Ubuntu-Development-Setup.html on installing dependencies, it will install all of the pytest/flake8 hooks you need.

@DLu
Copy link
Contributor Author

DLu commented Mar 29, 2023

I apologize if my question about the Github hook came off as combative, but I just didn't understand what was going on. Is there documentation about this kind of CI? Also, not to be pedantic, but do you still work for OSRF?

@MichaelOrlov
Copy link
Contributor

@clalancette I am also curious how to reproduce or catch errors from ament_flake8 locally.
I have tried to run colcon test --packages-select rosbag2_examples_py there are no errors.
Also have tried to run the same command as on CI

colcon test --base-paths "src" --build-base "build" --install-base "install" --event-handlers console_direct+ --executor sequential --retest-until-pass 2 --ctest-args -LE xfail --pytest-args -m "not xfail" --packages-above rosbag2_examples_py

Still no errors was reported. I've double checked I have up to date version of the ament_flake8 in my local environment.
Why on CI it is reporting errors/warnings while on local run not?

@MichaelOrlov
Copy link
Contributor

Trying to re-run CI one more time
Gist: https://gist.githubusercontent.com/MichaelOrlov/758e2c906fcfb97e3e7be59b516d99cc/raw/fe8d10c5fe162ccc871b2f1694188c8e7e2b8846/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_examples_py
TEST args: --packages-above rosbag2_examples_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11729

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

@clalancette I've spent some time trying to investigate situation why CI job giving us warning errors and we can't reproduce it locally.
The reasoning for that seems in github synchronization issue. It turns out that I started CI jobs right after @DLu made fixes in more linting commit and let me know about it.
From CI logs I see that colcon test picked up old version without latest changes in more linting commit.
I have tried to rerun CI - and now it passes green.

@DLu Sorry for confusion with false CI warnings. It was surprise for me as well.

@MichaelOrlov MichaelOrlov merged commit 7e91733 into ros2:rolling Apr 1, 2023
@MichaelOrlov MichaelOrlov changed the title Add API samples for Python [rebased] Add API samples for Python Apr 1, 2023
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-4-20-2023/31087/1

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.

None yet

5 participants