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

Cam2image py #107

Open
wants to merge 16 commits into
base: rolling
Choose a base branch
from
Open

Cam2image py #107

wants to merge 16 commits into from

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Jan 10, 2017

The code on this branch should resolve Issue #85 for quality of service demos in python. I've tested all of the basic functionality of this python code, including:

  • Getting images either from a camera or from the built-in "burger"
  • Flipping the image when an "image_flip" message comes in
  • Changing the image width and height
  • Changing the QoS parameters

I used the C++ "showimage" program to be the subscribing side for all of this testing. For the QoS parameter testing, I tested all of the following combinations:

cam2image_py reliable, showimage reliable - works
cam2image_py best effort, showimage best effort - works
cam2image_py reliable, showimage best effort - works
cam2image_py best effort, showimage reliable - doesn't work

From what Mikael told me, this seems to be the expected behavior.

One other thing I wasn't sure about was how I should setup the directory structure. The demos repository seems to be somewhat inconsistent on this, so I just ended up putting the python files image_tools_py/src. If they should go elsewhere, just let me know and I'll move them.

connects to #85

@mikaelarguedas
Copy link
Member

thanks @clalancette for implementing this! will review this soon, putting the label in review so that it shows up in the right column on waffle

@mikaelarguedas mikaelarguedas added the in review Waiting for review (Kanban column) label Jan 10, 2017
@dirk-thomas
Copy link
Member

The code is currently in a new folder which isn't a package yet. So it won't be compiled and can't be tested on the farm.

