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

Latest apt release for ROS Noetic on Ubuntu 20.04 has int64 id instead of string id #56

Closed
r0gi opened this issue Apr 12, 2021 · 10 comments · Fixed by #57
Closed

Latest apt release for ROS Noetic on Ubuntu 20.04 has int64 id instead of string id #56

r0gi opened this issue Apr 12, 2021 · 10 comments · Fixed by #57
Assignees

Comments

@r0gi
Copy link

r0gi commented Apr 12, 2021

OS: Ubuntu 20.04 (focal)

Installed a fresh ROS Noetic following the official instructions but installing vision_msgs via apt doesn't install the latest version

sudo apt update
sudo apt install ros-noetic-vision-msgs

apt show ros-noetic-vision-msgs

Package: ros-noetic-vision-msgs
Version: 0.0.1-1focal.20210112.080130
Priority: optional
Section: misc
Maintainer: Adam Allevato <REDACTED>
Installed-Size: 833 kB
Depends: ros-noetic-geometry-msgs, ros-noetic-message-generation, ros-noetic-message-runtime, ros-noetic-sensor-msgs, ros-noetic-std-msgs
Download-Size: 48.2 kB
APT-Manual-Installed: yes
APT-Sources: http://packages.ros.org/ros/ubuntu focal/main amd64 Packages
Description: Messages for interfacing with various computer vision pipelines, such as object detectors.

rosmsg show vision_msgs/ObjectHypothesisWithPose

int64 id
float64 score
...

Which is different to the latest Noetic API reference, which shows the correct string id.

@SteveMacenski
Copy link
Member

I'm not entirely sure - @Kukanani? I see 1.0.0 released last off noetic but I don't see 1.0.0 in the package.xml tags of the noetic branch. I still see 0.0.1 in rosdistro https://github.com/ros/rosdistro/blob/master/noetic/distribution.yaml#L8048 so that 1.0.0 wasn't bloomed up?

@Kukanani
Copy link
Collaborator

Kukanani commented Apr 14, 2021

@r0gi sorry for the confusion.

There has not been a release since 0.0.1 in ROS1. The commit tree is fairly messy; I think that most changes that are on noetic-devel should be in master instead. See screenshot from gitk at the end of this post.

I would bloom up another release for noetic but this will represent a breaking change for current package users. I seem to remember it being a general policy not to push breaking changes to message definitions after a ROS distro enters regular usage. @SteveMacenski is this still a good rule of thumb?

I propose the following:

  • Hard reset (or add revert commits) to put noetic-devel back to compatibility with 0.0.1, plus bugfix/style/doc changes. This will fix the docs to match the released package
  • Reset master to where noetic-devel currently points. This will ensure that there is still an up-to-date bleeding edge version of the package (with string id)
  • No new release. This will ensure no breaking changes for current users.

Screenshot from 2021-04-14 09-15-50

@SteveMacenski
Copy link
Member

@SteveMacenski is this still a good rule of thumb?

Still the right answer -- was that released before we moved this into ros-perception? I suppose I missed that. We should probably revert any changes from that release in the noetic branch if we're not going to re-release with changes.

I suggest to delete master, what value does it really provide if we don't release it and the changes between 0.0.1 and 1.0.0 are mostly superficial? We're not going to be able to change noetic / ROS1 distros so maintaining that master branch doesn't really do anything since it won't go anywhere in the future. The ROS2 branch is the "new master" w.r.t. changes being actually used in future distros

@Kukanani
Copy link
Collaborator

Kukanani commented Apr 16, 2021

was that released before we moved this into ros-perception?

Yes, I believe so.

I suggest to delete master, what value does it really provide if we don't release it and the changes between 0.0.1 and 1.0.0 are mostly superficial?

Yes, I'll remove. At the time, I was expecting at least one more ROS1 release. It doesn't serve much purpose now. I guess that master could potentially be used to ease the transition from ROS1 to ROS2, or to help fix compatibility issues like #39 for all the changes we recently made to the package. In other words, it could be the "ros1 version of the ros2 message definitions". However this is very confusing and would require additional maintenance effort going forward, so I think it's best to just remove.

@Kukanani
Copy link
Collaborator

master is no more.

@SteveMacenski
Copy link
Member

It has been reverted

@r0gi
Copy link
Author

r0gi commented Apr 18, 2021

Thank you, could you also trigger an update for the API reference so that it is consistent with the noetic release?
From the linked page it says "autogenerated on Tue, 21 Jul 2020 03:42:48" so it is still showing the old type.

@Kukanani
Copy link
Collaborator

I don't think I have the ability to update the API reference. Looking at the dates, it seems that the last autogeneration was a few days after the last commits were pushed to noetic-devel. Maybe we just need to wait a few more days for the changes to be picked up?

@mintar
Copy link
Contributor

mintar commented Sep 12, 2022

In rospy (ROS 1), you can set any value to any field. You could also do crazy stuff like:

vision_msgs.ObjectHypothesis(id=[1, 2, None, {"foo": 1}, "bar"])

... and it works (but it shouldn't). BUT: Once you try to publish that message, you'll get an error.

You can test whether a field is the correct type with this little function I wrote:

def serialize_deserialize(message):
    """
    Serialize and then deserialize a message. This simulates sending a message
    between ROS nodes and makes sure that the ROS messages being tested are
    actually serializable, and are in the same format as they would be received
    over the network. In rospy, it is possible to assign an illegal data type
    to a message field (for example, `message = String(data=42)`), but trying
    to publish this message will throw `SerializationError: field data must be
    of type str`. This method will expose such bugs.
    """
    from io import BytesIO

    buff = BytesIO()
    message.serialize(buff)
    result = message.__class__()  # create new instance of same class as message
    result.deserialize(buff.getvalue())
    return result

So in short: The id type must be an integer.

@zkytony
Copy link

zkytony commented Sep 12, 2022

Hi @mintar Thanks for the reply. I soon realized that and removed my post.

To people who read this later, I asked a question where I can create ObjectHypothesis with a string id field, even though the API reference says the field is integer.

I am moving forward with using the integer id.

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 a pull request may close this issue.

5 participants