Repository navigation
Conversation
…d env alone Fails on the parent: _peak_rss_mb (the old _vm_hwm_mb, renamed only) reads /proc/self/status and ignores ru_maxrss, and importing the module sets the four thread variables for every later test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…axrss _peak_rss_mb reads resource.getrusage(RUSAGE_SELF).ru_maxrss, KiB on Linux and bytes on macOS, instead of /proc/self/status, which macOS lacks. Each chain now runs through tests/realcpu_launcher.py: pinned to two CPUs, the four thread variables set to 2 in that process only, and forked from a bare interpreter so its peak RSS is its own. A subprocess child inherits the parent's lifetime peak in ru_maxrss on Linux (vfork + execve); measured on WSL2 6.18 as 626 MB in a child whose VmHWM was 11 MB. The module no longer sets thread variables at import. The wall budget is asserted only when the chain actually ran on two CPUs or fewer, and skipped loudly on macOS. The scale assertions now pin the shipped claim: source arkit, status unverified, the refusal reason, ARKit poses as the only used estimate, a positive bound, and on the LiDAR fixture the unused LiDAR estimate with lidar_too_few_pairs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
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. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
Co-Authored-By: Claude Code <noreply@anthropic.com>
Zephyrus task Z1. Fixes #298's peak-RSS reading on macOS and its open review threads on the CPU budget, memory isolation and scale assertions.
What was wrong
_vm_hwm_mb()(backend/tests/test_real_cpu_upstream.py:72on [obvious-fleet] Backend: the first four real stages on CPU on a real app-format capture #298) read/proc/self/statuswith no fallback, so on macOS every test in the file failed withFileNotFoundError.OMP_NUM_THREADS,OPENBLAS_NUM_THREADS,MKL_NUM_THREADSandHSS_POSES_THREADSwithsetdefaultat import, which changed them for every later test and kept any higher value already set. Nothing pinned the process to two CPUs unless the caller usedtaskset, somake testnever measured the 2-CPU budget.nonesource, averifiedstatus and a zero bound, and passed if the LiDAR attempt disappeared.What changed
_peak_rss_mb()(backend/tests/test_real_cpu_upstream.py:79) readsresource.getrusage(RUSAGE_SELF).ru_maxrss, dividing by 1024² on macOS (bytes) and by 1024 elsewhere (KiB).backend/tests/realcpu_launcher.py, which pins to the first two allowed CPUs, gets the four thread variables set to 2 in its environment only, and forks once before importing the module under test. The fork is there because asubprocesschild on Linux reports its parent's lifetime peak as its ownru_maxrss(CPython starts it with vfork, and execve folds the parent's high-water mark into the child's counter). On WSL2, kernel 6.18, a child whose VmHWM was 11 MB reported 626 MB after its parent had touched 600 MB. A freshfork()from a bare interpreter starts a clean counter._assert_budget(:267) checks the thread settings and the RSS budget, then asserts the 600 s wall budget only when the chain ran on one or two CPUs. On macOS, which has nosched_setaffinity, it skips with a reason after the correctness checks pass, instead of claiming the budget.arkit, statusunverified,scale_unresolved/meter_closeup_missing, confidencenear_only, a positiverel_bound_95, ARKit poses as the only used estimate, and the unused LiDAR estimate withlidar_too_few_pairsand 0 pairs (:331-341). The no-depth chain also asserts sourcearkit, a positive bound, no LiDAR estimate, and ARKit poses as the only used estimate (:366-374).Tests
New tests in
backend/tests/test_real_cpu_upstream.py:test_peak_rss_mb_reads_bytes_on_darwinsetssys.platformtodarwinandresource.getrusageto return 2×1024×1024, and expects 2.0 MB.test_peak_rss_mb_reads_kib_on_linuxexpects 2048.0 MB from the same number.test_peak_rss_mb_matches_vmhwm_in_the_chain_process(Linux only) raises pytest's own peak by 256 MiB, then runs a probe through the launcher that touches 64 MiB and compares_peak_rss_mb()with VmHWM, within 1 MB.test_importing_this_module_leaves_thread_env_aloneimports the module in a clean interpreter and checks that none of the four thread variables got set.Failing first. Commit a43cc0e adds the tests against #298's code, with
_vm_hwm_mbrenamed to_peak_rss_mband its body unchanged:The VmHWM test can't fail on #298 because #298 has no launcher. As a check of what it guards, I removed the launcher's fork locally: it failed with
479.9 == 263.8 ± 1, the pytest peak leaking into the child. I restored the fork before committing.The stricter scale assertions pass on #298 too, because the stages already produce the right claim. They don't fail first; they catch a regression that would claim a scale the fixture can't support.
After the fix:
Full suite, run once on this head (
uv sync --locked && OMP_NUM_THREADS=1 uv run pytest -q; pytest-xdist isn't in the lock, so no-n 2):The machine was shared and busy, so I ran the suite once, on this head only, and didn't rerun #298. Every failure and error is outside the changed file. Compared with #298's reported run (
make test: 5 failed, 1507 passed, 19 skipped, 14 errors):server/eval/tests/test_gates.pyfailures, and the 14tests/test_capture_cold_restart.pyerrors (ImportError: cannot import name 'load_receipt').server/packet/tests/test_round2.py::test_many_missing_files_stay_linearfailed on time (63.8 s against a 60 s limit). It passes alone.tests/test_capture_real_downstream.pyhad 7 setup errors, all a finalize drainTimeoutError(server/api/deps.py:112). [obvious-fleet] Backend: the first four real stages on CPU on a real app-format capture #298's import-timesetdefaultused to cap the BLAS and pose threads at 2 for every later test, and this PR removes that cap. So I ran the module alone without the cap, at a load average of 16 to 22 on 16 cores: 12 passed in 150 s. The timeouts came from contention in the full run, not from the removed cap.uv sync --lockedwithout--extra api, as the task rules ask.uv run ruff check .passes.uv run ruff format --check .flags one file,server/orchestrator/tests/test_greptile_followup.py, which is unchanged from #298 and outside this diff.Not verified
os.fork()and the wall-budget skip have not been run there.🤖 Generated with Claude Code
Review fix (Oct 23)
The launcher now polls
waitpid(..., WNOHANG)against a 900-second deadline, then sends SIGTERM, waits one second, sends SIGKILL if needed, reaps the child, and raises a timeout naming the stage. Normal child exit codes still pass through unchanged. The twosubprocess.runcalls now use timeouts of 900 seconds for real chains and 30 seconds for the import probe. These values are chosen well above the expected runtime; they were not measured.The new launcher test runs a child that sleeps past a 50 ms limit, checks that the call returns within two seconds, and verifies that the child is gone. It also checks normal exit-status preservation.
Against
9237a5b8, the old launcher reproduced the hang undertimeout 3swith exit 124. After the fix,OMP_NUM_THREADS=1 uv run pytest -q tests/test_real_cpu_launcher.pypasses 2 tests, andOMP_NUM_THREADS=1 uv run pytest -q tests/test_real_cpu_upstream.pypasses all 6 tests.