regression on kinetic due to #127

[jorgenfb]

The changes in #127 (https://github.com/ros/genpy/blob/kinetic-devel/src/genpy/generator.py#L761) adds the line:

codecs.lookup_error("rosmsg").msg_type = self._type

to deserialization. On kinetic this line throws LookupException with the message: unknown error handler name 'rosmsg'

The issue can be reproduced by using any package that has regenerated its messages since #127. The package where I discovered it was with rosbridge_suite.

This is how to reproduce it:

# Boot up the minimal ros image in docker
docker run -it -p 11311:11311 ros:kinetic-ros-core

# Update apt and install ros-kinetic-rosbridge-suite
apt update
apt install ros-kinetic-rosbridge-suite

# Start roscore
roscore

On the host computer run docker ps to get the container id.

Log into the container and start rosapi (from rosbridge_suite):

docker exec -it <CONTAINER ID> bash

# Inside the container
source /opt/ros/kinetic/setup.bash
rosrun rosapi rosapi_node

Log into the container and call service:

docker exec -it <CONTAINER ID> bash

# Inside the container
source /opt/ros/kinetic/setup.bash
rosservice call /rosapi/get_param what ever

The service call will fail when trying to deserialize the message.

This can probably also be done on your local machine, but I choose docker to make sure we have the same environment.

[dirk-thomas]

The problem is that you are mixing older and newer Debian packages.

After these instructions:

docker run -it -p 11311:11311 ros:kinetic-ros-core
apt update
apt install ros-kinetic-rosbridge-suite

you don't have the latest version of genpy yet. dpkg -l | grep genpy shows version 0.6.7 (which doesn't contain #127). But the newly installed version of ros-kinetic-rosbridge-suite was generated with the latest version of genpy and therefore expect it to be available.

@mikepurvis FYI since you provided the original patch.

