fix: UTF-8 result writes, and make --force actually discard prior results - #22
Open
dat999zx wants to merge 1 commit into
Open
fix: UTF-8 result writes, and make --force actually discard prior results#22dat999zx wants to merge 1 commit into
dat999zx wants to merge 1 commit into
Conversation
…ults
Two independent bugs, both hit on any platform whose default encoding is not
UTF-8, and both silent.
1. Result writes assumed a UTF-8 default encoding
open(path, "w") with no encoding uses the platform default, which is cp1252
on a stock Windows install. The first non-ASCII value in a retrieved context
or a saved result then raises UnicodeEncodeError and kills the run.
FactConsolidation reaches this immediately -- "pesäpallo" is one of the gold
answers -- but nothing here is method-specific. The writes are in agent.py
and main.py, on paths every method goes through, so every method crashes.
Five call sites now pass encoding="utf-8". The two that dump retrieved
context and final results also pass ensure_ascii=False, so non-ASCII stays
readable in the JSON rather than being escaped.
2. --force did not discard prior results
The flag is documented as "Force re-run even if results already exist", but
it only reached should_skip_context. load_existing_results ran
unconditionally, so a forced run still:
- seeded metrics from the saved rows, averaging the old scores into the
new number, and
- restored the query high-water mark, so should_skip_query -- which never
took the flag -- skipped the very queries --force asked to re-run.
Re-running after fixing your method therefore reports a figure partly
produced by the old code, with nothing in the output to indicate it. We hit
this: a 5-question run left rows behind, and the next 100-question run
reported 84% while rows 0-4 still carried input_len 25373/20374/16393/6468/
20727 against 108-146 for the rest. Over the 95 clean rows it was 86.3%; the
5 stale rows scored 2/5.
load_existing_results now takes force_rerun and returns empty state when it
is set. Resume behaviour without the flag is unchanged.
Verified: with a saved five-row file, the default path reloads 5 rows and
resumes at query 5; with force_rerun it reloads 0 and resumes at 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent bugs, both silent, both hit outside a UTF-8 default locale. Neither is specific to any one method — they sit on paths every method goes through.
1. Result writes assume a UTF-8 default encoding
open(path, "w")with noencodinguses the platform default, which iscp1252on a stock Windows install. The first non-ASCII value in a retrieved context or a saved result raisesUnicodeEncodeErrorand kills the run.FactConsolidation reaches this immediately —
pesäpallois one of the gold answers — but the writes are inagent.pyandmain.py, so every method crashes, not just one.Five call sites now pass
encoding="utf-8". The two that dump retrieved context and final results also passensure_ascii=False, so non-ASCII stays readable in the JSON instead of being escaped.2.
--forcedid not discard prior results--forceis documented as "Force re-run even if results already exist", but it only reachedshould_skip_context.load_existing_resultsran unconditionally, so a forced run still:metricsfrom the saved rows, averaging old scores into the new number, andshould_skip_query— which never took the flag — skipped the very queries--forceasked to re-run.Re-running after fixing your method therefore reports a figure partly produced by the old code, with nothing in the output to indicate it happened.
We hit this. A broken 5-question run left rows behind; the next 100-question run reported 84%, but rows 0–4 still carried
input_len25373/20374/16393/6468/20727 against 108–146 for the rest. Over the 95 clean rows it was 86.3%; the 5 stale rows scored 2/5.load_existing_resultsnow takesforce_rerunand returns empty state when set. Resume behaviour without the flag is unchanged.Verification
With a saved five-row results file:
--force--forceNote on scope
Kept deliberately to these two fixes so they can be reviewed quickly and independently. Happy to split them into separate PRs if you'd prefer.
input_lenturned out to be a useful contamination check while debugging this — rows produced by a different code path carry a visibly different context size, so a jump in it across rows of one results file means the rows are mixed.