Open aliddell opened 10 months ago
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?
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.
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).
@aganders3 if you want to share your fixes, I'd love to see them.
See test failures in this commit vs. successes in this commit in #126. Here's the diff:
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.