fix: only skip venv setup when the venv was setup - #2388
Conversation
|
/ok to test e04eacf |
|
/claude review |
|
SHIP — no reliability concerns. Correctly closes the mid-install-death gap in The fix is sound:
Backward compat is fine: pre-existing markerless venvs rebuild once ( |
github-actions
Bot
added
the
sla:triage-overdue
|
/ok to test 45b436a |
45b436a to
f09229c
Compare
f09229c to
c7e09fd
Compare
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
c7e09fd to
43ee085
Compare
| and venv_python_fpath.exists() | ||
| and venv_activate_fpath.exists() | ||
| and venv_marker_fpath.exists() | ||
| ) |
There was a problem hiding this comment.
this new marker cannot distinguish an interrupted installation from an environment created by an older Gym release. Every existing, pre-baked, or custom venv is missing this path.
With skip_venv_if_present=true, this rejects those usable environments, and the setup branch then recursively deletes them before reinstalling.
This can fail for offline, read-only, and non-root containers despite already having a working venv.
Could we preserve markerless legacy environments, or require other evidence of an interrupted Gym-owned build for detection instead?
| prefix_cmd = f" > >(sed 's/^/({prefix}) /') 2> >(sed 's/^/({prefix}) /' >&2)" | ||
| env_setup_cmd = f"{uv_venv_cmd}{prefix_cmd} && source {venv_activate_fpath} && {install_cmd}{prefix_cmd}" | ||
| # A venv that lacks the marker was not fully built; remove it. | ||
| env_setup_cmd = ( |
There was a problem hiding this comment.
uv_venv_dir is configurable, but the new marker checks and recursive cleanup interpolate its path without shell quoting. For example, a root such as /tmp/gym cache makes rm -rf receive /tmp/gym as a separate operand, which can delete an unrelated tree.
Please quote every generated path (for example with shlex.quote) and validate ownership/containment before recursive deletion, or perform cleanup through validated Path operations instead of generated shell.
|
|
||
| prefix_cmd = f" > >(sed 's/^/({prefix}) /') 2> >(sed 's/^/({prefix}) /' >&2)" | ||
| env_setup_cmd = f"{uv_venv_cmd}{prefix_cmd} && source {venv_activate_fpath} && {install_cmd}{prefix_cmd}" | ||
| # A venv that lacks the marker was not fully built; remove it. |
There was a problem hiding this comment.
Normal startup launches configured instances concurrently. Copied instances can resolve to the same component venv.
If the marker is absent, both setup processes can enter this block and one can delete the directory while the other is installing into it; a marker may also become visible while another writer is still active.
setup should deduplicate by resolved venv or hold a per-venv lock. alternatively this coul dbe built in a temporary staging directory and atomically replace the target directory.
|
|
||
| assert "uv pip install" in actual_command | ||
| # A venv that lacks the marker was not fully built; remove it. | ||
| assert actual_command.split(" && ")[1] == ( |
There was a problem hiding this comment.
This regression test only inspects the generated command string, so it cannot catch deletion of a valid legacy venv, permission failures on read-only paths, unsafe paths containing spaces, concurrent setup races, or missing log attribution.
could we execute the setup command with a fake uv and cover successful completion plus failed rm/install/touch, a markerless legacy environment, a path containing spaces, and two setups sharing one venv? The cleanup and marker actions should also be logged with the server prefix so failures are attributable during concurrent startup.
| env_setup_cmd = f"{uv_venv_cmd}{prefix_cmd} && source {venv_activate_fpath} && {install_cmd}{prefix_cmd}" | ||
| # A venv that lacks the marker was not fully built; remove it. | ||
| env_setup_cmd = ( | ||
| f"{{ [ -e {venv_marker_fpath} ] || rm -rf {venv_path}; }} && " |
There was a problem hiding this comment.
When nemo_gym_log_dir is configured, the outer 2>&1 | tee captures failures from this entire chain and pipefail preserves the nonzero status. However, only the uv commands receive the per-server prefix: rm, touch, and source errors are unlabelled in concurrent terminal output, successful deletion is silent, and the decision to classify and remove a markerless venv is never recorded. Could we log the detected venv state and selected action before deletion, and apply the server prefix to every setup step?
github-actions
Bot
removed
the
sla:triage-overdue
2 participants
skip_venv_if_presenttrusts based on the existence ofbin/pythonandbin/activate.Both these files appear long before the
venvis actually built. This allows for zombie venvs to be left behind which pass the skip check but are unusable.This PR adds a completion marker.