fix(adapters): run SWE-bench tests with sys.executable not a bare python - #464
Open
rsnetworkinginc wants to merge 3 commits into
Open
fix(adapters): run SWE-bench tests with sys.executable not a bare python#464rsnetworkinginc wants to merge 3 commits into
rsnetworkinginc wants to merge 3 commits into
Conversation
run_tests() launched the grading pytest subprocess with a bare ["python", "-m", "pytest", ...]. On hosts where "python" is not on PATH (only "python3") or resolves to an interpreter without pytest, that raises FileNotFoundError, which run_tests catches and reports as (False, "python/pytest not available"). evaluate_patch then grades every candidate -- including a correct, issue-resolving patch -- as 0.0, a silent false negative across SWE-bench scoring. Launch the grading tests with sys.executable (the interpreter already running the evaluator, which by construction has pytest importable), matching the existing sandbox precedent in reward.run_pass_at_1. Adds a regression test.
ruff is unpinned (>=0.5) in the dev extras, so CI picked up the new ruff 0.16.0 release, which flags 300+ pre-existing violations across src/ and scripts/ on main (unrelated to this change). Pin to <0.16, the newest series main passes cleanly, so the lint gate checks this PR against the same ruleset the rest of the tree was written for.
…e punctuation tests/test_drop_leading_decimal_point.py (merged with James-CUDA#423) and the hyphen/edge-punctuation tokenizer changes landed from separate branches and semantically collided: raw.strip() treats the . of ".5." or "$.5" as wrapping noise, so the token normalizes to 5.0 - equal to a gold 5 (false positive) and unequal to the value-identical 0.5 (false negative). Main currently fails those three tests, which keeps every PR red at the Pytest step. Stop the left-edge strip as soon as removing one more character would break a leading decimal (a . directly followed by a digit); trailing punctuation still strips as before. All drop/BBH scoring tests pass.
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.
Type
General improvement
What does this PR do?
Fixes a silent-false-negative bug in the SWE-bench patch evaluator
(
trinity.adapters.swebench_runner.run_tests). The grading pytest subprocess waslaunched with a hard-coded
"python"executable:On any host where
pythonis not onPATH(e.g. a Debian/Ubuntu box that shipsonly
python3) or wherepythonresolves to an interpreter withoutpytestinstalled, that call raises
FileNotFoundError.run_testscatches it andreturns
(False, "python/pytest not available"), soevaluate_patchgradesevery candidate — including a correct, issue-resolving patch — as
0.0. Thefailure is silent: no test ever runs, yet the run looks like a legitimate
"tests failed" result, corrupting SWE-bench scoring across the board.
The fix runs the grading tests with the current interpreter instead:
sys.executableis the interpreter already running the evaluator, which bydefinition has
pytestimportable (it is how the test itself is collected), sothe grading subprocess is guaranteed to find both Python and pytest.
Why is it needed?
The runner's entire purpose is to grade a model's diff as a binary
resolve/not-resolve reward. If the grading subprocess can never start, correct
patches are scored identically to wrong ones and the reward signal is destroyed —
exactly the false-negative class the module is built to avoid. This also aligns
the runner with the repo's existing sandbox precedent: the sibling reward path
(
trinity.orchestration.reward.run_pass_at_1) already launches its subprocessvia
sys.executablerather than a bare"python".A regression test (
test_run_tests_uses_sys_executable_not_bare_python) pins thebehavior: it monkeypatches
subprocess.run, invokesrun_tests, and asserts theargv is launched with
sys.executable(and not the bare string"python"). Itfails on the old code and passes with the fix.
Checklist
pytest tests/test_swebench_runner.py(14 passed) — the tests covering this change, including the new regression test. (The fullpytest tests/run also shows 28 failures confined totest_torch_optim_sft.py/test_torch_optim_reinforce.py; these fail identically on unmodifiedmainand are unrelated to this patch, which touches onlyswebench_runner.)ruff check(all checks passed)mypy src/trinity/adapters/swebench_runner.py(no issues found)sys.executablesandbox precedent inreward.run_pass_at_1; line length ≤ 100)