NOMRG Convert temp_video into a fixture

[NicolasHug]

Attempt at #3922 (comment)

It's a bit tricky as the fixture needs to be called with different parameters. I'm not sure how to best do this, but this solution is inspired from https://docs.pytest.org/en/latest/example/parametrize.html#indirect-parametrization.

In this specific case, the fixture doesn't bring much to the table (there's no significant time saving), and makes the tests a bit cryptic, so I wouldn't merge this.

@pmeier perhaps you're aware of a better solution for this?

[NicolasHug]

This seems to work as expected as I only see 2 print outputs over all tests:

fixture called once with {'num_frames': 10, 'height': 300, 'width': 300, 'fps': 5, 'lossless': True}
fixture called once with {'num_frames': 20, 'height': 30, 'width': 30, 'fps': 10, 'lossless': True}

[NicolasHug]

I just commented out the rest to avoid errors

[pmeier]

I'm not sure if they are "better", but I can think of two more options:

1. Instead of using @pytest.mark.parametrize to parametrize we could write a custom decorator, that does the same internally, but makes the UX easier:

   def temp_video_params(num_frames: int = 10, ...):
        return @pytest.mark.parametrize(
           "temp_video",
           dict(num_frames=num_frames, ...),
           indirect=True,
       )

2. We could add a factory fixture:

   @pytest.fixture(scope="session")
   def temp_video_factory():
       def temp_video_factory_(num_frames: int = 10, ...):
           return ...
   
       return temp_video_factory_
   
   @pytest.fixture
   def temp_video(temp_video_factory):
       return temp_video_factory()

   def test_foo(temp_video):
       ...
   
   def test_bar(temp_video_factory):
       temp_video = temp_video_factory(num_frames=20)
       ...

In both cases we have the option to not specifying any special attributes, but go with sensible defaults instead. I like option 2 better, since it works well with other parametrizations:

@pytest.mark.parametrize("num_frames", [10, 20])
def test_bar(temp_video_factory, num_frames):
    temp_video = temp_video_factory(num_frames=num_frames)
    ...

[pmeier]

On a side note: I dislike the name temp_video. Although it is accurate description, the test itself should not care if the video is temporary or not. So maybe we can go for test_video or just video?

[NicolasHug]

Thanks for looking into this @pmeier !

The problem with option 2 is that it will not make use of the fixture caching, so the video will be created multiple times: it will cache the outer factory but not the inner video creation. Also, the inner function temp_video_factory_ still needs to be a context manager, so we're not able to take advantage of the fixture teardown functionality, and we still need to call it with with temp_video_factory_(...) as ...:

Basically I'm looking for a way to have a "parametrized" fixture ("parametrized" not in the pytest sense here, just the regular sense), that:

• can cache the video module-wise
• automatically tearsdown stuff
• is simple to write.

My version in this diff currently fails on the last one ^^

[pmeier]

The problem with option 2 is that it will not make use of the fixture caching, so the video will be created multiple times: it will cache the outer factory but not the inner video creation.

True, but we have the temp_video fixture for that. If we set it to scope="module", we create the "default" video only once per module. If we need caching of "special" videos per module, that would warrant a fixture for each:

@pytest.fixture(scope="module")
def special_temp_video(temp_video_factory):
    return temp_video_factory(num_frames=42)

---

Also, the inner function temp_video_factory_ still needs to be a context manager, so we're not able to take advantage of the fixture teardown functionality, and we still need to call it with with temp_video_factory_(...) as ...:

My bad, I was under the impression temp_video is a tensor rather than a file. We don't need a context manager for that functionality. By using yield over return we have the option to perform manual cleanup:

@pytest.fixture(scope="session")
def temp_video_factory():
    temp_videos = set()

    def temp_video_factory_(num_frames: int = 10, ...):
        ...
        temp_videos.add(temp_video)
        return temp_video

    yield temp_video_factory_

    for temp_video in temp_videos:
        os.remove(temp_video)

If we use the tmp_path_factory fixture instead of generating a temporary directory / file ourselves, pytest will probably even handle the cleanup for us.

[NicolasHug]

True, but we have the temp_video fixture for that. If we set it to scope="module", we create the "default" video only once per module.

I'm not sure what you mean by that. No matter the scope, the inner function never gets cached, it's only the outer one

If we need caching of "special" videos per module, that would warrant a fixture for each

In this specific case there are only a small set of parameters with which we want to call the fixture so that is doable, but not in general.

[NicolasHug]

By using yield over return we have the option to perform manual cleanup:

Could be wrong but I think that turns the funciton into a generator and we'll have to use next or something? Oh nvm it's the fixture, ok. But that still doesn't cache the video creation I think

[pmeier]

Here is a minimal example using the factory strategy:

import pytest
import functools


@pytest.fixture(scope="session")
def video_factory(tmp_path_factory):
    @functools.lru_cache()
    def video_factory_(*, num_frames: int, extension: str):
        name = f"video-{num_frames}.{extension}"
        print(f"Creating {name}")
        return tmp_path_factory.mktemp(name)

    # we cannot cache this directly, since functools.lru_cache is not able to handle default values. 
    # Thus, calling video_factory_with_defaults() and video_factory_with_defaults(num_frames=10) 
    # would count as two separate calls, which is not desired here.
    def video_factory_with_defaults(num_frames: int = 10, extension="avi"):
        return video_factory_(num_frames=num_frames, extension=extension)

    return video_factory_with_defaults


@pytest.fixture(scope="module")
def video(video_factory):
    return video_factory()


def test_foo(video):
    assert video.exists()


def test_bar(video):
    assert video.exists()


@pytest.mark.parametrize("num_frames", [10, 20])
def test_spam(video_factory, num_frames):
    video = video_factory(num_frames=num_frames)
    assert video.exists()


@pytest.mark.parametrize("num_frames", [20, 30])
def test_ham(video_factory, num_frames):
    video = video_factory(num_frames=num_frames)
    assert video.exists()

If you run this with the -s flag you can observe that we only create three videos in total with 10, 20, and 30 frames respectively.

[NicolasHug]

Thanks @pmeier ! This seems to address most use-cases for this specific file. As discussed offline, there are a few caveats for a more general case though:

• since video_factory_ isn't a fixture, we can't have a setup/teardown logic inside of it: using yield would convert it into a generator
• the scope of the lru_cache isn't synced with the scope of the video_factory outer fixture.

I feel like none of the solutions are worth merging at this point, as here the video creation is really cheap. But this was a good exercise that might be useful in the future! Thanks again

[pmeier]

I feel like none of the solutions are worth merging at this point, as here the video creation is really cheap.

Depending on the usage of a "default" video throughout the test suite, for example

vision/test/test_io.py

Line 76 in 964ce1e

with temp_video(10, 300, 300, 5, lossless=True) as (f_name, data):

we might still create a temp_video fixture like

@pytest.fixture
def temp_video_fixture():
    with temp_video(10, 300, 300, 5, lossless=True) as (f_name, data):
        yield

to adhere a little more to pytest best practices.