Skip to content

Fix Windows training: UTF-8 logs, direct plot spawns, safe checkpoint replace - #32

Open
TomTheHand wants to merge 1 commit into
volotat:mainfrom
TomTheHand:fix/windows-runtime
Open

TomTheHand wants to merge 1 commit into
volotat:mainfrom
TomTheHand:fix/windows-runtime

Conversation

@TomTheHand

Copy link
Copy Markdown

Summary

Three Windows-runtime failures, each hit on a live training run on this machine:

  1. Log writers used the locale default encoding (cp1252 here). The model emits raw bytes, so a sample containing an unencodable character raised UnicodeEncodeError at sample time and killed the run mid-training. All four log writers are now explicit UTF-8, with errors="replace" on the session banner and sample log, where unencodable raw bytes are expected rather than exceptional.
  2. refresh_plots() spawned a bash -c "... && ..." chain. On Windows, CreateProcess may resolve bash to WSL's bash.exe, which cannot execute D:/... paths - and stderr went to devnull, so every plot refresh failed silently. The tools are now spawned directly via sys.executable with argv lists. All children are tracked (a later spawn failing no longer loses earlier PIDs), and the skip-if-still-running guard is preserved; the two plots render concurrently, which is safe because they write distinct outputs.
  3. Checkpoint saves publish with os.replace, which raises PermissionError (WinError 5) on Windows when any reader holds the destination - our own unwaited plot children open manifest.json and do exactly that. The window is milliseconds wide: one save survived, the next died. A bounded retry (8 attempts, exponential backoff, ~6.4 s worst case) keeps publication atomic; persistent failures still raise, and non-permission errors propagate immediately.

POSIX behavior is unchanged throughout.

Tests

tests/test_windows_runtime.py (10 tests, no torch import, no training, no production files): unicode sample-log writes, plot argv/process-state handling, retry exhaustion and immediate propagation of non-permission errors, plus a real Windows reader-lock race executed in a temp directory.

Untouched upstream sources reproduce these failures (5 failures + 3 errors, including the actual cp1252 UnicodeEncodeError and actual WinError 5); this branch passes all 10.

python -m unittest discover -s tests -v
# Ran 10 tests in 0.574s -- OK

… replace

Three Windows-runtime failures, each hit on a live run:

- Log writers used the locale default (cp1252 here); a raw-byte sample
  with an unencodable character raised UnicodeEncodeError at sample
  time and killed training. All four writers are now explicit UTF-8,
  with errors=replace on the sample/session pair.
- refresh_plots spawned a bash -c chain; Windows resolves bash to WSL's
  bash.exe, which cannot run D:/ paths, and stderr was devnull'd - so
  every plot refresh failed silently. Spawn each tool directly via
  sys.executable, track all children (a later spawn failing no longer
  loses the earlier PIDs), and keep the skip-if-still-running guard.
- Checkpoint saves publish with os.replace, which raises WinError 5 if
  any reader holds the destination - our own unwaited plot children do
  exactly that. Bounded retry with exponential backoff keeps
  publication atomic; persistent failures still raise.

POSIX behavior is unchanged. Regression tests cover unicode sample
writes, plot argv/process state, retry exhaustion, and a real Windows
reader-lock race; untouched upstream fails them.
@TomTheHand

Copy link
Copy Markdown
Author

If it's not obvious enough, the above was written by AI 😅 Sorry. But as with my other PR, a few small changes to file handling get mini-AGI running on Windows without hurting anything else.

GroveMinting pushed a commit to GroveMinting/mini-AGI that referenced this pull request Oct 22, 2026
… replace

Carry upstream PR volotat#32 into the fork. The store conflict is resolved by keeping the fork manifest writer and prune-after-publish ordering, while routing every publish through the new retrying _replace helper; _savez keeps temp cleanup on failure. The Windows sample test now exercises the fork implementation.
GroveMinting added a commit to GroveMinting/mini-AGI that referenced this pull request Oct 22, 2026
Upstream has not been merging community PRs. Record the fork-first policy, close the stale upstream handoff wording, and document the carried Windows runtime fixes plus the pending aa9d53f port.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant