fix(benchmarks): keep the server interpreter symlink intact in both LIBERO schedulers - #35
Merged
Merged
Conversation
…s too #33 fixed benchmarks/libero, but benchmarks/libero-plus/scheduler.py carried the identical line and kept the bug. run_eval.sh defaults SERVER_PYTHON=python and expands it with `command -v`, which inside an activated environment yields .venv/bin/python -- a symlink for both `uv venv` and the stdlib `python3 -m venv`. Resolving that symlink reaches the base interpreter, and starting it directly leaves no pyvenv.cfg beside the executable, so CPython never activates the venv and deploy.py comes up without its site-packages. Verified on Linux: for a stdlib venv, .resolve() rewrites .venv/bin/python to /usr/bin/python3.12 and importing a package present only in the venv raises ModuleNotFoundError, while .absolute() keeps sys.prefix pointing at the venv. Also record why these two lines must not use .resolve(). The adjacent comment explains keeping alias paths readable in logs and manifests, which is a different concern and does not cover the interpreter case -- without a note the call is easy to mistake for an oversight and "clean up" back to .resolve(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SCreatorX
approved these changes
Sep 17, 2026
SCreatorX
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified the reasoning: .venv/bin/python is a symlink, so .resolve() hands deploy.py the base interpreter without pyvenv.cfg next to it and the venv site-packages are lost. .absolute() keeps the symlink and still gives an absolute path. The two schedulers are the only places server_python is normalized, and the downstream consumers (_server_command and the _preflight existence check) work with either form. Consistent with how libero_python is already handled. CI lint + tests green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes and includes #33 — @seveirbian's commit is carried here unchanged, so merging this closes that PR with their authorship intact.
run_eval.shdefaultsSERVER_PYTHON=pythonand expands it withcommand -v, which inside an activated environment yields.venv/bin/python. That is a symlink for bothuv venvand the stdlibpython3 -m venvshown in the README..resolve()follows it to the base interpreter, and starting that binary directly leaves nopyvenv.cfgbeside the executable, so CPython never activates the venv anddeploy.pycomes up without its site-packages.#33 fixed
benchmarks/libero/scheduler.py.benchmarks/libero-plus/scheduler.pycarried the identical line — that block is otherwise a copy, differing only in thenum_trialsdefault — andbenchmarks/libero-plus/run_eval.shhas the sameSERVER_PYTHON:-pythondefault, so the bug was equally reachable there. Those two were the only occurrences in the repo.Verification
On Linux, with a stdlib venv and a module present only in that venv:
sys.prefix.absolute()→/tmp/x/.venv/bin/python/tmp/x/.venv.resolve()→/usr/bin/python3.12/usrModuleNotFoundErrorwhich reproduces the reported failure exactly.
ruffclean,pytest tests/benchmarks/66 passed, pre-commit hooks pass.Note on the comments
Each
.absolute()call now carries its own rationale. The existing comment is about keeping alias paths readable in logs and manifests — a different concern that does not cover the interpreter case, andserver_pythonnever reachesmanifest.jsonat all. Without a note the call reads like an oversight and is easy to "clean up" back to.resolve().No regression test is included: the normalization is inline in
main(), which then runs_preflightand launches subprocesses, so covering it would require extracting the path handling into its own function first.🤖 Generated with Claude Code