labstreaminglayer / pylsl

Python bindings (pylsl) for liblsl
MIT License
136 stars 57 forks source link

Search for liblsl only looks for the first path returned by `find_liblsl_libraries()` #50

Closed mscheltienne closed 2 years ago

mscheltienne commented 2 years ago

I might be mistaken, but isn't the code loading the library only looking at the first path returned by find_liblsl_libraries()?

https://github.com/labstreaminglayer/liblsl-Python/blob/88a4d35a0d7b34c9ac4bffc6db9033555152b412/pylsl/pylsl.py#L1277-L1293

cboulay commented 2 years ago

That's correct. It returns the first found library file. If PYLSL_LIB environment variable is set and the file exists then it returns that path. Then it searches within the pylsl package and if it doesn't find the lib there then it searches at the system level. The latter only works on non-Windows because Windows doesn't have a standard system-level location to put shared libraries (I don't consider C:\Windows\System32 to be a good place to put 3rd party libraries).

mscheltienne commented 2 years ago

@cboulay The general idea is clear to me and I agree with it and its implication for windows. But, the code runs only once here calling next() on the generator find_liblsl_libraries(), effectively trying only the first path yielded by find_liblsl_libraries() and not the following ones.

Shouldn't it be something like:

libpaths = find_liblsl_libraries()
while True:
    try:
        libpath = next(libpaths)
        lib = CDLL(libpath)
    except StopIteration:
        # [...] err message
        # break by raising
    except Exception:
        continue
mscheltienne commented 2 years ago

And in the case of an OSError, log the specific error message as a warning and continue. Or raise it only if one of the following iteration did not manage to load liblsl, in e.g. a finally statement.

mscheltienne commented 2 years ago

Also, imagine now that you have both the 32 bits and the 64 bits liblsl in the correct path on your Windows 64 bits machine (for some obscure reason). Now, if the 32 bits liblsl is yielded first, the OSError raises first and you get stuck even if you happen to also have the correct version of liblsl on your machine.

cboulay commented 2 years ago

I don't see a reason to return more than one path. If multiple libraries are being found and the first one is not the correct one then this is a problematic configuration. It's a problem that can be overcome pretty easily by using the environment variable, but other than that it's not a configuration we wish to support.

Part of the reason for that is the cause (multiple lsl.dll's on the search path) can lead to other errors in other applications that will be harder to catch. For example, I think there's a tool whose installer puts liblsl64.dll (old naming convention) in C:\Windows\System32. This is a bad idea. We don't want this to happen, and we don't want LSL users to accidentally use this ancient dll due to some other problem. I'd rather raise the error now and fix the problem.

Other notes:

Using while True: except Exception: continue seems like it can enter an infinite loop pretty easily.

If you have multiple dlls in the same folder then they should have a suffix to distinguish 32 and 64 bit (they can't have the same name!), and the correct one will be chosen by bitness:str(8 * struct.calcsize("P")). If they are in different folders and it finds the incorrect one first, then this is a configuration that we do not support.

In any case, this error shouldn't really be a thing to be concerned about because the windows packages on pypi ship with the correct dll and this will be found first because it's inside the package. I'm not sure why you weren't getting the dll when you were using pip install. If you can narrow that down then that would be helpful. Again, if pylsl found any old dll then it might hide this problem and we don't want that.

It might be overly opinionated of us, but we don't want to support system-level DLLs in Windows.

mscheltienne commented 2 years ago

Not supporting system-level DLLs in Windows is fine. The while True: except Exception: continue would obviously need some additional safeties, it was just for a quick demo.

Trying to avoid using outdated DLLs is also fine, but I suspect this can be done using a version number (library_version() maybe?). It could have different behaviors depending on how old and deprecated the DLL found is (info, warning, raise).

For the reason why I did not get the .dll with pip install on several windows installs in the past months, I can have a look tomorrow at the version, I still have a windows laptop at work with the path configured as an env variable because the DLL was missing.


But this does not address all the problems.


Finally, the problem I'm describing is worst on Linux and macOS where the distribution model does not ship liblsl with pylsl. The fact that 1. it does not ship with pylsl, and 2. the search for a valid and compatible liblsl file is limited makes the experience for the user extremely painful.

To give you an example, for a BrainHack a couple of months ago, a group spent the 4 first hours figuring out how to get import pylsl working on 3 different computers. Personally, I prefer the approach of other packages: if you install it from one of the official channels (pip, conda), it should just work. Regardless of the dependencies, regardless of the dependencies versions, even if you have to distribute alongside your package a specific version of a dependency.

One last comment for macOS users: many users are not homebrew users (even if it is great). Usually, those users are not tech-savvy, and getting pylsl working becomes even harder.

cboulay commented 2 years ago

I have a python application where I ship a given version of pylsl and liblsl with the package, with the liblsl in the pylsl/lib folder. Thus on the CIs, I had all the DLLs (for all 3 OS) in the same folder. The windows CI was failing because the 32 bits DLL was getting loaded first, despite the fact that the correct DLL was also in the same folder.

If both dll's have the 32 or 64 suffix then this should work. It should pick the correct one. But if one of them does not have a suffix then it will pick that one first, even if it is the incorrect one.

is worst on Linux [...] The fact that 1. it does not ship with pylsl, and 2. the search for a valid and compatible liblsl file is limited makes the experience for the user extremely painful.

We spent a long time trying to build Linux binaries for pylsl. See, for example, here: https://pypi.org/project/pylsl/1.14.1b5/#files This is very difficult to do. The requirements are very strict, and when we thought we had it all worked out, we had users reporting that the provided .so file was incompatible with their particular flavour of Linux. When this happens it is very hard to diagnose. Sure it worked with Ubuntu 18.04 and maybe a couple other Linux distros, but it actively created problems for other users. It is a lot of work to maintain and we decided it was not worth it.

There are definitely packages on pip that when you install them on Linux and try to run them, they will fail because you don't have a required system library installed. We aren't unique in this.

conda is probably a better example where it should "just work".

We do have liblsl on conda: https://anaconda.org/conda-forge/liblsl And @tstenner has pylsl but it is older: https://anaconda.org/tstenner/pylsl It would be good to get an updated version of pylsl onto conda-forge and to make sure it has liblsl as a dependency.

I'd love to have bulletproof installers for all platforms, with properly signed and certified binaries, but this is a lot of work to maintain and we are a small dev team volunteering our time on an open source project. If you want to contribute to the distribution and are willing to maintain it for years to come then we would love to have you join us.

mscheltienne commented 2 years ago

For the windows pip install pylsl that does not package the .dll, it occurred for me on Python 3.9.9 with pylsl 1.15.0. It occurred also on different versions of python 3.9, on different computers, during the past months. (I did double check by removing the PYLSL_LIB env variable I've set up on this laptop).

For the OSError on my CI following the confusion between 32 and 64 bits .dll, that's a good fix for me. Thanks.


I do understand better the constraint for Linux, but correct me if I'm wrong, Linux users who do not use Ubuntu 18.04 or 20.04 have to build liblsl themselves anyway? Do you know which part of your user base they represent, compared to Ubuntu 18.04/20.04 users, macOS users and Windows users? I suspect the latter represents the majority of users for LSL, and for most applications. Since those 4 systems are the only ones for which liblsl is built and available on the release page, wouldn't it be better to at least have "just work" package for those systems? And for other distributions of Linux, well if you have to build the lib yourself anyway, distribute pylsl without lib as you do now.


Also coming back to the original proposition in the issue, what is the downside in having a more flexible search tool for liblsl that does not simply stop at the first one found but test all the possible liblsl found; and that checks the version? Wouldn't that make configuration easier for the user?

cboulay commented 2 years ago

And for other distributions of Linux, well if you have to build the lib yourself anyway, distribute pylsl without lib as you do now.

There's no way to separate different Linux distributions on pypi. It's manylinux or it's nothing. It is possible to build Python packages for specific Linux distros, but those can't be published on pypi.

And yes, people on non-Ubuntu 18.04 or 20.04 have to build liblsl themselves. However, if those users have correctly installed liblsl at the system level, and they pip install pylsl to get a manylinux version that ships with an incompatible liblsl.so, then this liblsl.so will be found first and will cause pylsl to crash. (If I recall correctly, it's not as simple as a module import error).

What is the downside in having a more flexible search tool for liblsl that does not simply stop at the first one found but test all the possible liblsl found; and that checks the version?

  1. Maintenance. It's not as easy as it sounds.
  2. Import speed.
  3. I don't want to wallpaper over poorly configured systems. I would rather users report the error and then we can help them fix the problem. In some cases, the pylsl import errors help reveal a different underlying problem.

To be honest, this is a bit of a strange request. I have never seen a Python library actively test multiple encountered DLLs until it finds one that works. In every other DLL-using Python library I've encountered, they usually assume that the DLL is on the path and that's the end of it. At best they read an env variable. I think we are a bit generous in this regard but I would be interested to see some examples where they did extensive DLL testing. This feels like a case of "give them an inch and they take a mile".

Maybe this is our mistake. Do you think that you would have resolved your errors faster if we did not do any such searching?

mscheltienne commented 2 years ago

Thank you very much for detailing this information. I am not yet used to packaging and distributing software. Probably not, a different lib finding logic would not have resolved the problems I encountered (except the CI one, but that one was quick to solve).

The main roadblock faced was encountered at BrainHack by one of our groups: pip install pylsl on windows with Python 3.9 did not distribute the binary library. I believe at the time, they tried on python 3.9.7; I did encounter the same issue with 3.9.9 more recently, however, I remember it was working as intended with python 3.8.10.

Also, when this problem was encountered, the documentation was not sufficient, In the readme, in the Prerequisites section, there is a link to the liblsl release page. Great. But, there is no information about what to do with the obtained file. It might be trivial for you and me, but I remember the group at BrainHack was completely lost. Specifically:

At BrainHack, the issue this group faced got resolved when I looked into the pylsl code directly and how it searched for the binary library. That's when I noticed the variable PYLSL_LIB, which I think is not documented anywhere.

cboulay commented 2 years ago

I just tried the following:

conda create -n py39 python=3.9
pip install pylsl

And that gave me python 3.9.7, and pylsl 1.15.0 including lsl.dll found in C:\Users\Chad\.conda\envs\py39\Lib\site-packages\pylsl\lib. I did the same for python=3.10 and got the lsl.dll in the correct folder.

I can't reproduce your problem.

mscheltienne commented 2 years ago

Damn.. I am not a user of conda bit I can't see how this would make a difference. Also, I add this issue with pip install pylsl on the base python environment and on virtual environments.

Anyway, thanks for all the explanations; I hope the issue we encountered when installing pylsl on Windows was a one-time-only problem.

cboulay commented 2 years ago

I thought I already sent this message, sorry if you get a dup.

I think on the Windows machines, you may have encountered an import error due to missing msvc redistributable. This information is supposed to be provided in the error message. https://github.com/labstreaminglayer/liblsl-Python/blob/master/pylsl/pylsl.py#L1288-L1293

If the error that is raised when a dll dependency is missing is no longer an OSError then this message won't be printed. It would be good to know what the new error type is in these cases.

mscheltienne commented 2 years ago

Let's test this theory: I took the windows laptop running python 3.9.9 and removed the environment variable PYLSL_LIB I've set up. The error raised by import pylsl is:

RuntimeError: LSL binary library file was not found. Please make sure that the binary file can be found in the package lib folder

 (C:\Users\mscheltienne\pyvenv\bsl\lib\site-packages\pylsl-1.15.0-py3.9.egg\pylsl\lib)

 or specify the PYLSL_LIB environment variable.
 You can install the LSL library with conda: `conda install -c conda-forge liblsl`

or otherwise download it from the liblsl releases page assets: https://github.com/sccn/liblsl/releases

On this PC, I have the Microsoft Visual C++ 2015-2022 Redistributable.

cboulay commented 2 years ago

And lsl.dll exists in that folder (C:\Users\mscheltienne\pyvenv\bsl\lib\site-packages\pylsl-1.15.0-py3.9.egg\pylsl\lib) ? If not, then that means that pip installed pylsl-1.15.0-py2.py3-none-any.whl instead of pylsl-1.15.0-py2.py3-none-win_amd64.whl.

A bit of info on platform tags. (You'll see that manylinux is a complicated beast).

Run pip debug --verbose to see if py3-none-win_amd64 is in your list.

mscheltienne commented 2 years ago

Obviously lsl.dll is missing, the folder pylsl/lib does not exist. When I use the environment variable, I point towards a different location on my system where I downloaded liblsl.

Yes it is in the list. Here is the full output:

pip version: pip 21.3.1 from C:\Users\mscheltienne\pyvenv\bsl\lib\site-packages\pip (python 3.9)                                               
sys.version: 3.9.9 (tags/v3.9.9:ccb0e6a, Nov 15 2021, 18:08:50) [MSC v.1929 64 bit (AMD64)]                                                    
sys.executable: C:\Users\mscheltienne\pyvenv\bsl\Scripts\python.exe                                                                            
sys.getdefaultencoding: utf-8                                                                                                                  
sys.getfilesystemencoding: utf-8                                                                                                               
locale.getpreferredencoding: cp1252                                                                                                            
sys.platform: win32                                                                                                                            
sys.implementation:                                                                                                                            
  name: cpython                                                                                                                                
'cert' config value: Not specified                                                                                                             
REQUESTS_CA_BUNDLE: None                                                                                                                       
CURL_CA_BUNDLE: None                                                                                                                           
pip._vendor.certifi.where(): C:\Users\mscheltienne\pyvenv\bsl\lib\site-packages\pip\_vendor\certifi\cacert.pem                                 
pip._vendor.DEBUNDLED: False                                                                                                                   
vendored library versions:                                                                                                                     
  CacheControl==0.12.6                                                                                                                         
  colorama==0.4.4                                                                                                                              
  distlib==0.3.3                                                                                                                               
  distro==1.6.0                                                                                                                                
  html5lib==1.1                                                                                                                                
  msgpack==1.0.2 (Unable to locate actual module version, using vendor.txt specified version)                                                  
  packaging==21.0                                                                                                                              
  pep517==0.12.0                                                                                                                               
  platformdirs==2.4.0                                                                                                                          
  progress==1.6                                                                                                                                
  pyparsing==2.4.7                                                                                                                             
  requests==2.26.0                                                                                                                             
  certifi==2021.05.30                                                                                                                          
  chardet==4.0.0                                                                                                                               
  idna==3.2                                                                                                                                    
  urllib3==1.26.7                                                                                                                              
  resolvelib==0.8.0                                                                                                                            
  setuptools==44.0.0 (Unable to locate actual module version, using vendor.txt specified version)                                              
  six==1.16.0                                                                                                                                  
  tenacity==8.0.1 (Unable to locate actual module version, using vendor.txt specified version)                                                 
  tomli==1.0.3                                                                                                                                 
  webencodings==0.5.1 (Unable to locate actual module version, using vendor.txt specified version)                                             
Compatible tags: 33                                                                                                                            
  cp39-cp39-win_amd64                                                                                                                          
  cp39-abi3-win_amd64                                                                                                                          
  cp39-none-win_amd64                                                                                                                          
  cp38-abi3-win_amd64                                                                                                                          
  cp37-abi3-win_amd64                                                                                                                          
  cp36-abi3-win_amd64                                                                                                                          
  cp35-abi3-win_amd64                                                                                                                          
  cp34-abi3-win_amd64                                                                                                                          
  cp33-abi3-win_amd64                                                                                                                          
  cp32-abi3-win_amd64                                                                                                                          
  py39-none-win_amd64                                                                                                                          
  py3-none-win_amd64                                                                                                                           
  py38-none-win_amd64                                                                                                                          
  py37-none-win_amd64                                                                                                                          
  py36-none-win_amd64                                                                                                                          
  py35-none-win_amd64                                                                                                                          
  py34-none-win_amd64                                                                                                                          
  py33-none-win_amd64                                                                                                                          
  py32-none-win_amd64                                                                                                                          
  py31-none-win_amd64                                                                                                                          
  py30-none-win_amd64                                                                                                                          
  cp39-none-any                                                                                                                                
  py39-none-any                                                                                                                                
  py3-none-any                                                                                                                                 
  py38-none-any                                                                                                                                
  py37-none-any                                                                                                                                
  py36-none-any                                                                                                                                
  py35-none-any                                                                                                                                
  py34-none-any                                                                                                                                
  py33-none-any                                                                                                                                
  py32-none-any                                                                                                                                
  py31-none-any                                                                                                                                
  py30-none-any                                                                                              

This line is bothering me: sys.platform: win32. Is this normal?