fix(scripts): force UTF-8 on stdout/stderr so runs survive a cp1252 console - #201
Conversation
…onsole Progress output contains arrow and box-drawing characters (e.g. the '[2/6 REFLECT] failure=0->0 groups' line and the banner rules). On a Windows console that defaults to cp1252, writing them raises UnicodeEncodeError and kills the process partway through a run -- after rollouts and reflect calls have already been paid for. Both entry points now reconfigure stdout/stderr to UTF-8 with errors='replace' at import time, guarded by hasattr so redirected or exotic streams are left alone.
There was a problem hiding this comment.
Pull request overview
This PR hardens the train and eval_only entry-point scripts against UnicodeEncodeError on Windows consoles that default to cp1252 by forcing stdout/stderr to UTF-8 (with errors="replace") early in process startup.
Changes:
- Reconfigure
sys.stdout/sys.stderrto UTF-8 at import time inscripts/train.pyandscripts/eval_only.py. - Add pytest coverage to verify the entry-point scripts perform the reconfiguration and that arrow/box-drawing output becomes writable after reconfigure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_console_encoding.py | Adds tests for entry-point stream reconfiguration and for writing previously-problematic characters. |
| scripts/train.py | Reconfigures stdout/stderr to UTF-8 early to prevent UnicodeEncodeError during progress output. |
| scripts/eval_only.py | Reconfigures stdout/stderr to UTF-8 early to prevent UnicodeEncodeError during progress output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ready-utf8 streams - Never raise from stream reconfigure (wrap in try/except); a stream that exposes an incompatible reconfigure() no longer aborts startup. - Skip streams already encoded as UTF-8 so redirected output is not needlessly re-encoded, while cp1252 consoles and cp1252 file redirects are still fixed. - Drop the unused 'script' parametrization on the arrow-output test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/train.py:34
- This stdout/stderr reconfiguration block is duplicated in both entry-point scripts. Duplicated startup logic tends to drift over time (e.g., if one script later adjusts the encoding detection or exception list), so it would be more maintainable to factor this into a small shared helper (e.g., skillopt/utils/console.py: force_utf8_stdout_stderr()) and call it from each script.
# Progress output contains box-drawing and arrow characters. On a Windows
# console defaulting to cp1252 those raise UnicodeEncodeError mid-run, which
# kills a training loop after real work has already been done. A cp1252 file
# redirection crashes identically, so this deliberately is not gated on
# isatty(); it is a best-effort no-op for streams that are already UTF-8 or
# expose an incompatible reconfigure().
scripts/eval_only.py:31
- This stdout/stderr reconfiguration logic is identical to the block in scripts/train.py. To avoid the two scripts diverging, consider extracting it into a shared helper function and reusing it from both entry points.
# Progress output contains box-drawing and arrow characters. On a Windows
# console defaulting to cp1252 those raise UnicodeEncodeError mid-run. A cp1252
# file redirection crashes identically, so this deliberately is not gated on
# isatty(); it is a best-effort no-op for streams that are already UTF-8 or
# expose an incompatible reconfigure().
Addresses review: the reconfigure block was duplicated in both entry points and would drift. Move it to skillopt/utils/console.force_utf8_stdout_stderr() and call it from train.py and eval_only.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skillopt/utils/console.py:24
- The UTF-8 short-circuit check won’t recognize common encoding spellings like
utf_8(underscore), so the function may unnecessarily callreconfigure()even when the stream is already UTF-8. Normalizing underscores as well keeps the behavior aligned with the docstring (“no-op for streams that are already UTF-8”).
if (getattr(stream, "encoding", "") or "").lower().replace("-", "") == "utf8":
continue
…lper Addresses re-review: the short-circuit normalized only hyphens, so an encoding reported as 'utf_8' (underscore) still triggered an unnecessary reconfigure(). Normalize underscores too, and add coverage for the utf-8 / utf_8 / UTF8 spellings vs cp1252. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Follow-up in |
Yifan Yang (Yif-Yang)
left a comment
There was a problem hiding this comment.
Validated the guarded UTF-8 stream configuration and regression coverage. This is a focused Windows reliability fix that leaves unsupported or redirected streams unchanged. Thank you.
Progress output contains arrow and box-drawing characters (e.g. the '[2/6 REFLECT] failure=0->0 groups' line and the banner rules). On a Windows console that defaults to cp1252, writing them raises UnicodeEncodeError and kills the process partway through a run -- after rollouts and reflect calls have already been paid for.
Both entry points now reconfigure stdout/stderr to UTF-8 with errors='replace' at import time, guarded by hasattr so redirected or exotic streams are left alone.