tests: read checked-in files as UTF-8 instead of the platform default#7438
tests: read checked-in files as UTF-8 instead of the platform default#7438LeoBorcherding wants to merge 4 commits intounslothai:mainunslothai/unsloth:mainfrom LeoBorcherding:fix-test-source-encodingLeoBorcherding/unsloth:fix-test-source-encodingCopy head branch name to clipboard
Conversation
Path.read_text() with no encoding uses locale.getpreferredencoding(), which is UTF-8 on the Linux runners and cp1252 on a stock Windows install. Nine module-level reads of checked-in source files were relying on that default. studio/backend/routes/inference.py carries the DeepSeek tool-call token regexes, so it holds U+FF5C and U+2581. Under cp1252 that read raised UnicodeDecodeError on byte 0x81 at position 97806, and because the reads run at import time it took test_cancel_atomicity.py and test_cancel_id_wiring.py out at collection, not as failures. Green on CI, permanently broken for a Windows contributor running the suite locally. Adds a guard: at module scope there is no tmp_path fixture, so a bare read_text()/write_text()/open() there is always touching a checked-in file. That makes the rule mechanical enough to enforce with no allowlist, while staying quiet about temp-dir I/O inside test bodies where the platform default is harmless. The repo already spells this correctly in 464 other places; this only stops the stragglers coming back.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f77627fbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Follows up on the Codex review: - add `from __future__ import annotations`, since `str | None` in `_offender` is evaluated at import on Python 3.9 and pyproject declares requires-python ">=3.9,<3.15". - widen the guard from module scope to import time. Class bodies and the bodies of module-level helpers called from an executing statement run during collection too, so `CODE = _extract_mixed_precision_code()` was the same hazard as an inline read. `if __name__ == "__main__":` blocks are skipped: pytest never executes them. - scan studio/backend/tests/ as well as tests/. Both trees are collected on Windows by separate CI jobs, and the offender that started this, test_tool_xml_strip.py reading routes/inference.py, lives there. Widening it surfaced seven more import-time reads of checked-in sources; all now name utf-8.
|
Thanks, all three addressed in a603ce3. P1 P2 The guard now walks import time rather than module scope: class bodies, and the bodies of module-level helpers reached from an executing statement.
Widening surfaced seven more import-time reads of checked-in sources; all fixed in the same commit. Base is merged in. |
|
@codex review |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793bb6a43e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| left = node.test.left | ||
| if not (isinstance(left, ast.Name) and left.id == "__name__"): | ||
| return False | ||
| return any(isinstance(c, ast.Constant) and c.value == "__main__" for c in node.test.comparators) |
There was a problem hiding this comment.
Require equality in the main-guard check
When a test contains if __name__ != "__main__":, this predicate still returns true because it checks only the operands and ignores the comparison operator. Pytest executes that branch, but _import_time_calls skips it entirely, allowing an encoding-less source read—and the Windows collection failure this guard is intended to prevent—to pass unnoticed.
Useful? React with 👍 / 👎.
| if isinstance(func, ast.Name) and func.id in helpers and func.id not in entered: | ||
| entered.add(func.id) | ||
| frontier.append(list(helpers[func.id].body)) | ||
| stack.extend(ast.iter_child_nodes(node)) |
There was a problem hiding this comment.
Exclude deferred expression bodies from the import-time scan
When module-level test data contains a lambda or generator whose body performs read_text() or write_text() later—for example, a lambda passed through parametrization and invoked with a temporary path—walking every child node descends into that deferred body and reports it as an import-time operation. Such reads do not run during collection, so this creates false CI failures for the temporary-file I/O the guard explicitly intends to ignore.
Useful? React with 👍 / 👎.
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) or _is_main_guard(node): | ||
| continue |
There was a problem hiding this comment.
Inspect expressions evaluated while defining functions
When a source read appears in a function decorator, default argument, or annotation—for example, @pytest.mark.parametrize("case", Path("cases.txt").read_text())—Python evaluates it during module import, but this unconditional skip discards the entire FunctionDef, including those definition-time expressions. The guard therefore misses a common collection-time read and allows the Windows decoding regression it is meant to prevent.
Useful? React with 👍 / 👎.
| func = call.func | ||
| if isinstance(func, ast.Attribute) and func.attr in GUARDED_METHODS: | ||
| return f"{func.attr}()" | ||
| # Binary handles have no encoding to name. | ||
| if isinstance(func, ast.Name) and func.id == "open" and "b" not in _open_mode(call): | ||
| return "open()" |
There was a problem hiding this comment.
Detect text reads opened through Path.open
When a test opens checked-in text with Path.open() at module scope, omitting encoding has the same locale-dependent behavior as builtin open(), but _offender ignores the call because "open" is absent from GUARDED_METHODS and the function is an attribute rather than an ast.Name. A standard with source_path.open() as handle: source read can therefore reintroduce the Windows collection failure without tripping this guard.
Useful? React with 👍 / 👎.
| if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) or _is_main_guard(node): | ||
| continue |
There was a problem hiding this comment.
Scan the else arm of a main guard
When an if __name__ == "__main__": statement has an else arm, that arm executes during pytest collection, but treating the entire If as script-only skips both its body and its orelse. An encoding-less read in the import-only else branch is therefore invisible to the guard even though it can fail collection on Windows.
Useful? React with 👍 / 👎.
| if any(kw.arg == "encoding" for kw in call.keywords): | ||
| return None |
There was a problem hiding this comment.
Reject encoding=None as a platform-default read
When a call explicitly passes encoding=None, Python still selects the locale-dependent default encoding, but this check treats the mere presence of the keyword as safe. Thus Path.read_text(encoding=None) or open(path, encoding=None) can still raise the same cp1252 decoding failure on Windows while passing the new guard.
Useful? React with 👍 / 👎.
| if len(call.args) > 1 and isinstance(call.args[1], ast.Constant): | ||
| return str(call.args[1].value) | ||
| for kw in call.keywords: | ||
| if kw.arg == "mode" and isinstance(kw.value, ast.Constant): | ||
| return str(kw.value.value) | ||
| return "r" |
There was a problem hiding this comment.
Avoid assuming dynamic open modes are text
When binary mode is supplied through a constant or variable, such as MODE = "rb"; open(path, MODE), _open_mode cannot see a literal and defaults to "r", so the guard reports an encoding violation for a binary handle. Binary opens cannot accept an encoding argument, leaving legitimate module-level binary fixture reads with no valid way to satisfy this test other than rewriting the call to inline the mode.
Useful? React with 👍 / 👎.
The bug
Path.read_text()with noencodinguseslocale.getpreferredencoding(): UTF-8 on the Linux runners, cp1252 on a stock Windows install. Nine module-level reads of checked-in source files were relying on that default.studio/backend/routes/inference.pycarries the DeepSeek tool-call token regexes, so it holds U+FF5C and U+2581. Under cp1252 that read raises:Because the reads run at import time, this doesn't show up as a test failure. It takes
test_cancel_atomicity.pyandtest_cancel_id_wiring.pyout at collection, so 20 tests silently don't run. Green on CI, permanently broken for a Windows contributor running the suite locally.The fix
Pass
encoding = "utf-8"at the 9 sites. The repo already spells it that way in 464 other places, so this just aligns the stragglers with existing house style.The guard
tests/test_source_read_encoding.pykeeps it from coming back.The trick that makes it enforceable: at module scope there is no
tmp_pathfixture, so a bareread_text()/write_text()/open()there is always touching a checked-in file. That makes the rule mechanical with no allowlist and no false positives, while staying quiet about temp-dir I/O inside test bodies where the platform default is harmless. Binary-modeopen()is skipped since it has no encoding to name.Uses only
astandpathlib: no new dependencies. It sits attests/root, whichrepo-cpu-testsalready auto-discovers, so no workflow change is needed.Verification
scripts/enforce_kwargs_spacing.pyclean,ruff checkclean, and the diff is already run through the repo's ownscripts/run_ruff_format.py(ruff 0.6.9) so pre-commit.ci has nothing to reformat.Scope
Test-only. No runtime or user-facing behavior changes.
Not covered here:
tests/python/test_e2e_no_torch_sandbox.pybuildsopen(...)snippets that run in subprocesses and hit the same decode error through a different mechanism. Different fix, separate PR.