perf(devtools): trim testmon from full verification - #4650
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db1c1e014
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "-p", | ||
| PROGRESS_PLUGIN_NAME, | ||
| *MANAGED_PLUGIN_ARGS, | ||
| *managed_plugin_args(testmon=testmon), |
There was a problem hiding this comment.
When devtools verify --all is run after a refactor changes which source files an unchanged test executes, omitting pytest-testmon here leaves .cache/testmon/testmondata with the pre-refactor dependency edges despite the complete run passing. A later affected verification after editing a newly used source file can then deselect that test and report a false green; this stale primary graph is also copied into PR lanes by .github/workflows/verify.yml. Keep the complete run in no-selection tracing mode (or otherwise refresh the same dependency graph) rather than unloading the plugin.
AGENTS.md reference: AGENTS.md:L162-L165
Useful? React with 👍 / 👎.
7db1c1e to
78db14d
Compare
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
## Summary
`devtools verify --all` traces again, so a complete corpus run leaves a
usable testmon datafile instead of an empty directory.
## Problem
Today's corpus run in `/realm/project/polylogue` (18:47-19:45, 20,600
tests, 58 minutes) left `.cache/testmon/` present and empty.
`_pytest_steps` computed `testmon = selection not in {"all",
"descriptor"}`, so the `all` tier ran with `-p no:pytest-testmon` and no
`--testmon` flags at all. A pytest session without the testmon plugin
writes no fingerprints, so the run left the datafile exactly as it found
it — and the provision step had already discarded the unusable seed at
run start, leaving nothing behind. Every subsequent `devtools verify`
then had to reseed by running everything.
This contradicts the documented contract in `CLAUDE.md`: "`--all` runs
every test and still updates fingerprints."
The tracing was removed by #4650 on the premise that "a complete-corpus
run already executes every collected test, so loading testmon adds
dependency tracing state without changing the selected corpus". True of
that run's own selection, false of the graph it was the only thing
producing. #4650's other half — a two-worker ceiling, the actual memory
remedy — is no longer present (`CORPUS_MAX_WORKERS = 8`), so only the
removal survived.
## Mechanism found
Established by experiment, not inference. Running the real verifier argv
against `tests/unit/devtools` (739 tests) with testmon forced on:
- `-n 2 --testmon-forceselect`: `SELECT count(*) FROM test_execution` =
**739**, 1 environment row (`polylogue`).
- `-n 8 --testmon-noselect`: **739** rows, 1 environment row — with 12
tests failing in the session.
So xdist width, `--testmon-noselect`, and a `FAILED` exit path all
record correctly. Under xdist the controller saves incrementally from
`pytest_runtest_logreport` on each teardown report, not once at session
end. `sync_db_fs_tests(retain=...)` only inserts; it never prunes rows
for uncollected tests, so a partial run cannot truncate the graph
either.
The absence had one cause: the plugin was not loaded.
## Solution
`_pytest_steps` traces for every tier whose collection is a corpus.
`all` passes `--testmon-noselect` (execute the whole collection, record
what it traced); `affected` keeps `--testmon-forceselect`. `descriptor`
still opts out — it collects a contract slice, so its fingerprints would
describe a collection no later run has.
## Verification
- `devtools test tests/unit/devtools/test_verify.py -k
"traces_and_deselects or records_a_usable_testmon_graph" -n 0`: `2
passed`.
- Anti-vacuity, reverting `testmon = selection != "descriptor"` to the
shipped expression: `2 failed` — `assert 'pytest-testmon' in command`,
and the executing test's session reports `2 passed` while `assert
datafile.exists()` is False. That is this defect reproduced in
miniature.
- `devtools test tests/unit/devtools/test_verify.py
tests/unit/devtools/test_run_tests.py -n 2`: `8 failed, 65 passed`. The
same 8 fail on `master` with the change stashed (`8 failed, 63 passed`)
— seven are the `build_pytest_cmd` worker-cap tests reacting to the
agent cgroup this lane runs in, one is a pre-existing descriptor red.
The two added tests are the entire delta.
- `devtools verify --quick`: all 13 gates ok, including
`testmon-selection`.
## Residual risk
An earlier lane datafile holding only 53 rows is not explained by this
change and was not reproduced here; the experiments above rule out
xdist, `--testmon-noselect`, and failing exits as its cause, leaving an
interrupted session as the likeliest remainder. The corpus was not
re-run end to end in this lane — that the `all` tier now records is
proven at 739 tests, not at 20,600. #4650's memory observation stands on
its own: tracing at eight workers has a cost, and the two-worker ceiling
it added is already gone from `master` independently of this change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01DNGJ3awfNrsLaMdHgQZvid
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Summary
Remove redundant pytest-testmon state from deliberate complete-corpus verification while retaining testmon for affected selection. Bound all managed verification to two pytest workers.
Problem
A complete-corpus run already executes every collected test, so loading testmon adds dependency tracing state without changing the selected corpus. The previous eight-worker run consumed about 9 GB, made the desktop lag, and caused systemd-oomd to kill three agent scopes. SQLite archive construction makes this suite IO-bound.
Solution
Add a shared managed-plugin profile. Affected runs retain testmon; complete runs omit its plugin and flags. Set the descriptor and verifier ceiling to two workers, independent of host CPU count and instantaneous free RAM.
Verification
devtools test tests/unit/devtools/test_verify.py tests/unit/devtools/test_dev_loop_service.py -n 0: 55 passed.devtools gate testmon-selection: selected 4 of 100; workers=2.devtools verify --quickpassed all static gates before publication.Residual risk
The testmon-removal PSS threshold still needs comparable complete-run receipts. A managed affected run and the next corpus run must confirm desktop latency and wall time at the new width.