Repository navigation
fix(trainer): resolve LocalProcessBackend Windows compatibility issues - #762
kingabdulsalamsas111-lgtm wants to merge 1 commit into
Conversation
- Validate bash resolves to a working executable instead of relying on unqualified PATH lookup, which could pick a broken WSL relay over a working Git Bash install (fixes kubeflow#757) - Fix subprocess.Popen env= replacing the entire parent environment on Windows instead of extending it, which wiped PATH for the training subprocess - Normalize backslash paths (venv_dir, python_bin) to forward slashes, required by bash script templates - Detect Scripts/ vs bin/ venv layout dynamically, since Windows venvs use Scripts/ rather than the Unix bin/ convention Signed-off-by: kingabdulsalamsas111-lgtm <kingabdulsalamsas111@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@robert-bell @andreyvelich PR #762 is ready for review, with all Windows LocalProcessBackend issues fixed and verified end-to-end. |
aniket-3001
left a comment
aniket-3001
left a comment
There was a problem hiding this comment.
repro'd #757 on win11 (git bash + wsl): which("bash") gives git bash but subprocess launches the wsl one, main fails with [WinError 87]. that's the empty env={}, the env merge in job.py fixes it, nice.
tried it on linux too (python:3.11 container), 3 things inline. unit tests pass there since they mock the script, so CI won't catch these. maybe a test for the generated script on both nt and posix?
unrelated, hit it while testing: python/temp path with a space breaks the script, vars aren't quoted (C:/Users/Aniket: No such file or directory). quoting got me further but the torchrun line and cleanup need it too, so separate issue? can open one
| $PYENV_LOCATION/bin/python -m ensurepip --upgrade --default-pip | ||
| if [ -d "$PYENV_LOCATION/Scripts" ]; then VENV_BIN="$PYENV_LOCATION/Scripts"; else VENV_BIN="$PYENV_LOCATION/bin"; fi | ||
| source $VENV_BIN/activate | ||
| $VENV_BIN/python.exe -m ensurepip --upgrade --default-pip |
There was a problem hiding this comment.
this runs on every OS but linux/mac venvs only have bin/python, no python.exe. job now dies here (bin/python.exe: No such file or directory), on main it gets through ensurepip. maybe a VENV_PYTHON next to VENV_BIN?
| _c = [os.path.join(venv_bin_dir, local_exec_constants.TORCH_COMMAND)] | ||
| trainer.set_command(tuple(_c)) | ||
| torch_command = str( | ||
| Path(venv_bin_dir) / f"{local_exec_constants.TORCH_COMMAND}.exe" |
There was a problem hiding this comment.
same on linux, script ends up calling bin/torchrun.exe. the Scripts vs bin bit above checks os.name, this probably needs it too
| return t.substitute(**mapping) | ||
|
|
||
|
|
||
| def _resolve_bash() -> str: |
There was a problem hiding this comment.
tried a healthy wsl: bash --version returns 0 so if System32�ash.exe is before git on PATH it gets picked. job then runs in wsl and fails on the windows paths (/bin/bash: C:/Users/...: No such file or directory). so broken wsl is fixed, working wsl ahead of git isn't. prefer the git paths first, or check uname -s?
This branch has not been deployed
2 participants
What this PR does / why we need it:
Resolves both issues discussed in #757:
Bash resolution —
LocalProcessBackendcalledsubprocess.Popenwith the bare string"bash", which on Windows could resolve to a broken WSL relay launcher instead of a working Git Bash install. Now validates that the resolved bash actually runs before using it.Environment wiping —
LocalJobcalledsubprocess.Popen(..., env=self.env)with an explicitenvdict. On Windows, passing any explicitenv=replaces the entire parent environment instead of extending it, wiping PATH for the training subprocess regardless of the user's system PATH. Now mergesself.envintoos.environ.While verifying these two fixes end-to-end on Windows, I also had to fix two supporting issues that were blocking the training subprocess from starting at all:
Path normalization — Windows paths (
venv_dir,python_bin) use backslashes, which bash interprets as escape characters, corrupting paths inside the generated bash script. Now normalized to forward slashes.Scripts/ vs bin/ — Python's
venvmodule creates aScripts/folder on Windows, notbin/(the Unix convention the code assumed). The activate script and binary paths now detect this dynamically.Verification:
I tested this locally on Windows with
LocalProcessBackend+ thetorch-distributedruntime, before and after the fix:RPC call... handle differs...) caused by bash resolving to a broken WSL relay.torch,torchvision) end-to-end via pip — verified across multiple runs with full logs.Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #757
Checklist: