Repository navigation
Conversation
…ges on CPU The dev journey answers every app question from stand-ins and STATUS.md records every real stage behind the stubs as unverified. Pin the first four real stages to the committed mini-arkit-lidar packet: contexts built the way the coordinator builds them, artifacts published and hydrated the way runner._publish and graph.STAGE_INPUTS do it, stills promoted into a local storage backend. Poses ships the ARKit-guided fixed-pose triangulation (all six frames registered, pose_source recorded), scale carries a source and rel_bound_95, both measured pinned to two cores against the 2 CPU / 8 GB budget. The 64x48 mini-arkit packet runs the same chain and pins its loud refusal: reads disabled, scale unresolved (meter_model_unread).
|
@codex review |
|
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 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: f905f1a378
ℹ️ 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".
| os.environ.setdefault("OMP_NUM_THREADS", "2") | ||
| os.environ.setdefault("OPENBLAS_NUM_THREADS", "2") | ||
| os.environ.setdefault("MKL_NUM_THREADS", "2") | ||
| os.environ.setdefault("HSS_POSES_THREADS", "2") |
There was a problem hiding this comment.
When pytest collects this module, these setdefault calls mutate the process-wide environment for every test that runs afterward. If the variables were initially unset, unrelated pose and numerical tests will now run with OMP_NUM_THREADS, BLAS threads, and HSS_POSES_THREADS forced to 2, changing their performance and potentially their behavior. Apply these limits only in a fixture that restores the environment, or run the real-CPU chain in a subprocess instead of changing globals at import time.
Useful? React with 👍 / 👎.
|
|
||
| Resource budget: every measurement is taken with the process pinned to two | ||
| cores (`taskset -c 0,1`) and `OMP_NUM_THREADS=2 OPENBLAS_NUM_THREADS=2 | ||
| MKL_NUM_THREADS=2 HSS_POSES_THREADS=2`. A stage over 600 s or a process peak | ||
| RSS over 8 GiB fails the budget the target host (2 CPUs, 8 GB) imposes. | ||
|
|
||
| No weights, no network, no GPU: the VLM readers stay off | ||
| (`VISION_EXTERNAL_CALLS`), so `reads` runs only the local barcode decoder, and | ||
| `poses` triangulates over the ARKit priors with pycolmap on CPU. |
There was a problem hiding this comment.
The test only limits library thread counts; it never pins the process to two CPUs. The documented taskset -c 0,1 constraint is supplied only by the focused shell command, while ordinary make test runs this test without it, so the wall-time and RSS assertions do not exercise the stated two-CPU worker budget and can pass under a different resource profile. Pin the process in the test or make the budget check conditional on an enforced affinity.
Useful? React with 👍 / 👎.
|
|
||
| # Scale carries a source and a 95% relative bound. The LiDAR depth and the | ||
| # meter tap both feed it; `source` names what shipped. | ||
| scale = json.loads((tmp_path / "scale" / "scale" / "scale.json").read_text()) |
There was a problem hiding this comment.
The test description and commit claim that this LiDAR fixture ships an ARKit-only scale, but this assertion accepts any non-none source. A regression that incorrectly certifies the result from LiDAR, a meter read, or another unsupported source would still pass, so the test does not pin the behavior it says to protect. Assert the expected source (and the corresponding refusal/source combination in the no-depth chain) explicitly.
Useful? React with 👍 / 👎.
|
@greptileai review this at plus |
 [Low impact] Adds a test file for CPU pipeline stages. The PR appears safe to merge, with non-blocking gaps in its resource measurements and LiDAR scale assertions.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| os.environ.setdefault("OMP_NUM_THREADS", "2") | ||
| os.environ.setdefault("OPENBLAS_NUM_THREADS", "2") | ||
| os.environ.setdefault("MKL_NUM_THREADS", "2") | ||
| os.environ.setdefault("HSS_POSES_THREADS", "2") |
There was a problem hiding this comment.
make test runs pytest without restricting it to two cores. These setdefault calls also preserve any higher thread counts already in the environment. A passing test therefore does not establish that the stages fit the two-core worker's time budget.
Run the measured chain in a fresh process with explicit thread settings and two-core affinity, or check those conditions before accepting the measurements.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/tests/test_real_cpu_upstream.py
Line: 37-40
Comment:
**CPU budget is not enforced**
`make test` runs pytest without restricting it to two cores. These `setdefault` calls also preserve any higher thread counts already in the environment. A passing test therefore does not establish that the stages fit the two-core worker's time budget.
Run the measured chain in a fresh process with explicit thread settings and two-core affinity, or check those conditions before accepting the measurements.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| stored = _publish(_ctx(stage, work_dir, packet_dir, storage, capture_id, run_id, branch, tier), result) | ||
| report[stage] = { | ||
| "wall_s": round(wall_s, 2), | ||
| "vm_hwm_mb": round(_vm_hwm_mb(), 1), |
There was a problem hiding this comment.
VmHWM includes the whole pytest process, including tests that ran before this chain. If an earlier test reaches 8 GiB, these budget checks fail even when the new stages stay below that limit. That makes failures depend on unrelated tests and points developers at the wrong work.
Measure each chain in a fresh child process so its memory peak belongs to the work being tested.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/tests/test_real_cpu_upstream.py
Line: 170
Comment:
**Other tests inflate memory readings**
`VmHWM` includes the whole pytest process, including tests that ran before this chain. If an earlier test reaches 8 GiB, these budget checks fail even when the new stages stay below that limit. That makes failures depend on unrelated tests and points developers at the wrong work.
Measure each chain in a fresh child process so its memory peak belongs to the work being tested.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| assert scale["source"] | ||
| assert scale["source"] != "none" | ||
| assert isinstance(scale["rel_bound_95"], float) and scale["rel_bound_95"] >= 0.0 |
There was a problem hiding this comment.
These assertions accept any nonempty source, a status of verified, and a zero rel_bound_95. They also pass if the LiDAR attempt disappears entirely. This fixture is expected to produce ARKit-only, unverified scale, so the test would miss a false scale claim.
Assert the expected source and status, plus the unused LiDAR estimate with reason equal to lidar_too_few_pairs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/tests/test_real_cpu_upstream.py
Line: 232-234
Comment:
**False scale claims pass**
These assertions accept any nonempty `source`, a `status` of `verified`, and a zero `rel_bound_95`. They also pass if the LiDAR attempt disappears entirely. This fixture is expected to produce ARKit-only, unverified scale, so the test would miss a false scale claim.
Assert the expected `source` and `status`, plus the unused LiDAR estimate with `reason` equal to `lidar_too_few_pairs`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.This branch has not been deployed
1 participant
fleet-packet: burst14-bs-real-poses-scale
Human author: Sam Gu (sgu07966@gmail.com)
What changed
Adds
backend/tests/test_real_cpu_upstream.py. It runs the first four processing stages for real, on CPU, against the committed app-format captureios-kit/fixtures/mini-arkit-lidar, and pins the loud refusal a capture without depth gets instead of a scale claim. No production code changed.The test builds the same
StageContextobjects the coordinator builds (ctx.gpu = None, external vision off), publishes each stage's artifacts with the runner's own_publishhelper, and hydrates each next stage fromgraph.STAGE_INPUTS, so the chain is the orchestrator's chain, not a re-creation of it. Two assertions hold the whole thing to the same budget as the smallest CPU worker: every stage finishes under 600 seconds and the process peak RSS stays under 8 GiB, measured with the test pinned to two cores.Per-stage results on
mini-arkit-lidar, six 1920x1440 frames with ARKit poses and LiDAR depth:few_images,synthetic_sourcefallback_arkit_fixed: all 6 frames registered over the ARKit priors,pose_sourcerecorded per camerapartialwith no stills in the packet, cost $0arkit,rel_bound_950.035, statusunverifiedThe second chain runs the same four stages on
data/fixtures/packets/mini-arkit(64x48 frames, no depth) and asserts the refusal shape: reads publishesstatus: disabled, and scale refuses withscale_unresolved/meter_model_unreadand weak confidence instead of certifying a metric scale.Why
backend/docs/STATUS.mdrecords every real stage behind the journey stubs as unverified, and the dev journey answers every app question from stand-ins. Until now nothing proved that a real packet walks validate, poses, reads and scale on the CPU path. This test is that proof, kept executable: it fails if the guided-pose fallback stops registering frames, if scale starts claiming a source it cannot resolve, or if a stage blows the 2 CPU / 8 GB budget.Two things I expected to need fixing turned out to work as designed, so I changed nothing:
barcode.readdegrades to an empty result under pytest whenzxing-cppis absent (the defaultmake testenvironment), and scale records the LiDAR branch's own refusal (lidar_too_few_pairs, 0 pairs on this six-frame fixture) as an estimate while shipping the ARKit-only source and asking for a meter close-up.Verification
Focused, in both environments:
Full suite, once, on this head:
Four of the failures are the pre-existing SD-96 gates on
alpha(server/eval/tests/test_gates.py, same names as on the base), and the 14 collection errors are the pre-existing cold-restart fixtures, both unchanged by this PR. The fifth,server/orchestrator/tests/test_run.py::test_hard_stop_cancels_worker, is a timing-sensitive worker-cancellation test that failed only under the suite's sustained ~90% CPU contention: it passes in isolation (0.55 s) and passes immediately after the new tests (18 passed), so no test here leaks processes or threads into it. The new tests pass inside the suite.Ruff check and format clean on the changed file.
Coverage limits, next to the claims they weaken: the fixtures are mini packets (six frames, one reference tap), so the LiDAR-vs-sparse alignment finds no usable pairs and scale ships ARKit-only, unverified. The VLM readers are off by design, so reads exercises the local barcode path only. No GPU, no model weights, no network.
Risk
The test adds about 12 seconds to
make testand depends only on committed fixtures. Its budgets are wide enough for slower runners, but a much slower runner could trip the 600 s wall budget; that failure would be a signal, not a flake.Checklist
🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact
Opened by the maintainer from the Obvious thread's own description, so every review bot reviews it (CodeRabbit skips PRs authored by bots). The work and its commits are by Obvious.
Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>