image_pipeline icon indicating copy to clipboard operation
image_pipeline copied to clipboard

Saving tar fails in python 3.8

Open PfeifferMicha opened this issue 5 years ago • 12 comments

When pressing "save" in cameracalibrator, the tool fails to save the tarballs and prints:

...
/image_pipeline/camera_calibration/src/camera_calibration/calibrator.py", line 1266, in taradd
    if isinstance(buf, basestring):
NameError: name 'basestring' is not defined

It looks like "basestring" is deprecated in python 3.8. That document recommends replacing basestring with str, however then saving still fails with

Traceback (most recent call last):
  File "/home/user/Projects/ROS/catkin_ws_noetic/src/image_pipeline/camera_calibration/src/camera_calibration/camera_calibrator.py", line 273, in on_mouse
    self.c.do_save()
  File "/home/user/Projects/ROS/catkin_ws_noetic/src/image_pipeline/camera_calibration/src/camera_calibration/calibrator.py", line 578, in do_save
    self.do_tarfile_save(tf) # Must be overridden in subclasses
  File "/home/user/Projects/ROS/catkin_ws_noetic/src/image_pipeline/camera_calibration/src/camera_calibration/calibrator.py", line 1279, in do_tarfile_save
    taradd('left.yaml', self.yaml("/left", self.l))
  File "/home/user/Projects/ROS/catkin_ws_noetic/src/image_pipeline/camera_calibration/src/camera_calibration/calibrator.py", line 1275, in taradd
    tf.addfile(tarinfo=ti, fileobj=s)
  File "/usr/lib/python3.8/tarfile.py", line 1995, in addfile
    copyfileobj(fileobj, self.fileobj, tarinfo.size, bufsize=bufsize)
  File "/usr/lib/python3.8/tarfile.py", line 256, in copyfileobj
    dst.write(buf)
  File "/usr/lib/python3.8/gzip.py", line 276, in write
    data = memoryview(data)
TypeError: memoryview: a bytes-like object is required, not 'str'

I currently do not know enough about tarballs to fix this myself, maybe someone can have a look?

Working on noetic branch, commit 98364014ae3d4aa6ca92a0c7f6e8541ad0354675

PfeifferMicha avatar Jun 24 '20 11:06 PfeifferMicha

@PfeifferMicha thanks for the note. Can you try memoryview(bytes("This is a string", encoding='utf-8'))? That should get you the right types. If that all works out, please submit a PR and we'd be happy to merge it in

SteveMacenski avatar Jun 24 '20 18:06 SteveMacenski

Done, pull request is open. I ended up removing the dependency on StringIO entirely, as using it would give me a lot of troubles downstream. Saving now works on python3.8, Ubuntu 20.04.

Side Note: Since the current head of branch 'noetic' is broken (Charuco integration is halfway done), I made this pull request on top of the previous commit, 98364014ae3d4aa6ca92a0c7f6e8541ad0354675. See also #550. Maybe the Charuco implementation could be moved to another (temporary) branch until it is fixed? Or is 'noetic' considered a development branch? In this case, is there a "stable" noetic branch?

PfeifferMicha avatar Jun 26 '20 07:06 PfeifferMicha

What do you mean its half implemented? (@JStech) Please describe.

SteveMacenski avatar Jun 26 '20 17:06 SteveMacenski

I don't currently have a Noetic install, so I just cherry-picked the commits and trusted CI. Everything is there. I can probably look into it this weekend.

JStech avatar Jun 26 '20 18:06 JStech

I think we need more info from @PfeifferMicha. Functionally noetic and melodic shouldn't really vary much so I want to know what they believe is "halfway done" to know what needs updates.

SteveMacenski avatar Jun 26 '20 18:06 SteveMacenski

By "halfway done" I mean I needed to change quite a few things to get it to not crash. Then once that was done I noticed Stereo Calibration (my actual goal) was not implemented yet. I had closed #550 since I thought this was sort of intentional (with "WIP" in the commit message), I'll reopen it and give more details there.

PfeifferMicha avatar Jun 26 '20 19:06 PfeifferMicha

@JStech can you follow up on #550? Looks like your commit may have introduced some issues.

Stereo for those tags haven't been implemented yet, you're welcome to do so. Its often a pain to implement a new tag for mono/stereo/fisheye so its not expected that someone does all of them (and in some cases the library may not support them)

SteveMacenski avatar Jun 26 '20 19:06 SteveMacenski

Yeah, I'll check it out. "Too many values to unpack" is an error I became very familiar with in implementing this feature, so it's probably my fault. Sorry about that.

And yes, stereo and fisheye are both unimplemented with the ChArUco board at this point because OpenCV doesn't have those functions implemented for ChArUco boards, and, frankly, it's not something I need right now.

JStech avatar Jun 26 '20 20:06 JStech

if its not in openCV, that's enough of a reason for me

SteveMacenski avatar Jun 26 '20 22:06 SteveMacenski

@JStech / @PfeifferMicha Can you please verify that #554 fixed this issue?

JWhitleyWork avatar Jun 29 '20 16:06 JWhitleyWork

With #550 still open, I cannot verify, as (stereo) calibration fails, so I never get to the point where I can save. As soon as #561 is working and merged, I can try to verify this (for noetic, I still don't have a ros2 setup).

PfeifferMicha avatar Jul 08 '20 07:07 PfeifferMicha

@PfeifferMicha Can you please re-test this now that #561 is merged?

JWhitleyWork avatar Jul 29 '20 23:07 JWhitleyWork

Presumed to be fixed by last noted commit, since no further feedback

mikeferguson avatar Jan 17 '24 23:01 mikeferguson