-
Notifications
You must be signed in to change notification settings - Fork 760
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
Discover and prefer the parent interpreter when invoked with python -m uv
#3736
Conversation
python -m uv ...
python -m uv
@@ -1053,22 +1063,16 @@ impl SourceSelector { | |||
InterpreterSource::SearchPath, | |||
InterpreterSource::PyLauncher, | |||
InterpreterSource::ManagedToolchain, | |||
InterpreterSource::ParentInterpreter, |
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.
This is in the SystemPython::Required
clause. Maybe I should move it out? This may be a bit weird if you do ./.venv/bin/python -m uv pip install --system
as we'll still find this Python... but also a bit weird if you do /sys/python -m uv --system
and we find a different Python?
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.
I think I can fix this with some changes to discovery
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.
Is the ideal behavior that:
./.venv/bin/python -m uv pip install --system
does not find.venv/bin/python
/sys/python -m uv --system
does find/sys/python
?
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.
I think so yea. With #3739 this will be possible
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.
(rebasing this onto that now)
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.
The behavior should be as described now, I'll do some QA.
python/uv/__main__.py
Outdated
@@ -31,6 +31,9 @@ def _run() -> None: | |||
if venv: | |||
env.setdefault("VIRTUAL_ENV", venv) | |||
|
|||
# Let `uv` know that it was spawned by this Python interpreter | |||
env["UV__PARENT_INTERPRETER"] = sys.executable |
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.
Can we use something other than the double underscore, like a name that is so long that no one likes to use it outside that script?
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.
I feel like the double underscore is sufficient for saying it's not user facing and we're not going to document or support it elsewhere. Idk about making the name arbitrarily longer.... I guess I could do UV_PRIVATE__...
?
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.
Added more suffix
Previously, we enforced `SystemPython` outside of the interpreter discovery exclusively with source selection. Now, we perform additional filtering of interpreters depending on if they are a virtual environment. This should not change any existing behavior, but will make it much easier to have consistent behavior in ambiguous cases like #3736 (comment) where a source could provide either a system interpreter or virtual environment interpreter.
/// Only allow a system Python if passed directly i.e. via [`InterpreterSource::ProvidedPath`] or [`InterpreterSource::ParentInterpreter`] | ||
#[default] | ||
Explicit, | ||
/// Do not allow a system Python | ||
Disallowed, |
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.
I decided to add an Explicit
variant to avoid confusing internal behavior where SystemPython::Disallowed
could still get a system Python from this environment variable.
# Conflicts: # crates/uv-interpreter/src/discovery.rs
e.g.
|
Closes #2222
Closes #2058
Replaces #2338
See also #2649
We use an environment variable (
UV_INTERNAL__PARENT_INTERPRETER
) to track the invoking interpreter whenpython -m uv
is used. The parent interpreter is preferred over all other sources (though it will be skipped if it does not meet a--python
request or if--system
is used and it belongs to a virtual environment). We warn if--system
is not provided and this interpreter would mutate system packages, but allow it.