Also it should have a test ensuring the demos works as expected as well as linter test (e.g. like https://github.com/ros2/demos/tree/master/demo_nodes_py/test). The linters will have some warnings which should be addressed. Some (but not) all style guides can be found in the developer guide.

@clalancette clalancette added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Jan 11, 2017
@mikaelarguedas
Copy link
Member

This PR is back to in progress based on offline comments, let's review it once it's labeled as is

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Two general comments:

  • We try to use single quotes for strings whenever "possible". We use double quotes when the string contains single quotes to avoid escaping.

  • We try to either wrap all function arguments or none and avoid to only wrap after some. The linters can't check for that atm. The rational is that otherwise all wrapped and aligned lines are dependent on the first line. If you e.g. change parser in this line to p you must update the next following 30 lines (otherwise only one per block).

@dirk-thomas
Copy link
Member

@mikaelarguedas Ups, just saw your comment after finishing reading the patch.

@clalancette
Copy link
Contributor Author

I've now updated the code, which should address all of the comments I've received so far. I don't quite know how to test out that that package.xml and setup.py that I created are good; is there a local test I can do for that? I'm going to mark this as in review again to see if there is anything further. Thanks for the review so far.

@mikaelarguedas
Copy link
Member

One general comment that hasn't been addressed yet is the import statements. We usually import only the modules/classes that we use: e.g. from std_msgs.msg import Bool
This will also allow you to refer to it as Bool later on rather than std_msgs.msg.Bool

Ideally demos are tested using launch_testing that does regex matching on stdout. The c++ image demo you are replicating is a good example (you can do it in pure python rather than using CMake generators). Testing this may be tricky though because I think that OpenCV isn't packaged with python3 bindings, meaning compiling OpenCV3 from source if we want to test the demo...
@ros2/team What do you think about this? Should we reconsider building/testing/packaging separately the demos from the core code to avoid bringing in additional dependencies?

A README on how to run it would be great. Testing it on Windows would be also a requirement before merging it.

@clalancette
Copy link
Contributor Author

Oops, right, I forgot about those bits.

  1. From what I can tell, there doesn't seem to be much of a performance difference between "import foo" and "from foo import bar". In tight loops this can apparently make a difference since in the former case you have to look up "foo" in the global namespace, and then find the method, but that doesn't really apply here. So it really just comes down to style. I prefer to be more explicit in the namespacing, so I always prefer "import foo", but if the general consensus is to use "from foo import bar", I can make that change.

  2. I'll add a README describing the fact that you need to compile OpenCV3 with Python3 bindings in order for this to work, and describing how to run it once you have that.

  3. For the testing bit, it doesn't seem like any of the other python examples/demos have tests for the functionality, so there isn't a lot to look up as reference. I'll see what I can do to make this work for python.

  4. Finally, for Windows support, I'd need to have access to a Windows machine where I could try compiling some of this stuff. Are there any around that I could use?

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

This looks good!
a few comments to address plus a bunch of cosmetics ones

image_tools_py/package.xml Show resolved Hide resolved
image_tools_py/package.xml Outdated Show resolved Hide resolved
image_tools_py/setup.py Outdated Show resolved Hide resolved
image_tools_py/src/burger_py.py Outdated Show resolved Hide resolved
image_tools_py/src/cam2image_py.py Outdated Show resolved Hide resolved
image_tools_py/src/showimage_py.py Outdated Show resolved Hide resolved
image_tools_py/src/showimage_py.py Outdated Show resolved Hide resolved
image_tools_py/package.xml Outdated Show resolved Hide resolved
image_tools_py/src/image_qos_profile.py Outdated Show resolved Hide resolved
image_tools_py/src/showimage_py.py Outdated Show resolved Hide resolved
@mikaelarguedas
Copy link
Member

For the testing bit, it doesn't seem like any of the other python examples/demos have tests for the functionality, so there isn't a lot to look up as reference. I'll see what I can do to make this work for python.

Yeah I understand that this can be confusing: The policy is that examples are not tested but demos / tutorials are. There were no python demos hence no testing using launch_testing on any python package. demo_nodes_py should be tested though, it's just that we didn't have time to do it before Beta1.

This testing bit can be added later once we decided what we will do about the OpenCV3 with python3 bindings situation

@clalancette
Copy link
Contributor Author

I just pushed some additional commits that addressed most of Mikael's review, plus added a README. I'm still looking at getting things working in Windows.

@clalancette
Copy link
Contributor Author

I just tested this out on Windows, using the instructions provided here: https://github.com/ros2/ros2/wiki/Windows-Development-Setup . In addition to those instructions, I also had to install the packages for opencv 3 with python 3 and numpy. I did that by downloading those packages from http://www.lfd.uci.edu/~gohlke/pythonlibs/ , then installing them with pip.

With all of that done, I was able to run this demo on Windows without changes. It does seem pretty slow, with the subscriber (showimage) running behind the publisher, but it is working (some of this may have to do with the fact that it is in a VM, but I'm not sure).

Other than the packaging/testing situation, is this ready to go? If so, how should we proceed here?

@wjwwood
Copy link
Member

wjwwood commented Jan 12, 2017

Well, we should either update the instructions to install OpenCV 3 or change this so that it can use OpenCV 2.4 (as we advise installing now) if that it possible. I don't think we've been using OpenCV 3 because it isn't available by default from apt-get (could be wrong about that now that we have Xenial rather than Trusty as our target). Also the installation instructions should add numpy if that is required for this demo. That is unless we don't intend this demo to work out of the box, and instead if someone wants to try it then they'll have to do some more stuff (like install OpenCV 3 and numpy) in order to do so. I haven't been following this issue very closely, so I don't know what the desired result is.

Related to that is packaging, which would be impacted by the need for a different OpenCV version. Again, we have a choice about when to test and package this, as we do with the ros1 bridge, but it would obviously be better if we always built for it.

@clalancette
Copy link
Contributor Author

Right, so the basic problem here is that the OpenCV bindings for Python 3 only came along with OpenCV 3.0, which is not available in Ubuntu or Debian yet. So in order to make this work, you have to build/install OpenCV 3.x from source. I'm not sure how we resolve that in the near term; it's not clear to me when OpenCV 3.0 will be available in Ubuntu.

@wjwwood
Copy link
Member

wjwwood commented Jan 12, 2017

I see, that make sense.

It still leaves the question though, do we try to update our instructions to install OpenCV 3 for all platforms (using a ppa or something else for ubuntu) or do we just expect this demo will not be working/tested out of the box.

I suppose we could look at what you're doing with OpenCV (I assume mostly capturing webcam data or synthesizing images) and try to replace that with dependencies that are available for Python 3 on Xenial, but that sounds counter productive to me.

@clalancette
Copy link
Contributor Author

Yeah, replacing the OpenCV3 stuff had occurred to me as well. But given that this is a demonstration that we want other people to look at, it seems like hand-coding all of that stuff isn't exactly what we want to promote.

@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Apr 6, 2017
@mikaelarguedas mikaelarguedas modified the milestone: untargeted Feb 22, 2018
@clalancette
Copy link
Contributor Author

After a very long hiatus on this, with the switch to bionic I think we may be able to put this back into review. The only potential downside here is that this demo will not work on Xenial CI; but we may be able to work around that somehow. In any case, I'm going to run a CI here so we can see where it is at, then place it back into review:

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

@dirk-thomas
Copy link
Member

dirk-thomas commented Jul 18, 2018

One of the very early comments hasn't been addressed yet:

it should have a test ensuring the demos works as expected

@mikaelarguedas
Copy link
Member

I'm going to run a CI here

As this doesn't have any test or code to compile, is the goal of running CI just to check for linter violations?

I think it would be valuable to update the code to latest master and clarify the targeted platforms / installation and testing strategy before diving into a full review

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM but for a few small observations. I tested it locally and it works as expected, though:

  • Upon Ctrl-C, KeyboardInterrupt is not handled. Maybe we can catch it to preclude the stack trace?
  • When -s 1 is passed, upon closing the window it just gets re-opened. Not sure if it's a bug or a feature.

image_tools_py/README.md Outdated Show resolved Hide resolved
image_tools_py/README.md Outdated Show resolved Hide resolved
image_tools_py/README.md Outdated Show resolved Hide resolved
image_tools_py/README.md Show resolved Hide resolved
image_tools_py/README.md Outdated Show resolved Hide resolved
image_tools_py/README.md Outdated Show resolved Hide resolved
image_tools_py/test/test_showimage_cam2image.py.in Outdated Show resolved Hide resolved
image_tools_py/README.md Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

* Upon Ctrl-C, `KeyboardInterrupt` is not handled. Maybe we can catch it to preclude the stack trace?

Good call. Will fix.

* When `-s 1` is passed, upon closing the window it just gets re-opened. Not sure if it's a bug or a feature.

As far as I can tell, that's a "feature" of OpenCV highgui. I don't know how to change it, so I'll leave it for now.

Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM but we should run CI one last time before merging!

@clalancette
Copy link
Contributor Author

LGTM but we should run CI one last time before merging!

Yeah, before we merge this we need to install opencv-python on the Windows CI machines. I'm working on it now, then I'll run CI again :). Thanks!

@clalancette
Copy link
Contributor Author

I realized that I actually needed another PR to the CI scripts so that opencv-python will get properly installed in both Windows Release and Debug mode; that is ros2/ci#317 . A new CI, using that PR plus this one:

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

@clalancette
Copy link
Contributor Author

clalancette commented Jul 11, 2019

So, the latest commit should fix the test failure on Windows, and make the timeout checking more robust.

However, while debugging that test failure on Windows, I ran into another bug. For reasons I don't understand, when the launch file quits, it does not kill off the spawned {cam2image_py, showimage_py} python processes. Instead, they remain running as "Background processes", apparently forever. This happens regardless of whether the test completes successfully or unsuccessfully. I'm not quite sure how to go about debugging this, but until that problem is fixed I don't think this can go in.

@hidmic Any thoughts about what might be going on there?

@hidmic
Copy link
Contributor

hidmic commented Jul 11, 2019

@clalancette do these processes behave upon SIGTERM on Windows?

@clalancette
Copy link
Contributor Author

@clalancette do these processes behave upon SIGTERM on Windows?

So, I'm not 100% sure, but I think the answer is no.

What I did was to run cam2image_py by:

cd install\image_tools_py\Lib\image_tools_py
cam2image_py -b

In another terminal, I did:

pslist
pskill <PID-corresponding-to-cam2image_py>

When I did that, I got returned to a console in the first terminal, but the process kept running. If I then found the PID of the python process, and killed that, the whole thing stopped.

So there may be a more generic problem with monitoring/killing python processes on Windows. I'm still not sure, though; do we have examples anywhere else?

@hidmic
Copy link
Contributor

hidmic commented Jul 11, 2019

Test on https://github.com/ros2/system_tests use launch_testing extensively.

@hidmic
Copy link
Contributor

hidmic commented Aug 15, 2019

@clalancette this may be related with ros2/launch#306.

clalancette and others added 16 commits August 16, 2019 14:40
This is useful for showing off dropped messages with QOS
changes.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Make it use the new test infrastructure.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

@clalancette this may be related with ros2/launch#306.

And that change does indeed fix the zombie process problem for me. I left a comment over there on the (now-closed) pull request.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants