lightly-ai / lightly

A python library for self-supervised learning on images.
https://docs.lightly.ai/self-supervised-learning/
MIT License
3.18k stars 285 forks source link

Remove opencv dependency from tests #1477

Open guarin opened 10 months ago

guarin commented 10 months ago

We still have an old opencv dependency in the tests, see here: https://github.com/lightly-ai/lightly/blob/2b215aa5d19b50515f280c1b0181f2f135cbc93f/tests/data/test_VideoDataset.py#L11

We should refactor the test(s) to not rely on opencv and remove opencv from all requirements files.

ishaanagw commented 2 months ago

Hey @guarin, Can I work on this ?

SauravMaheshkar commented 2 months ago

Sure @ishaanagw give it a go 😃

ishaanagw commented 1 month ago

Can you please explain the task a little more please ? Do I have to replace the videoWriters from OpenCV to some other library ?

guarin commented 1 month ago

Can you please explain the task a little more please ? Do I have to replace the videoWriters from OpenCV to some other library ?

Yes the idea would be to replace cv2.VideoWriter with write_video from torchvision in lightly/tests/data/test_VideoDataset.py. Looking at it this could actually be a more tricky issue than anticipated. If you would like to work on it, could you try the following?

If you run into errors regarding video timestamps we most likely can't remove cv2.

ishaanagw commented 1 month ago

Getting this error - ValueError: Expected numpy array with ndim 3 but got 2 in self._test_video_dataset_non_increasing_timestamps()

Is this what you were talking about ?

guarin commented 1 month ago

Interesting, could you post the full stack trace?

Is this what you were talking about ?

No I believe the ValueError already happens before. With the timestamps I meant that depending on which library is used when creating a new video file, the timestamps stored in the file can be different. And I am not sure if write_video creates the exact same timestamps as cv2.VideoWriter.

ishaanagw commented 1 month ago

Stack Trace - tests/data/test_VideoDataset.py:238: in _test_video_dataset_non_increasing_timestamps self.create_dataset(n_videos=2, n_frames_per_video=5) tests/data/test_VideoDataset.py:83: in create_dataset out = torchvision.io.write_video(path, frame, n_frames_per_video) ../miniconda3/envs/lightly/lib/python3.10/site-packages/torchvision/io/video.py:131: in write_video frame = av.VideoFrame.from_ndarray(img, format="rgb24") av/video/frame.pyx:551: in av.video.frame.VideoFrame.from_ndarray

guarin commented 1 month ago

This looks like something is wrong with the dimensions of the numpy arrays. Each frame should have dimension (W, H, C) with C = 3. But somehow the array it got has only 2 dimensions instead of 3.

SGCODEX commented 1 month ago

Can I work on this issue as well @guarin

ishaanagw commented 1 month ago

I think we can remove it, I was able to remove cv2 dependencies but facing some test failure, these test fail with no changes as well.

Failed Test Cases - FAILED tests/data/test_VideoDataset.py::TestVideoDataset::test_video_dataset_dataloader__pyav - TypeError: cannot pickle '_thread.lock' object FAILED tests/data/test_VideoDataset.py::TestVideoDataset::test_video_dataset_non_increasing_timestamps__pyav - TypeError: cannot pickle 'VideoLoader' object

Do you think this is due to some local setup ?

moli-debugger commented 1 month ago

I would like to pick this up as well @guarin

guarin commented 1 month ago

Do you think this is due to some local setup ?

Could be, which operating system are you using? You can also already create a PR and push it, then we can check if it is due to the local setup or not.

@SGCODEX @moli-debugger Its best if one person works on an issue at the time. Please have a look at our other issues for contribution opportunities.

moli-debugger commented 1 month ago

@SGCODEX , please pick up this task . I will continue work on the other issues.

CC: @guarin

ishaanagw commented 1 month ago

Could be, which operating system are you using?

Mac OS

You can also already create a PR and push it, then we can check if it is due to the local setup or not.

Created PR for this - https://github.com/lightly-ai/lightly/pull/1679