Skip to content

fix(adapters): run SWE-bench tests with sys.executable not a bare python - #464

Open
rsnetworkinginc wants to merge 3 commits into
James-CUDA:mainfrom
rsnetworkinginc:fix/swebench-sys-executable
Open

fix(adapters): run SWE-bench tests with sys.executable not a bare python#464
rsnetworkinginc wants to merge 3 commits into
James-CUDA:mainfrom
rsnetworkinginc:fix/swebench-sys-executable

Conversation

@rsnetworkinginc

Copy link
Copy Markdown

Type

  • General improvement — bug fix, docs, refactor, or infrastructure (welcome but earns no TAO)

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 was
launched with a hard-coded "python" executable:

subprocess.run(["python", "-m", "pytest", ...], ...)

On any host where python is not on PATH (e.g. a Debian/Ubuntu box that ships
only python3) or where python resolves to an interpreter without pytest
installed, that call raises FileNotFoundError. run_tests catches it and
returns (False, "python/pytest not available"), so evaluate_patch grades
every candidate — including a correct, issue-resolving patch — as 0.0. The
failure 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:

subprocess.run([sys.executable, "-m", "pytest", ...], ...)

sys.executable is the interpreter already running the evaluator, which by
definition has pytest importable (it is how the test itself is collected), so
the 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 subprocess
via sys.executable rather than a bare "python".

A regression test (test_run_tests_uses_sys_executable_not_bare_python) pins the
behavior: it monkeypatches subprocess.run, invokes run_tests, and asserts the
argv is launched with sys.executable (and not the bare string "python"). It
fails on the old code and passes with the fix.

Checklist

  • Tests pass: pytest tests/test_swebench_runner.py (14 passed) — the tests covering this change, including the new regression test. (The full pytest tests/ run also shows 28 failures confined to test_torch_optim_sft.py / test_torch_optim_reinforce.py; these fail identically on unmodified main and are unrelated to this patch, which touches only swebench_runner.)
  • Lint passes: ruff check (all checks passed)
  • Type check passes: mypy src/trinity/adapters/swebench_runner.py (no issues found)
  • New code follows existing style (mirrors the sys.executable sandbox precedent in reward.run_pass_at_1; line length ≤ 100)
  • Public functions have docstrings and type annotations

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.
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.

1 participant