The Runtime is leaky

[aliddell]

See test failures in this commit vs. successes in this commit in #126. Here's the diff:

diff --git a/tests/test_basic.py b/tests/test_basic.py
index 9f2a98f..8ef4b7d 100644
--- a/tests/test_basic.py
+++ b/tests/test_basic.py
@@ -11,16 +11,10 @@ import pytest
 import tifffile
 
 
-@pytest.fixture(scope="module")
-def _runtime():
-    runtime = acquire.Runtime()
-    yield runtime
-
-
+# FIXME (aliddell): this should be module scoped, but the runtime is leaky
 @pytest.fixture(scope="function")
-def runtime(_runtime: Runtime):
-    yield _runtime
-    _runtime.set_configuration(acquire.Properties())
+def runtime():
+    yield acquire.Runtime()
 
 
 def test_set():
diff --git a/tests/test_zarr.py b/tests/test_zarr.py
index b3cdb5a..6511805 100644
--- a/tests/test_zarr.py
+++ b/tests/test_zarr.py
@@ -16,16 +16,10 @@ import acquire
 from acquire import Runtime, DeviceKind
 
 
-@pytest.fixture(scope="module")
-def _runtime():
-    runtime = acquire.Runtime()
-    yield runtime
-
-
+# FIXME (aliddell): this should be module scoped, but the runtime is leaky
 @pytest.fixture(scope="function")
-def runtime(_runtime: acquire.Runtime):
-    yield _runtime
-    _runtime.set_configuration(acquire.Properties())
+def runtime():
+    yield acquire.Runtime()
 
 
 def test_write_external_metadata_to_zarr(

The only change is setting the scope of the Runtime to "function". Reusing the runtime across tests seems to result in data carryover from one test to the next. Given that these tests are meant to reflect isolated usages of the runtime and not a scripted series of actions, this is an appropriate change. However, users will probably expect to be able to use the runtime in a "memoryless" fashion, so we should address this problem.

[nclack]

It looks like the Optional types might not be handled correctly in some of those zarr tests (or the type signature is wrong).

Any idea what the nature of the problem is?

[aliddell]

It looks like the Optional types might not be handled correctly in some of those zarr tests (or the type signature is wrong).

Any idea what the nature of the problem is?

In the Zarr tests I can see failures of state carrying over from previous runs. Not all state variables getting zeroed back out after calling stop(). But the weirdest one was seeing test_write_external_metadata_to_tiff(), where it runs fine on its own but when runs as part of a suite it crashes here because the file doesn't exist.

[aganders3]

I spent some (probably too much) time looking at this leaky runtime issue, and finally came up with some concrete thoughts on what I think are three separate issues:

The first is that runtime.get_configuration() and runtime.set_configuration(p) are not quite symmetric. set_configuration will skip setting values for invalid streams (source and sink both None), but get_configuration will still return values for invalid streams. So inthe fixture teardown code runtime.set_configuration(acquire.Properties()) is not enough to fully clear the config. This can cause surprising configuration changes between tests, where the runtime is often configured by first getting the config, changing it, then setting it again.

The second issue is that re-configuring a camera may attempt to re-use the existing source stream without closing/opening it. This can cause "data before trigger" in the case of running tests/test_basic.py::test_invalidated_frame before tests/test_basic.py::test_execute_trigger. There is similar code on the sink side, but I'm not sure if it could be similarly problematic.

The third issue is that stopping the runtime does not always flush data, for example if you have not attempted to read it. This can also cause "data before trigger" in the case of running tests/test_basic.py::test_switch_device_identifier before tests/test_basic.py::test_execute_trigger.

I'm probably not understanding all of the context, and am happy to chat about this in standup or another time. Also I can just make a PR for these changes - it's not a big diff. Locally tests/test_basic.py is all green with these changes using a module-scoped runtime fixture (and trying various test orders and combinations).

[aliddell]

@aganders3 if you want to share your fixes, I'd love to see them.