-
Notifications
You must be signed in to change notification settings - Fork 670
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
pick up site packages from the interpreter's sys.path
#2636
Conversation
fixing CI and probably will add tests (it's turning out to be kinda non-trivial hmm, still looking though), should be done sometime today or tomorrow. |
@@ -520,6 +520,7 @@ def main() -> None: | |||
"prefix": sys.prefix, | |||
"base_executable": getattr(sys, "_base_executable", None), | |||
"sys_executable": sys.executable, | |||
"sys_path": sys.path, |
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 it desirable for cwd to be in here? (if not, it should be removed, accounting for sys.flags.safe_path)
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.
probably will leave this value as is so that it still equals to sys.path
(it may be confusing if it's called sys_path
but the value returned isn't sys.path
) - but added filtering via suffix site/dist packages where it's used.
taking longer than expected :.( - down to a couple of failing tests (having trouble reproducing it locally atm) + system setup jobs failing for windows, will continue to look. |
@zanieb do you happen to know why also on CI machines i've noticed if i run it multiple times, it only fails the first time and succeeds in all subsequent runs... 🤔 |
😆 ok after rebasing this time it seems like the only failures are windows; i'll try to get these resolved this week. |
What kind of problems are you having with the test suite? Have you merged main lately? We've recently improved the test snapshot filtering (i.e. #2678) |
the most cryptic test started succeeding after rebasing a couple of times this week 🥲 i think i should be good once i figure out this windows issue (seemed like it can be resolved with something similar to thank you! |
@@ -656,7 +657,7 @@ jobs: | |||
yum install tar gzip which -y | |||
- uses: actions/checkout@v4 | |||
- name: "Install Python" | |||
run: yum install python3 python3-pip -y | |||
run: yum install python3 -y && python3 -m ensurepip |
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.
pip
installed via yum install python3-pip
has RECORD
missing, which causes issues described in pypa/pip#11631 (this issue can be reproduced pulling the docker image used and running the command here)
closing since this PR is too stale at this point |
Summary
Resolves #2500
This change got way bigger and more complex than initially thought 😭 (happy to break up into multiple smaller PRs if preferred!) - the main changes are as follows:
sys.path
from the chosen interpreter to be able to pick up more import paths beyond purelib / platlibinstall
fromuv-dispatch
to take an additional argumentreinstall
so that venv creation with--seed
can always force reinstall (this seemed the least amount of change required to not break the behavior of--seed
). This was necessary sinceuv
will thinkpip
already exists with this change (in the base interpreter) and would skip installing it for the venv.uv pip install
with reinstall option seems to pick up site package paths multiple timesprobably it should be done once for the lifetime of a single command invocation? still looking into it.
Test Plan
where