Skip to content

check if python symbols are present in the current process#279

Merged
kevinushey merged 3 commits intorstudio:masterfrom
randy3k:embed
Mar 29, 2021
Merged

check if python symbols are present in the current process#279
kevinushey merged 3 commits intorstudio:masterfrom
randy3k:embed

Conversation

@randy3k
Copy link
Contributor

@randy3k randy3k commented May 27, 2018

When R is embedded in a Python environment, it is possible that the python symbols get loaded into the main process. This PR first checks if the current process contains the python symbols. If found, it uses the python symbols found instead of the given libPath.

It would be more robust to check if the library found in the main process is consistent with libPath, and throw an error if they do not match. But it will require some work and I currently don't have an elegant way to implement the check.

related to #98

loadSymbol(pLib, "Py_IsInitialized", (void**) &Py_IsInitialized, pError);
::dlclose(pLib);

if (Py_IsInitialized != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to do this for Windows as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might use this from PyCall of Julia. I don't have a proper windows machine to test it though.

This PR is needed for me to use the latest conda python (an older version of conda python doesn't load symbols into the main process). I am not sure if python symbols will be loaded into the main process in Windows as well.

@jjallaire
Copy link
Member

This looks good to me at first reading. I am going to wait to merge it until after the next CRAN release (which we are just now locking down for)

@randy3k
Copy link
Contributor Author

randy3k commented May 28, 2018

I have updated the code. Now it checks the python symbols in py_discover_config. It returns the following in radian for instance.

r$> reticulate::py_discover_config()
python:         /Users/Randy/miniconda3/bin/python
libpython:      /Users/Randy/miniconda3/bin/python
pythonhome:     /Users/Randy/miniconda3:/Users/Randy/miniconda3
version:        3.6.5 |Anaconda, Inc.| (default, Mar 29 2018, 13:14:23)  [GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)]
numpy:           [NOT FOUND]

NOTE: Python version was forced by main process

Please notice that the path to libpython is the same as python because the symbols can be found in the main PIE executable.

update: not working in python 2, will fix soon.
works in python2 now.

r$> reticulate:::py_discover_config()
python:         /Users/Randy/miniconda3/envs/py27/bin/python
libpython:      /Users/Randy/miniconda3/envs/py27/lib/libpython2.7.dylib
pythonhome:     /Users/Randy/miniconda3/envs/py27:/Users/Randy/miniconda3/envs/py27
version:        2.7.15 |Anaconda, Inc.| (default, May  1 2018, 18:37:05)  [GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)]
numpy:           [NOT FOUND]

NOTE: Python version was forced by main process

@randy3k randy3k force-pushed the embed branch 4 times, most recently from bac40c6 to e3aab6e Compare May 28, 2018 22:55
@jjallaire
Copy link
Member

@randy3k Thanks for all of this work. I am going to wait to merge until after the next CRAN release.

@randy3k
Copy link
Contributor Author

randy3k commented Jul 31, 2018

bump

@randy3k
Copy link
Contributor Author

randy3k commented Aug 12, 2018

I can see that a new release was made. Maybe it's the time to review this PR?

@jjallaire
Copy link
Member

Yes, apologize for not reviewing as part of v1.10 but there is a lot of code to review and it touches some sensitive parts of initialization so there is definitely a risk of breakage. I will try to review for the v1.11 release (which should be in another ~ 30 days).

@adamroyjones
Copy link

(Bumping as it's still an issue that is tripping people up.)

when the libpython is the same as the executable (i.e. PIE),
the only way to find the correct symbols is `dlopen(NULL,..)`

Consider

```py
>>> import ctypes
>>> libpython = ctypes.PyDLL("/root/miniconda/lib/libpython3.6m.so")
>>> libpython.Py_IsInitialized()
0
>>> libpython = ctypes.PyDLL("/root/miniconda/bin/python")
>>> libpython.Py_IsInitialized()
0
>>> libpython = ctypes.CDLL(None)
>>> libpython.Py_IsInitialized()
1
>>>
```

# Conflicts:
#	R/package.R
@randy3k
Copy link
Contributor Author

randy3k commented Mar 8, 2019

This is no longer needed for radian as we are patching reticulate directly randy3k/radian@88e0943.

@kevinushey kevinushey merged commit b1d7e71 into rstudio:master Mar 29, 2021
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.

4 participants