weiqingy commented on code in PR #979:
URL: https://github.com/apache/flink-agents/pull/979#discussion_r3741676168
##########
tools/build.sh:
##########
@@ -101,7 +101,7 @@ if $build_python; then
# build python
cd python
rm -rf dist/ # Clean old build artifacts before building
- pip install uv==0.11.0
+ python3 -m pip install uv==0.11.0
Review Comment:
You're right, this only fixed the install half. I hit the same thing locally
without needing `PYTHONUSERBASE`: `command -v uv` finds nothing on my machine
while `python3 -m uv` works, so the patch cleared line 104 and then died on 105.
Now covered: the five `uv` calls in `build.sh`, the two `ruff` calls in the
`lint.sh` fallback, and the three `pytest` calls in the `ut.sh` fallback. I ran
`lint.sh` with PATH arranged so `command -v uv` fails, so that branch is
actually exercised rather than just eyeballed.
There are two places I left bare, and I'd like your read on whether the line
I drew is the right one.
The rule I landed on is to use the interpreter form only where the same
script pip installed that tool a few lines earlier, since that's what
guarantees the module is importable. The `command -v uv` branches don't
qualify: a standalone uv (curl installer, homebrew) gives you a binary and no
module, so `python3 -m uv` would break those setups.
`tools/e2e.sh` doesn't qualify either, though I had it converted at first.
It never installs uv, it borrows one from `build.sh`, and it skips that build
whenever `e2e-test/target` and `uv.lock` already exist. So with a standalone uv
on PATH, converting it turns a working run into a failing one. I backed it out
and filed #982 for a real guard. Does that split seem right to you, or would
you rather see e2e.sh handled here?
One thing your comment led me to that I'd otherwise have shipped: `python3
-m uv` isn't a transparent alias. Its `__main__` injects `VIRTUAL_ENV` from the
calling interpreter, and `uv pip` honors it even though `lock`, `sync` and
`run` ignore it. Same directory, both exit 0, no warning:
```
uv pip install <pkg> -> ./.venv
python3 -m uv pip install <pkg> -> the interpreter's own venv
```
At `build.sh:109` that would put the freshly built wheel outside
`python/.venv`, and `:111` then deletes `flink_agents/lib`, so the venv ends up
pointing at a tree with no jars. That line now names its target with `--python
.venv`, which is a no-op wherever the bare form already found `.venv`.
I skipped the PATH option mainly because a `--user` fallback puts the
scripts somewhere `sysconfig.get_path("scripts")` doesn't point, so the probe
needs to try both schemes. Happy to go that way instead if you think it reads
better.
Retitled the PR and rewrote the description, since it's no longer just about
pip.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
