Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Build and bundle liblsl in wheels #351

Merged
merged 118 commits into from
Nov 14, 2024
Merged

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Oct 31, 2024

  • Add liblsl as a git submodule, with v1.16.2 checked out
  • Add tiny cffi module that will be compiled during installation that exposes one function from liblsl. This will make it so that once we...
  • Add cibuildwheel config (and CI run), auditwheel and related utils will pull liblsl into the wheel for us
  • Get things working locally on Linux
  • Get things working on CIs:
    • Linux
    • macOS
    • Windows
  • Add CI job that depends on the wheel build job that downloads and tests the built wheel (clean room)
  • Remove remote LSL fetching code (_load_liblsl_mne_lsl, _fetch_liblsl no longer necessary now that pip wheels and conda should work automatically?)
  • Migrate publish.yml to build_wheels.yml (or vice-versa)
  • Reduce number of tests in pytest.yml (probably just high/low i.e. 3.10 and 3.13, no need for 3.11 and 3.12?) to reduce CI burden

Locally on Linux things seem to work:

$ CIBW_BUILD="cp312-*" cibuildwheel
...
$ pip install wheelhouse/mne_lsl-1.7.0.dev0-cp310-abi3-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
...
$ MNE_LSL_LOG_LEVEL=debug python -c "import mne_lsl; mne_lsl.sys_info()"
[load_liblsl:_load_liblsl_environment_variables:109] DEBUG: The environment variable 'MNE_LSL_LIB' is not set. (2024-10-31 14:42:42,750)
[load_liblsl:_load_liblsl_environment_variables:109] DEBUG: The environment variable 'PYLSL_LIB' is not set. (2024-10-31 14:42:42,750)
[load_liblsl:_load_liblsl_wheel_path:92] DEBUG: Found wheel path /home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/mne_lsl.libs/liblsl-65106c22.so.1.16.2 (2024-10-31 14:42:42,751)
Platform:                 Linux-6.8.0-47-generic-x86_64-with-glibc2.39
Python:                   3.12.3 (main, Sep 11 2024, 14:17:37) [GCC 13.2.0]
Executable:               /home/larsoner/python/virtualenvs/base/bin/python
CPU:                      x86_64
Physical cores:           4
Logical cores:            8
RAM:                      62.8 GB
SWAP:                     0.0 GB
mne_lsl:                  1.7.0.dev0
liblsl:                   1.16 (/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/mne_lsl.libs/liblsl-65106c22.so.1.16.2)
...

@mscheltienne
Copy link
Member

mscheltienne commented Nov 13, 2024

@larsoner Finally managed to run the liblsl tests, windows was annoying.. They add 30s-1'30 to the build and then 20-30s to run the executables.

0971b29 runs it also for the emulated aarch64 architecture, which doubles the build duration and anyway a lot of the liblsl tests are failing which I would assume is due to the emulation spawning outlets too slowly and then timing out during discovery 🤞
https://github.com/mne-tools/mne-lsl/actions/runs/11819726527/job/32930347365

In the end, I configured the liblsl tests to run on commits which include [test-liblsl]; but at least now we have (1) a way to run them and (2) we know that they pass on macOS-arm64 which I really wanted to confirm ;)

Also updated the documentation, added configuration variable to disable the build, added upload of the artifacts as release assets.. and tackled the last bullet points listed in this PR. Happy to merge :)

@larsoner
Copy link
Member Author

windows was annoying

Always is! 😆

In the end, I configured the liblsl tests to run on commits which include [test-liblsl]; but at least now we have (1) a way to run them and (2) we know that they pass on macOS-arm64 which I really wanted to confirm ;)

Like the arm64 builds I'd also run on any non-PR run (workflow dispatch, push, etc.). If we only ever run it on pushes with that commit message I worry they'll never run. WDYT?

I wouldn't bother running them on the emulated architecture, I'm not surprised they fail. At some point I think GH plans to have native aarch64, we can test more completely then I think.

Comment on lines +21 to +23
class BinaryDistribution(Distribution): # noqa: D101
def has_ext_modules(self): # noqa: D102
return True
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much less of a hack than what I was doing 🙂

Comment on lines 206 to 228
# TODO: Check if the minimum version for MNE-LSL requires those try/except.
try:
lib.lsl_pull_chunk_f.restype = c_long
lib.lsl_pull_chunk_d.restype = c_long
lib.lsl_pull_chunk_l.restype = c_long
lib.lsl_pull_chunk_i.restype = c_long
lib.lsl_pull_chunk_s.restype = c_long
lib.lsl_pull_chunk_c.restype = c_long
lib.lsl_pull_chunk_str.restype = c_long
lib.lsl_pull_chunk_buf.restype = c_long
except Exception: # pragma: no cover
logger.info(
"[LIBLSL] Chunk transfer functions not available in your LIBLSL version."
)
try:
lib.lsl_create_continuous_resolver.restype = c_void_p
lib.lsl_create_continuous_resolver_bypred.restype = c_void_p
lib.lsl_create_continuous_resolver_byprop.restype = c_void_p
except Exception: # pragma: no cover
logger.info(
"[LIBLSL] Continuous resolver functions not available in your LIBLSL "
"version."
)
Copy link
Member Author

@larsoner larsoner Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we bundle and conda-forge is probably sufficiently up to date, maybe we don't need these EDIT: inside a try/except?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone could provide an old liblsl in the environment variable; so even if it's unlikely, it doesn't hurt to have this try/except caught this edge case.

@mscheltienne
Copy link
Member

Like the arm64 builds I'd also run on any non-PR run (workflow dispatch, push, etc.). If we only ever run it on pushes with that commit message I worry they'll never run. WDYT?

Agree and done!

@mscheltienne mscheltienne merged commit 582b81a into mne-tools:main Nov 14, 2024
27 checks passed
@mscheltienne mscheltienne added this to the 1.7 milestone Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants