fix: make train --hf-jobs work on Windows - #35
Open
lboucher26 wants to merge 2 commits into
Open
lboucher26 wants to merge 2 commits into
lboucher26 wants to merge 2 commits into
Conversation
argv[0] is however the OS spells the installed console script, and Windows
spells it `train.exe`. `_invoked_as_train()` stripped `.py` and `-script`
but not `.exe`, so the bare-name comparison never matched, the hook declined
to intercept, and the flag fell through to tyro:
Unrecognized options: --hf-jobs, --dry-run
i.e. the exact failure this hook was added to prevent, one platform over.
Nothing warns about it: the submission path is simply absent on Windows.
tyro's error hides the cause, printing the program as `...\Scripts\train`
with the extension stripped, so the name looks like it should have matched.
The existing cases all used posix argv[0] (`/x/.venv/bin/train`), so they
passed throughout. Parametrized over the spellings a console script actually
takes, with `play.exe` added to the negative case so the suffix strip can't
over-reach into "every .exe is the trainer".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019oSADSTKCB8mSdb6yFpvLt
`wandb login` follows the platform's netrc convention and writes `~/_netrc`
on Windows, printing the path it chose:
Appending key for api.wandb.ai to your netrc file: C:\Users\...\_netrc
`_wandb_api_key()` only looked at `~/.netrc`, so submission reported
[wandb] x no API key found (checked $WANDB_API_KEY and ~/.netrc).
Run `wandb login` locally, or pass --no-wandb to skip.
directly after a successful login, and refused to forward a key that was
sitting in the user's home directory. The advice in the message is also a
dead end on Windows: running `wandb login` again just rewrites `_netrc`.
Try both spellings. The stdlib `netrc` default path doesn't know about
`_netrc`, and this passes an explicit path anyway, so the fallback has to be
here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019oSADSTKCB8mSdb6yFpvLt
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.
train <task> --hf-jobsis unusable on Windows. Two independent bugs, bothone-liners, both invisible in CI because they only trigger on
nt. Foundwhile following
scripts/hf/README.mdfrom a clean clone.1. The flag is never intercepted (
train_hook.py)_invoked_as_train()compares the console script's bare name to"train",stripping
.pyand-script. Windows installs the script astrain.exe, sothe comparison fails, the hook returns early, and the flag reaches tyro:
Which is the same symptom #20 fixed on Linux. The cause is easy to miss
because tyro prints the program without the extension —
C:\...\.venv\Scripts\train— so argv[0] looks like it should have matched.Confirmed with a
sitecustomizeprobe on the real invocation:Every existing case in
test_hf_jobs_flag.pyuses posix argv[0](
/x/.venv/bin/train), so the suite stayed green. Now parametrized over thespellings a console script actually takes, with
play.exeadded to thenegative case so the suffix strip can't over-reach.
2. The wandb key is never found (
hf_jobs.py)wandb loginwrites~/_netrcon Windows and says so:_wandb_api_key()reads only~/.netrc, so submission reportsimmediately after a successful login. The suggested remedy is a dead end:
wandb loginagain just rewrites_netrc. Now tries both spellings, with anew
test_hf_jobs_wandb_key.pycovering both, env-var precedence, and theno-key path.
Verification
uv run --with pytest pytest tests/test_hf_jobs_flag.py tests/test_hf_jobs_wandb_key.py— 21 passed (see note below for the 1 unrelated pre-existing failure).
End-to-end on Windows, which previously could not get past argument parsing:
Not fixed here
test_train_on_path_is_a_mjlab_traineralso fails on Windows, for anunrelated reason: it looks for
b"mjlab"in the first 8192 bytes of theresolved script, but a Windows console script is a PE launcher stub with the
payload appended at the end. In this venv
train.exeis 46080 bytes andmjlabfirst appears at offset 43543, so the check can't pass on Windowseven when the script is correct. Left alone to keep this PR to the two
functional bugs — happy to add a commit or open a separate issue, whichever
you prefer.
🤖 Generated with Claude Code
https://claude.ai/code/session_019oSADSTKCB8mSdb6yFpvLt