Skip to content

Add support for Linux and global python installations #7

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ncguilbeault
Copy link
Contributor

This PR adds 2 new features, namely support for Python scripting on Linux and support for global Python installations.

Support for Linux OS

Previously, the way the RuntimeManager and EnvironmentHelper configured the path variables and python engine were hard-coded and specific to the Windows OS. I have added several lines of code in both of these which will 1) determine the current OS; and 2) configure path and python engine based on the OS detected.

Support for global python installations

In the previous version, if no virtual environment was detected, then creation of the python engine would fail due to incorrect loading of the python DLL. With the newly added feature, the EnvironmentHelper will search for a global python installation on the system path by performing a directory search through the directories in the system's path. It will then initialize the Python engine using the first available python DLL, which should pick up the global python installation environment.

@ncguilbeault ncguilbeault changed the title Updated env manager for added linux support Updated EnvironmentHelper for added Linux support and loading of global python installations Jan 22, 2024
@glopesdev glopesdev self-requested a review February 2, 2024 14:56
@glopesdev glopesdev changed the title Updated EnvironmentHelper for added Linux support and loading of global python installations Add support for Linux and global python installations Feb 14, 2024
@glopesdev glopesdev added the feature New planned feature label Feb 14, 2024
Comment on lines 81 to 82
path = EnvironmentHelper.GetVirtualEnvironmentPath(path);
EnvironmentHelper.SetVirtualEnvironmentPath(path);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like both of these functions change environment variables before initializing the python runtime. Is it necessary to set VIRTUAL_ENV to correctly configure the environment on Linux? I thought this was only necessary to support environment activation when no specific path was provided.


namespace Bonsai.Scripting.Python
{
static class EnvironmentHelper
{
public static string GetPythonDLL(string path)

private static string GetFirstFile(string path, string pattern, HashSet<string> visitedDirectories = null)
Copy link
Member

@glopesdev glopesdev Feb 17, 2024

Choose a reason for hiding this comment

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

This turns out to be a gigantic rabbit hole, but this solution unfortunately cannot guarantee that the correct python shared libraries will be found. The problem lies with Multiarch handling of shared library paths.

Multiarch is the linux standard to allow side-by-side installation of 32-bit and 64-bit shared libraries in 64-bit systems which can run both types of applications. To do this, shared libraries with multiarch-support may be installed simultaneously to x86_64-linux or i386-linux folders inside usr/lib for example.

Because of folder sort order, systems with both python 64-bit and 32-bit installed may pick up the i386-linux folder accidentally simply because it shows up first in the hierarchical search.

I found this issue while looking for alternative ways to automatically discover libpython3*.so using only environment or system variables. Unfortunately I have no viable solution yet, but for a number of reasons related to performance, reliability, and security I feel we should avoid doing an arbitrary recursive search on system and user folders.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, it might be possible to bypass this altogether, since the following works:

Runtime.PythonDLL = "libpython3.10.so.1";

From Python.NET readme:

You must set Runtime.PythonDLL property (...) Typical values are python38.dll (Windows), libpython3.8.dylib (Mac), libpython3.8.so (most other Unix-like operating systems).

This seems to work cross-platform without any extra work to find the shared library assuming that there is a system-wide python install or standard local path configuration.

Of course the issue remains of how to find the exact shared library name.

Copy link
Member

@glopesdev glopesdev Feb 17, 2024

Choose a reason for hiding this comment

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

Another clue might be a symbolic link to the actual shared library stored in what seems a possibly predictable folder path: /usr/lib/python3.10/config-3.10-x86_64-linux-gnu/libpython3.10.so.

Adding the folder where this symlink is located to LD_LIBRARY_PATH seems to allow the following to work, which is a much more typical shared library file name:

Runtime.PythonDLL = "libpython3.10.so";

Copy link
Member

Choose a reason for hiding this comment

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

@ncguilbeault I think I got it, this last issue is resolved simply by installing libpython-dev with the corresponding python version. With this I was able to get the kernel running with almost no code changes, I will submit an alternative PR for us to review soon.

@ncguilbeault
Copy link
Contributor Author

ncguilbeault commented Feb 26, 2024

@glopesdev Here are the workflows I was using to test the speed of the interpreter lock feature.
python-interpreter-lock-test.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New planned feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants