added loop and latency args#58
Conversation
felix-engelmann
left a comment
There was a problem hiding this comment.
Thanks for the addition to make is easier to create a payload by replaying it.
Please add a test that checks if the specified latency is actually used (total time > frames * latency)
For the loop, the tricky part of testing is how to stop it gracefully. But it would be nice if a test could show that the loop processed the same events twice.
| time.sleep(latency) | ||
| time.sleep(latency) | ||
| except StopIteration: | ||
| # gens = [get_internals(f) for f in zmq_files] |
There was a problem hiding this comment.
why is that comment added?
There was a problem hiding this comment.
I think this the end-result of some uncommitted iterations in the code; Bob first just re-initialised gens on StopIteration, but the final version switched to using itertools.cycle. The comment can be removed.
c1914b5 to
0bbd173
Compare
felix-engelmann
left a comment
There was a problem hiding this comment.
Thanks for the test for looping. It requires a bit adjustment and the latency should be tested as well. For other latency testings, I usually set a relatively high latency (e.g. 1s) and then check that e.g. 10 events take longer than 10s. If the latency somehow is not used, it will surely finish in less than 10s and fail the test.
| len_results = len(f.get("results", [])) | ||
| logging.info("Length of results: %s", len_results) | ||
| if len_results > single_run_len_results: | ||
| test_pass = True |
There was a problem hiding this comment.
f["results"] is a group with 11 subgroups: ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '10'], one for each event_number from the replay. The second loop will overwrite the elements once over. This is a general issue with a loop, the event numbers will restart from 0, with all series_start headers. Maybe I would like to better understand the use-case of the loop feature.
There was a problem hiding this comment.
Oh! Is this because of the start frame? I thought the test streams defined in conftest are length 10?
There was a problem hiding this comment.
and in addition, I see in replay that event_number will just be passed though, but in the loop regime, perhaps we could artificially increment past the last recorded event number?
I've drafted this in my last commit.
In addition I added now listening to the stop_event inside the replay loop - so we can stop an infinite replay programmatically.
There was a problem hiding this comment.
Oh! Is this because of the start frame? I thought the test streams defined in
conftestare length 10?
The replay first creates its own recordings through dump_data as a test helper in test_replay.py It defines
dranspose/tests/test_replay.py
Lines 107 to 123 in 5db108b
There was a problem hiding this comment.
This works now - I switched to checking that length is > 11, and I added the stop_event inside the loop for long-running (or infinite) loops so they can still be stopped sensibly (if stop_event gets set while still in the loop, a StopIteration is raised, so that we exit the loop according to the standard process.
| None, | ||
| par_file, | ||
| ), | ||
| kwargs={"port": 5010, "stop_event": stop_event}, |
There was a problem hiding this comment.
if looping should be tested, an additional "loop": True is needed. But then the test does not terminate.
There was a problem hiding this comment.
I resolved this by listening to the stop signal inside the replay loop - seems sensible to me, what do you think?
felix-engelmann
left a comment
There was a problem hiding this comment.
The test for looping with the stop event is very elegant. Thanks!
Now we are just missing a test for the latency, that the replay actually waits.
Another idea to consider: maybe it would be nice instead of looping all events, just start from 1 until n-1 (not the first and last event) which represents actual data.
felix-engelmann
left a comment
There was a problem hiding this comment.
I still don't have a clear usecase for the looping feature, but I like the latency argument. It just needs a test that the replay actually happens slowly enough.
| None, | ||
| par_file, | ||
| ), | ||
| kwargs={"port": 5010, "stop_event": stop_event, "loop": True, "latency": 0.1}, |
There was a problem hiding this comment.
The test sets a latency parameter, but does not check if the replay actually happens "slow" enough
| len_results = len(f.get("results", [])) | ||
| logging.info("Length of results: %s", len_results) | ||
|
|
||
| if len_results > 2 * single_run_len_results: |
There was a problem hiding this comment.
With the event number increasing past the final recorded event, we should check that all the event numbers are sequential and don't e.g. skip an event number.
It seems to me that it makes sense because it allows simulating longer runs even when the recorded data is quite short. On top of that, it's quite a nice thing to be able to test interactive workflows (eg featuring ROIs) with recorded data, without it running out half way through |
Hello this is Bob, ❤️ your work, here's an option to loop the replay data and specify the latency on the command line. xoxox