Skip to content

fix(benchmarks): UTF-8 encoding + ASCII print chrome on Windows for the 3 sibling runners - #1382

Merged
igorls merged 1 commit into
MemPalace:developfrom
techempower-org:fix/benchmarks-windows-encoding
Aug 18, 2026
Merged

fix(benchmarks): UTF-8 encoding + ASCII print chrome on Windows for the 3 sibling runners#1382
igorls merged 1 commit into
MemPalace:developfrom
techempower-org:fix/benchmarks-windows-encoding

Conversation

@jphein

@jphein jphein commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #1204 (LongMemEval Windows encoding) per @mschultheiss83's request. Applies the same narrow Windows correctness pattern to the three sibling benchmark runners that share the same shape:

  • benchmarks/locomo_bench.py
  • benchmarks/membench_bench.py
  • benchmarks/convomem_bench.py

What changed (per file)

  • All local open(path) / open(path, "w") calls now pass encoding="utf-8" (was inheriting the platform default — cp1252 on Windows, GBK on Chinese Windows). Mirrors benchmarks: harden longmemeval runner for Windows encoding #1204's pattern at longmemeval_bench.py:2917 / :2927 / :2957 / :2998 / :3031.
  • Non-ASCII separator characters in print(...) chrome replaced with ASCII (-, ->) so a default cp1252 console doesn't raise UnicodeEncodeError mid-run. Comments/docstrings (which never hit stdout) are untouched.
  • urllib.request.urlopen(...) calls intentionally left alone — they don't open local files, and the original benchmarks: harden longmemeval runner for Windows encoding #1204 audit didn't flag them.

Total diff: +16 / -16 across 3 files. No behavior change beyond Windows correctness.

Audit provenance

The file selection isn't speculative — jphein already audited the codebase in #1204's review thread and flagged the exact open(...) lines in these three files. @mschultheiss83 asked for the follow-up at #1204 (comment). This PR is just the mechanical follow-through.

Out of scope

  • The pre-existing C901 ruff finding on locomo_bench.py:621 (run_benchmark complexity 36) is from upstream develop (verified by stash + re-lint on stock branch); not from this PR.
  • Non-encoding-related Windows fixes (path separators, line-ending handling) — those would be a different audit.

Test plan

  • CI green on Linux / macOS / Windows
  • Manual smoke (Linux): each of the three runners imports without error and the read/write paths exercised by --cache-dir arguments don't regress
  • No new ruff lint errors introduced (verified locally — pre-existing C901 unchanged)

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the three benchmark runner scripts (locomo, membench, convomem) for native Windows execution by making local JSON file I/O explicitly UTF-8 and replacing non-ASCII console separators/arrows that can trigger UnicodeEncodeError on default cp1252 terminals.

Changes:

  • Add encoding="utf-8" to local open(...) calls used for reading/writing JSON caches and results in the three runners.
  • Replace non-ASCII console “chrome” characters (, ) used in print(...) output with ASCII-safe equivalents (-, ->).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
benchmarks/membench_bench.py Explicit UTF-8 for JSON file read/write; ASCII-only separator output.
benchmarks/locomo_bench.py Explicit UTF-8 for dataset/cache/results I/O; ASCII-only separator and progress output.
benchmarks/convomem_bench.py Explicit UTF-8 for cache/result JSON I/O; ASCII-only separator output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jphein
jphein force-pushed the fix/benchmarks-windows-encoding branch from 736037c to ea63b28 Compare May 8, 2026 21:42
… Windows

Follow-up to MemPalace#1204 per @mschultheiss83's request — applies the same
narrow Windows encoding fix to the three remaining LongMemEval-shaped
benchmark runners that share the audit pattern jphein flagged in
MemPalace#1204's review (`benchmarks/{locomo,membench,convomem}_bench.py`).

For each file:

  - All `open(path)` / `open(path, "w")` calls gained `encoding="utf-8"`
    so cached benchmark JSON, palace-cache files, and result files are
    always read/written as UTF-8 instead of inheriting the platform
    default (cp1252 on Windows, GBK on Chinese Windows, etc.). Same
    pattern as MemPalace#1204's #2917 / #2927 / #2957 / #2998 / #3031.

  - Replaced non-ASCII separator characters in `print(...)` chrome
    with ASCII equivalents (`─` → `-`, `→` → `->`) so the runners
    don't raise `UnicodeEncodeError` on a default cp1252 console.
    Comments and docstrings (which never hit stdout) are untouched.

  - `urllib.request.urlopen(...)` calls left alone — they don't open
    local files, the original audit didn't flag them.

Audit was already done in MemPalace#1204's thread; this PR carries the
mechanical follow-through. No behavior change beyond Windows
correctness.

Refs MemPalace#1203 (the original Windows reproducer), MemPalace#1204 (sibling PR
that fixed `longmemeval_bench.py`).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jphein
jphein force-pushed the fix/benchmarks-windows-encoding branch from ea63b28 to 91d81d5 Compare May 11, 2026 18:55
@jphein jphein added area/ci CI/CD and workflows area/windows Windows-specific bugs and compatibility bug Something isn't working labels May 11, 2026
@mschultheiss83

Copy link
Copy Markdown

@bensig @milla-jovovich please review...

@jphein

jphein commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Trivial Windows fix: adds explicit encoding="utf-8" to the local JSON I/O in the three benchmark runners and swaps the non-ASCII console chrome (, ) for ASCII so default cp1252 terminals don't raise UnicodeEncodeError. +16/-16 across the three sibling runners, no behavior change off Windows. Thanks @mschultheiss83 for flagging interest above — should be a quick one to land. Merges clean on develop.

@igorls
igorls merged commit 7843291 into MemPalace:develop Aug 18, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci CI/CD and workflows area/windows Windows-specific bugs and compatibility bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants