pyxdf
pyxdf copied to clipboard
xdf-modules
135 playback lsl - Fix for errors when not looping or when sleep time is shorter than sample interval
LGTM! I assume you have tested this and the changes fix the issues. Would it be possible to add test cases which fail on main but pass here?
I would prefer to add it as an extra, because users would also need pylsl in order to use the playback CLI.
I added a test that simply attempts to playback one of the files already in the example-files folder. This test fails without the change to the loop argument in this PR.
(FYI: Adding this test also prompted me to add a noble release to liblsl -- glad to have that done.)
However, I do not wish to test the other problem this PR fixes which was the early termination when the time between samples was larger than the sleep time. This is rather difficult to test because it requires a new contrived test xdf file with long time intervals and a separate thread to read the stream and verify the entire contents were passed through.
Why can you not just use pylsl?
pylsl does not come with the liblsl binaries on Linux and Mac because it assumes you'll have liblsl installed at the system level.
Yes.
Previously we were shipping binaries inside the wheels for all platforms. However, the MacOS binaries were annoying to maintain because of security issues. The linux binaries were causing more problems than they were worth because the docker containers that were available to produce manylinux binaries were not producing binaries that were meeting the needs of all users and it was actually worse to provide non-working binaries than it was to provide no binaries with a helpful error message. Packaging tools were not able to produce bespoke limited-scope wheels that pypi would accept and would only be provided to platforms on which they worked without also feeding them to platforms on which they didn't work.
Is this going to be a problem for this PR?
Thanks for clarifying. I think this is problematic, because I am hesitant to add a dependency (even if optional) which requires manual tinkering. Therefore, I'd rather not include the test and the pylsl dependency (and return to the status quo).
I'm happy to discuss pylsl packaging in a separate issue. I do have some limited experience with cibuildwheel, which I use to create binary wheels for SleepECG (which contains a C extension). Ideally, pylsl should provide self-contained binary wheels for all three platforms, since this has been the standard way to ship such things for quite some time (and this is also what users will expect). Maybe this is worth another look (tools might have matured since you last tried)?
There’s non-Debian, arch Linux, Android, all the different flavours of arm (pi), etc. These are all platforms where users complained that pylsl wasn’t working for them. The thing about these users is they are typically able to follow the instructions in an error message to install missing liblsl, but they get stuck when they have an incompatible liblsl.so.
Anyway, I’ll edit the PR to remove the tests when I have a moment.
There’s non-Debian, arch Linux, Android, all the different flavours of arm (pi), etc. These are all platforms where users complained that pylsl wasn’t working for them. The thing about these users is they are typically able to follow the instructions in an error message to install missing liblsl, but they get stuck when they have an incompatible liblsl.so.
Yeah, but this should not be a problem nowadays. Even ARM64 builds work out of the box for me.