Once the Docker image is being updated the problem should go away. Until then you can invoke apt dist-upgrade to make sure that all already installed packages are updated - after that you should have version 0.6.14 (which contains #127) and the problem should go away.

[mikepurvis]

Yeah, we don't have a good story for expressing a reverse dependency like this— like, everything that builds against pkg X needs to then have a >= run-depend on that version of pkg X.

[mikepurvis]

I suppose we could anticipate this problem and catch the LookupError with a no-op; I don't really have strong opinions either way on it.

[dirk-thomas]

I don't really have strong opinions either way on it.

Me neither. Expecting users to have an up-to-date installation isn't too much imo. The problem that the ros:* Docker images aren't up-to-date is a long standing one (osrf/docker_images#112).

If someone wants to make a PR to make it more defensive I am happy to review. There will be another Kinetic rebuild (due to a ros_comm patch version) coming soon so it wouldn't imply a huge overhead if done now-ish. Otherwise it would trigger a huge rebuild and re-download by users which is questionable if that is worth it.

[gwendolynbrook]

Posting for anyone else who's seen a similar regression for ros-melodic on 18.04. This changed caused rostopic echo and all of our python subscribers to fail "silently" for any custom message using this base image:

FROM arm64v8/ubuntu:18.04 as build

ENV MB_ROBOT_WS="/home/robot" \ LC_ALL=C.UTF-8 \ DEBIAN_FRONTEND=noninteractive \ ROS_DISTRO=melodic

with the following packages installed, and several custom messages built.

ros-${ROS_DISTRO}-ros-core
ros-${ROS_DISTRO}-angles
ros-${ROS_DISTRO}-bondcpp
ros-${ROS_DISTRO}-pluginlib
ros-${ROS_DISTRO}-robot-state-publisher
ros-${ROS_DISTRO}-roslint
ros-${ROS_DISTRO}-tf
ros-${ROS_DISTRO}-xacro

ros-${ROS_DISTRO}-uuid-msgs
ros-${ROS_DISTRO}-eigen-conversions
ros-${ROS_DISTRO}-tf2-geometry-msgs
ros-${ROS_DISTRO}-cmake-modules

We see the following error when subscribing to any user defined message type. Note that we are still using python2.7 (:facepalm: ), but this generated code is leaking in where it should not be.

in deserialize codecs.lookup_error("rosmsg").msg_type = self._type LookupError: unknown error handler name 'rosmsg'

Known working .deb:
| 2693. Selecting previously unselected package ros-melodic-genpy.
| 2693. Preparing to unpack .../388-ros-melodic-genpy_0.6.12-1bionic.20200530.174122_arm64.deb ...
| 2693. Unpacking ros-melodic-genpy (0.6.12-1bionic.20200530.174122)

(apparently) broken .deb:
| 131.7 Selecting previously unselected package ros-melodic-genpy.
| 131.7 Preparing to unpack .../389-ros-melodic-genpy_0.6.14-1bionic.20200813.171631_arm64.deb ...
| 31.7 Unpacking ros-melodic-genpy (0.6.14-1bionic.20200813.171631) ...

[dirk-thomas]

@gwendolynbrook did you run an apt update && apt dist-upgrade in the Docker container to ensure you have the latest packages? Then you shouldn't get this error.

[gwendolynbrook]

@gwendolynbrook did you run an apt update && apt dist-upgrade in the Docker container to ensure you have the latest packages? Then you shouldn't get this error.

update, yes; dist-upgrade no. What package is the dist-upgrade affecting that's a dependency to get this working? I should be clear that we're building a custom image and installing ros + all ros packages ourselves, so any ros packages are freshly installed -- I wouldn't expect them to need an upgrade. Is there a system package or other dependency involved?

Also -- have confirmed that the issue persists after dist-upgrade (which is a no-op for the ros packages).

[gwendolynbrook]

Still grok'ing the generator code, but I'm wondering why this line isn't also guarded by an "if python3":

https://github.com/ros/genpy/pull/127/files#diff-cda8e9d50594619a9ea61bdd3216b820R759

(i.e. #133)

[dirk-thomas]

@gwendolynbrook Please provide reproducible steps for your case.

We see the following error when subscribing to any user defined message type. Note that we are still using python2.7, but this generated code is leaking in where it should not be.

While codecs.lookup_error("rosmsg").msg_type = self._type isn't necessary (since unused) with Python 2 the custom error handler is registered unconditionally (https://github.com/ros/genpy/pull/127/files#diff-f4626899ff80377c1eaf9d092e00785eR87) and therefore should be available independent of the used Python version.

I still don't see how you can run into this if you have the latest version of the genpy package. So in order to determine the severity of #134 please provide reproducible steps. If this is a general problem it needs an immediate release / sync, if it is a usage problem it is only a minor improvement to avoid the overhead when not being used and doesn't justify a new release at this time.

[dirk-thomas]

@gwendolynbrook Can you please reply to the above comment.

[gwendolynbrook]

@gwendolynbrook Please provide reproducible steps for your case.

We see the following error when subscribing to any user defined message type. Note that we are still using python2.7, but this generated code is leaking in where it should not be.

While codecs.lookup_error("rosmsg").msg_type = self._type isn't necessary (since unused) with Python 2 the custom error handler is registered unconditionally (https://github.com/ros/genpy/pull/127/files#diff-f4626899ff80377c1eaf9d092e00785eR87) and therefore should be available independent of the used Python version.

I still don't see how you can run into this if you have the latest version of the genpy package. So in order to determine the severity of #134 please provide reproducible steps. If this is a general problem it needs an immediate release / sync, if it is a usage problem it is only a minor improvement to avoid the overhead when not being used and doesn't justify a new release at this time.

@dirk-thomas .... me neither 🤦

Here's my attempt to repro: kinetic-devel...Maidbot:test-serde-issue (tl;dr -- I'm unable to reproduce in a unit test case; or a simple "functional" test in the container using rostopic pub with the generated test_pkg/Range).

After taking a much closer look at my build logs, it looks like the issue was a multi-stage build. We install roscore in a build stage and a (size-optimized) run stage; and copy build products from the first to the second. It looks like the ros core install was cached in the run stage, but not in the build stage -- so our messages would have been generated by 0.6.14, but used by 0.6.12 at runtime; I'm working on confirming this now.

[fmessmer]

We started to see a similar error during one of our unittests that started to fail recently for both kinetic and melodic - without any changes to the test itself:

<?xml version="1.0" encoding="utf-8"?>
<testsuite errors="1" failures="0" name="unittest.suite.TestSuite" tests="1" time="1.991"><testcase classname="__main__.PythonMoveGroupTest" name="test_validation" time="0.6440"><error type="LookupError">unknown error handler name 'rosmsg'
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/root/target_ws/src/mojin_manipulation/mojin_manipulation_tests/test/python_move_group.py", line 60, in test_validation
    conversions.msg_from_string(plan1_traj, plan1)
  File "/opt/ros/melodic/lib/python2.7/dist-packages/moveit_commander/conversions.py", line 53, in msg_from_string
    msg.deserialize(data)
  File "/opt/ros/melodic/lib/python2.7/dist-packages/moveit_msgs/msg/_RobotTrajectory.py", line 249, in deserialize
    codecs.lookup_error("rosmsg").msg_type = self._type
</error></testcase><system-out>&lt;![CDATA[
]]&gt;</system-out><system-err>&lt;![CDATA[
]]&gt;</system-err></testsuite>

the effected lines are:

            plan1 = self.plan(group, rostest1)
            plan1_traj = RobotTrajectory()
            conversions.msg_from_string(plan1_traj, plan1)

where one of MoveIt!'s internal conversion functions is used: https://github.com/ros-planning/moveit/blob/master/moveit_commander/src/moveit_commander/conversions.py

If indeed #134 fixes this issue, could we please get this released into kinetic and melodic?
If this is a different issue, let me know what other information is helpful to get things fixed...

[dirk-thomas]

If this is a different issue, let me know what other information is helpful to get things fixed...

@fmessmer Do you have the latest version of genpy installed when you run into this? If no, please make sure to update to the latest version. If yes, please provide reproducible steps.

[fmessmer]

@dirk-thomas I can confirm that having genpy and moveit_msgs from source in our upstream_workspace lets our tests pass again...thus, I hope #134 will get released for kinetic and melodic some time soon
thanks!

[dirk-thomas]

I can confirm that having genpy and moveit_msgs from source in our upstream_workspace lets our tests pass again...thus, I hope #134 will get released for kinetic and melodic some time soon
thanks!

@fmessmer The referenced PR is not planned to be released anytime soon if the already released version is sufficient (simply because a full rebuild requires a lot of resources and all users have to download new binary packages). So please provide more information if the latest available binary package isn't sufficient.