Skip to content

QCEngine Windows Unicode Fix - #508

Open
loriab wants to merge 2 commits into
MolSSI:masterfrom
loriab:unicodewin
Open

QCEngine Windows Unicode Fix#508
loriab wants to merge 2 commits into
MolSSI:masterfrom
loriab:unicodewin

Conversation

@loriab

@loriab loriab commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Description

I started having trouble with a Psi4 test case (tests/nbody-multi-leve/) at psi4/psi4#3389 only on Windows and having to do with the § symbol. I'm not sure about the timing but perhaps something changed in GHA Win CI? I've compensated for it in psi4 with envvar PYTHONIOENCODING=utf-8, but AI (Haiku 4.5) suggests this is the more robust sol'n.

I've modified /psi/gits/QCEngine_next/qcengine/util.py to add robust error handling for subprocess output decoding on Windows.

The Fix (2-line change):

  • Line 406: Changed stdout.getvalue().decode() → stdout.getvalue().decode(errors='replace')
  • Line 407: Changed stderr.getvalue().decode() → stderr.getvalue().decode(errors='replace')

Why This is the Right Upstream Fix:

  1. Root cause location: The popen() function is where qcengine actually decodes subprocess output - it's the
    central point of failure
  2. Robust and safe: Using errors='replace' is the Python standard for handling uncertain encodings. It:
    - Converts invalid UTF-8 sequences to U+FFFD (replacement character)
    - Never crashes, just replaces bad bytes
    - Is backward compatible - valid UTF-8 works exactly the same
  3. Comprehensive: This fix applies to ALL qcengine subprocess calls, not just Psi4
  4. Minimal: Only adds an error handling parameter - no logic changes

Changelog description

Status

  • Code base linted
  • Ready to go

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 17.39130% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.92%. Comparing base (cdda366) to head (8169b0b).
⚠️ Report is 26 commits behind head on master.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

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 QCEngine’s subprocess output capture on Windows by preventing UnicodeDecodeError when decoding stdout/stderr, addressing failures triggered by non-UTF-8 bytes (e.g., Windows code pages).

Changes:

  • Decode captured stdout/stderr with errors="replace" to avoid crashing on invalid byte sequences.
  • Add inline commentary explaining the Windows/non-UTF-8 motivation for the change.

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

Comment thread qcengine/util.py
Comment on lines 400 to +404
stderr_reader.join()

# Retrieve the standard output for the process
ret["stdout"] = stdout.getvalue().decode()
ret["stderr"] = stderr.getvalue().decode()
# Use 'replace' error handling for robustness across platforms
# On Windows, subprocess output may contain non-UTF-8 bytes (e.g., code pages)
Comment thread qcengine/util.py
@loriab
loriab marked this pull request as ready for review June 26, 2026 23:17
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.

2 participants