-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: clean up runtime code for python setup #26042
Conversation
3911d3d
to
a4be1b8
Compare
143a880
to
87ae90e
Compare
@jacksonrnewhouse - rebased on main and retested on linux/amd64. This is ready to review for the approach. If you're generally happy with it, I'll do full testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@jacksonrnewhouse - thanks for the review! In doing further testing and looking at #26050, I need followup commits (which may end up fixing #26050 in the process). UPDATE: I'm going to mark this as DRAFT just to be sure that it doesn't get committed before I have those ready. |
Since you approved, I'll take that to mean that you like the approach. When I have the follow-up commits ready, I'll do full testing and list it in this issue, then undraft. |
ccd5d22 introduced working but temporary code for setting up the python runtime environment. This cleans that up: * refactor various find_python() functionality into virtualenv.rs * refactor PYTHONHOME calculation to virtualenv.rs:find_python_install() * adjust init_pyo3() to temporarily set PYTHONHOME based on virtualenv.rs:find_python_install() as this is the only place it is needed (indeed, venv activation scripts try to remove it) Importantly, virtualenv.rs:find_python_install() tries to find the python build standalone runtime based on a few heuristics. This function could be improved in the fullness of time to, eg, be configured via a build parameter. Also, virtualenv.rs:find_python() can be used pre and post venv activation. Before entering the venv, it will use find_python_install() which is useful for things like setting up the initial venv. After entering the venv, it will honor VIRTUAL_ENV (as set by virtualenv.rs:initialize_venv()) to find python, which is important for install packages with pip and having them installed into the venv.
f86bf2f
to
f57def5
Compare
c57fe65
to
f474906
Compare
@jacksonrnewhouse - ok, when doing full testing I noticed a couple of things with sys.prefix and sys.path that I thought I would fix in this PR (ie, address some small issues in the previous setup). |
When activating a venv in the shell, sys.base_prefix and sys.base_exec_prefix should be set to the installation location while sys.prefix and sys.exec_prefix should be set to the venv dir. Unfortunately, when initialize_venv() and init_pyo3() are called, we can't use Py_InitializeFromConfig() to set any of these and certain platforms are unable to find python-build-standalone. For now, we'll temporarily set PYTHONHOME in init_pyo3() to the installation location to make python work. By setting it at this point in the code, sys.prefix and sys.exec_prefix end up also being set to the installation location, which is fine when not under a venv, but is different from when entering an venv. To address this, in PYTHON_INIT.call_once() and when VIRTUAL_ENV is set (which initialize_venv() will have set at this point), manually set sys.prefix and sys.exec_prefix to what is in VIRTUAL_ENV. Similarly, when activating a venv in the shell, sys.path is appended to have the venv's site-packages dir. Previously we were setting PYTHONPATH in initialize_venv() which ensures that the venv's site-packages dir is in sys.path, but this ends up having the venv's site-packages dir first in sys.path. To correct this, don't set PYTHONPATH any more and instead adjust PYTHON_INIT.call_once() to append the venv's site-packages dir to sys.path when VIRTUAL_ENV is set. Finally, when exiting init_pyo3(), unconditionally unset PYTHONHOME when VIRTUAL_ENV is set (like activation scripts do) and restore/unset when it isn't. Prior to these changes, all target incorrectly had the venv's site-packages first in sys.path and OSX and Windows additionally had an incorrect sys.prefix and sys.exec_prefix. With these initialization changes in place, the runtime environment for the plugins is much closer to that of a shell activated venv.
f474906
to
1f8001d
Compare
ccd5d22 introduced working but temporary code for setting up the python runtime environment. This cleans that up:
chore: clean up runtime code for python setup
ccd5d22 introduced working but temporary code for setting up the
python runtime environment. This cleans that up:
virtualenv.rs:find_python_install() as this is the only place it is
needed (indeed, venv activation scripts try to remove it)
Importantly, virtualenv.rs:find_python_install() tries to find the
python build standalone runtime based on a few heuristics. This function
could be improved in the fullness of time to, eg, be configured via a
build parameter.
Also, virtualenv.rs:find_python() can be used pre and post venv
activation. Before entering the venv, it will use find_python_install()
which is useful for things like setting up the initial venv. After
entering the venv, it will honor VIRTUAL_ENV (as set by
virtualenv.rs:initialize_venv()) to find python, which is important for
install packages with pip and having them installed into the venv.
chore: update README_processing_engine.md for default venv
chore: add bug reference for venv migrations with python minor releases
chore: update README_processing_engine.md for default venv
chore: add bug reference for venv migrations with python minor releases
chore: add security URLs to README_processing_engine.md
chore: find_python_home() returns Option. Thanks Jackson Newhouse
fix: manually set sys.prefix, exec_prefix and sys.path
When activating a venv in the shell, sys.base_prefix and
sys.base_exec_prefix should be set to the installation location while
sys.prefix and sys.exec_prefix should be set to the venv dir.
Unfortunately, when initialize_venv() and init_pyo3() are called, we
can't use Py_InitializeFromConfig() to set any of these and certain
platforms are unable to find python-build-standalone. For now, we'll
temporarily set PYTHONHOME in init_pyo3() to the installation location
to make python work. By setting it at this point in the code, sys.prefix
and sys.exec_prefix end up also being set to the installation location,
which is fine when not under a venv, but is different from when entering
an venv.
To address this, in PYTHON_INIT.call_once() and when VIRTUAL_ENV is set
(which initialize_venv() will have set at this point), manually set
sys.prefix and sys.exec_prefix to what is in VIRTUAL_ENV.
Similarly, when activating a venv in the shell, sys.path is appended to
have the venv's site-packages dir. Previously we were setting PYTHONPATH
in initialize_venv() which ensures that the venv's site-packages dir is
in sys.path, but this ends up having the venv's site-packages dir first
in sys.path. To correct this, don't set PYTHONPATH any more and instead
adjust PYTHON_INIT.call_once() to append the venv's site-packages dir to
sys.path when VIRTUAL_ENV is set.
Finally, when exiting init_pyo3(), unconditionally unset PYTHONHOME when
VIRTUAL_ENV is set (like activation scripts do) and restore/unset when
it isn't.
Prior to these changes, all target incorrectly had the venv's
site-packages first in sys.path and OSX and Windows additionally had an
incorrect sys.prefix and sys.exec_prefix. With these initialization
changes in place, the runtime environment for the plugins is much closer
to that of a shell activated venv.
Importantly, virtualenv.rs:find_python_install() tries to find the python build standalone runtime based on a few heuristics. This function could be improved in the fullness of time to, eg, be configured via a build parameter.
Also, virtualenv.rs:find_python() can be used pre and post venv activation. Before entering the venv, it will use find_python_install() which is useful for things like setting up the initial venv. After entering the venv, it will honor VIRTUAL_ENV (as set by virtualenv.rs:initialize_venv()) to find python, which is important for install packages with pip and having them installed into the venv.
Testing
This has been tested to work correctly for all of:
The paths are correctly setup when
.venv
doesn't exist, when it does and when--virtual-env-location
is set (to an existing or non-existing dir).Furthermore, I did testing before and after
fix: manually set sys.prefix, exec_prefix and sys.path
1f8001d0.Shell activated venv
It is useful to compare what a shell activated venv looks like to know how we compare:
On all OSes:
base_prefix
andbase_exec_prefix
point to thepython-build-standalone
runtime dirprefix
andexec_prefix
point to the venv dirpath
has the venv's site-packages at the end of the listCurrent main
All targets for current main incorrectly have the venv's site-packages first in sys.path and OSX and Windows additionally had an incorrect sys.prefix and sys.exec_prefix. This doesn't seem to affect the operations of plugins.
Linux
OSX
Windows
With this PR
While the current main behavior doesn't seem to be causing problems, it seems like there could be potential for them. With this PR, the runtime environment for the plugins is much closer to that of a shell activated venv.
Linux
OSX
Windows
Closes #26012