close five review findings from PR #363, and measure the sixth away by oshaughnessy-junior · Pull Request #365 · oshaughnessy-junior/research-projects-RIT

oshaughnessy-junior · 2026-09-17T20:21:01Z

Six non-blocking findings from the adversarial review of #363 (merged as 27a0e4cda). None was live. Five are real and fixed; one is not a defect. Then, on request, the gates were taken to a real GPU, which turned up two more.

	finding	action
F1	defaults reader returned on the first `ast.Dict`, so a second construction site was never read	pin the site count exactly; check every site's dict
F2	the cast comment claims two jobs one line cannot do	not a defect, see below
F3	`.replace(" ", "")` normalizes one side only; `k.value` raises `AttributeError`	normalize both sides via `ast.unparse`; named assertion on the key
F4	docstring claims more than the test proves	reword to a partition of tuples; assert calls/drops disjoint
F5	a `--lane` typo built a fixture and leaked a directory on `/`	move the refusal above `mkdtemp`
F6	`LANES.index` is a first-match lookup	`enumerate`; fix the field-count comment

F2 is not a defect

cupy.asarray rejects object dtype only when no dtype is given. The line passes one, so it does land on the device and does take object draws. The rewrite the review suggested, xpy.asarray(np.asarray(x, dtype=float)), raises TypeError on any cupy input and would break the path the line exists for. Section 4 of the stand-in gate now runs that against real cupy instead of arguing it.

The device path

The e2e gate pinned CUDA_VISIBLE_DEVICES="" and so said nothing about the path production takes by default on a GPU node: RIFT binds its array module at import from whether cupy imports, not from --gpu. Section 5 runs the prior-only and closed-form answers with a device visible and asserts the child reached one, via a marker the driver prints only after cupy.array(5) succeeds.

The stated blocker did not reproduce. "CUDA_VISIBLE_DEVICES="" is required, the scalar AV path raises Unsupported dtype float128" is true where it was written, in test_psi_marginalization.py, and false here: under --zero-likelihood the stand-in builds lnL with xpy.zeros, float64 on device, so the scalar likelihood that would have built a RiftFloat never runs.

What it covers: the sampler on device, the stand-in's base array, the supplementary factor. Not the GPU signal likelihood, because --zero-likelihood replaces likelihood_function outright. Measured rather than assumed: AV with and without --vectorized --gpu --force-xpy returns bit-identical lnZ, and that inertness is its own test.

Two pre-existing GPU defects, now recorded instead of hidden

Both reproduce with no supplementary factor, both are TypeError: Unsupported type <class 'numpy.ndarray'> from a cupy ufunc, and both are invisible in production because the driver prints FAILED ANALYSIS and exits 0.

sampler	site
portfolio	`RIFT/likelihood/vectorized_general_tools.py` `histogram()`: `xpy.maximum` gets a device `blank_array` and numpy `samples`
adaptive_cartesian	`RIFT/integrators/mcsampler.py`: `fval*joint_p_prior/joint_p_s`, device integrand against numpy priors

They are pinned lanes, by site and exception, not skips — skipping is what hid them. The test reddens if one starts working and if the failure moves. Fixing them is separate work and is not in this PR.

The GMM device lane is A=0.75 B=3, not A=8 B=2. This file already records GMM at A=8 with the inclination term as an n_eff lottery, 2 of 8 seeds collapsing to sigma 0.028 and 0.073, over MAX_SIGMA, which is why no CPU lane runs it. Eight clean GPU seeds do not refute a lottery; the sweep that found it was also eight seeds.

Verification

Every guard was mutation-checked against the mutation it is for, with the pre-change file as the control arm.

On GPU (ldas-pcdev2 slot 0, A100 cc 8.0; cupy 12.0.0; same HEAD and md5s as the local tree):

arm	result
control, stand-in gate	27 passed
the suggested `xpy.asarray(np.asarray(x, dtype=float))`	3 failed
`dtype=float` dropped from either cast	3 failed
a factor that ignores `xpy` and returns numpy	3 failed
control, device lanes	6 passed
device lanes silently falling back to the host	6 failed
a recorded known-failing sampler starts passing	1 failed
a recorded failure moves to another site	1 failed

On CPU (ldas-grid, cupy absent): F1 two-site mutation red, control green. F3 multi-token value green where control was spuriously red; **spread and computed keys now named assertions rather than AttributeError. F4 a calling definition whose tuple is already recorded as dropping goes red where the full 23-test control suite was green. F5 control leaks a directory, fixed leaks none. F6 control gives two rows cal_3_1000, fixed gives cal_3_1000 and cal_4_1000.

Gates

gate	ldas-grid (no cupy)	ldas-pcdev2 (A100)	ldas-pcdev13 (2080 Ti)
`test_zero_likelihood_standin.py`	24 passed, 3 skipped	27 passed	27 passed
`test_e2e_analytic_pipeline.py`	17 passed, 7 skipped	24 passed
`.travis/test-ci-roster.py`	PASS	PASS

_ZLSTANDIN_EXPECTED 23 → 27, _E2E_EXPECTED 17 → 24. Skipped tests are still collected, so both pins are host-independent, and a green CI run has not exercised the device path. Device calibration is 8 seeds on the A100, recorded in the table beside its host twins; worst |z| across the gate moves 2.15 → 2.30, so Z_TOLERANCE's margin is 2.2x rather than 2.3x. No constant changed.

🤖 Generated with Claude Code

F1 The defaults reader returned on the FIRST ast.Dict it walked into, so a second construction site with wrong defaults was never looked at. Pin the construction-site count exactly, and check every site's dict. F3 Normalize both sides of that comparison through ast.unparse instead of deleting every space from one side, and refuse an unresolvable dict key with a named assertion rather than an AttributeError on k.value. F4 The drop-the-factor test pins a partition of six parameter TUPLES, not of the eight definitions; say so, since the claim belongs to it and _EXPECTED_SIGNATURES together. Also assert the calling and dropping sets are disjoint. F5 make_e2e_calibration: refuse a bad --lane before mkdtemp and build_event, which cost a fixture build and leaked a directory on /. F6 Tag lanes by enumerate rather than LANES.index, a first-match lookup; correct the field-count comment above LANES. F2 is NOT a defect. xpy.asarray(x, dtype=float) does land on the device AND take object dtype: cupy refuses object dtype only when no dtype is passed, and the reviewer's suggested xpy.asarray(np.asarray(x, dtype=float)) would raise on every cupy input. Measured on cupy 12.0.0 on two hosts and recorded on the line; pinned by a new 5 s test on the dtype keyword. Each new guard was mutation-checked against the mutation it exists for, with the pre-change file as the control arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The F2 conclusion rested on a probe I ran by hand, and the test I shipped for it covered only the numpy path; its own docstring said the device half was "an argument in the comment, not a test". That is the read-and-trust shape. Fixed: New section 4 runs the shipped example and the driver's own stand-in factory against REAL cupy -- both input kinds (device float64, host object dtype), both lnL conventions, a fully-sampled signature and one falling back to a scalar default -- and checks the result is ON the device and equals the numpy arm. _FakeXpy stays: it runs everywhere, and a fake can be more permissive than what it stands for, so the two cover each other. _cupy_or_skip separates the two failures that look alike. `import cupy` fails on a host with no CUDA runtime; it SUCCEEDS on the CIT GPU head nodes while the visible device is one this cupy cannot build a kernel for (pcdev13 slots 0-2, all of pcdev11: Blackwell cc 12.0, cupy 12.0.0 -> "invalid value for --gpu-architecture"). So it runs a kernel rather than trusting the import, probes at dispatch because the slot map moves, and says in the skip reason that a skip is not a pass. Measured, ldas-pcdev2 slot 0 (A100, cc 8.0) and ldas-pcdev13 slot 3 (RTX 2080 Ti, cc 7.5), cupy 12.0.0, same HEAD and same md5s as the local tree: 27 passed on both. Mutation on the A100: the rewrite the comment warns against, xpy.asarray(np.asarray(x, dtype=float)), now fails 3 tests instead of being refused by an argument; dropping dtype= fails 3; a factor that ignores xpy and returns numpy fails 3. Also corrects the module docstring and the CI comment, which both said this file needs no GPU. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… that cannot The whole file pinned CUDA_VISIBLE_DEVICES="" and so said nothing about the device path, which is the path production takes by default on a GPU node: RIFT binds its array module at import from whether cupy imports, not from --gpu. Section 5 runs the prior-only and closed-form answers with a device visible and asserts the child reached one. WHAT IT COVERS: the extrinsic sampler on device, the --zero-likelihood stand-in building its base array with xpy_default=cupy, and the supplementary factor evaluating on device. NOT the GPU signal likelihood -- --zero-likelihood replaces likelihood_function outright, so NoLoop never runs. Measured, not assumed: AV with and without --vectorized --gpu --force-xpy returns bit-identical lnZ, and that inertness is now its own test. THE BLOCKER DID NOT REPRODUCE. "CUDA_VISIBLE_DEVICES='' is required, the scalar AV path raises Unsupported dtype float128" is true where it was written (test_psi_marginalization.py) and false here: under --zero-likelihood the stand-in builds lnL with xpy.zeros, float64 on device, and the scalar likelihood that would have built a RiftFloat never runs. AV on an A100 completes and lands 1.5 sigma from the closed form. TWO SAMPLERS CANNOT RUN ON A DEVICE, both pre-existing, both reproduced with no supplementary factor, both `TypeError: Unsupported type <class 'numpy.ndarray'>` from a cupy ufunc, and both invisible in production because the driver prints FAILED ANALYSIS and exits 0: portfolio RIFT/likelihood/vectorized_general_tools.py histogram(), where xpy.maximum gets a device blank_array and numpy samples adaptive_cartesian RIFT/integrators/mcsampler.py, fval*joint_p_prior/joint_p_s, device integrand against numpy priors They are RECORDED lanes, pinned by site and exception, not skips: skipping is what hid them. The test reddens if one starts working and if the failure moves. The GMM device lane is A=0.75 B=3, not A=8 B=2. This file already records that GMM at A=8 with the inclination term is an n_eff lottery -- 2 of 8 seeds collapsed to sigma 0.028 and 0.073, over MAX_SIGMA -- which is why no CPU lane runs it. Eight clean GPU seeds do not refute a lottery; the sweep that found it was also eight seeds. Calibration, 8 seeds on ldas-pcdev2 slot 0 (A100, cc 8.0), cupy 12.0.0, recorded in the table with its host twins. Worst |z| across the whole gate moves 2.15 -> 2.30, so Z_TOLERANCE's margin is 2.2x rather than 2.3x; no constant changed. Mutation-checked on the A100: a silent host fallback reddens all six device lanes; a recorded failure that starts passing reddens; a failure that moves site reddens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…rdenings REGRESSION I INTRODUCED. `make_e2e_calibration.py --seeds 8` -- the invocation two comments tell you to run, one under "RE-DERIVE THIS TABLE, do not trust it" -- exited 1 having measured nothing, because the four new device rows are selected by default and demanded --gpu-slot. No single --lane selected the 17 host rows and excluded the 4 device ones, so the committed host table could not be regenerated anywhere. Now: without --gpu-slot the device rows are dropped and NAMED ON STDERR (stdout builds the table, so a dropped row cannot slip into it), and it refuses only when that leaves nothing to measure. Verified in three directions, and neither refusal leaks a fixture directory. CLAIMS THAT NO LONGER MATCHED THE CODE, six of them, and the awkward part is that three went stale inside this branch: the commit that gave the e2e gate its own device lanes did not update the standin file's "the end-to-end gate is structurally blind to this", _FakeXpy's "the only place that can see it", or the example's "the gate cannot check it". - .travis/test-integrate.sh said these lanes skip "because the runners have no cupy". .gitlab-ci.yml's `gpu_integration` job runs THIS script with RIFT_CI_REQUIRE_GPU=1 and CUDA_VISIBLE_DEVICES=0 in a GPU container, and the script's own preflight makes a working cupy a hard requirement there. They run on that runner. Corrected, both places. - The device calibration rows were recorded as measured "at 5bb8da0", a commit where --gpu-slot does not exist. They were measured on the tree that became the commit adding them; say that, and note "slot 0" is CUDA's numbering and not nvidia-smi's (on pcdev2 nvidia-smi calls the A100 index 2). - test_the_paths_that_silently_drop_the_factor's new "the two tests TOGETHER catch a new silently-ignored configuration" is FALSE, disproved by mutation: wrapping an existing calling body's factor call in a new `if` is a new silently-ignored configuration and leaves the file fully green. What the pair catches is a new silently-ignored DEFINITION. - "today's 17 distinct rows" in a table that now has 21. - The e2e file ran sections 1, 2, 3, 5, with no 4, and two comments use the number to navigate. Renumbered. HARDENING. - SKIP GUARD on both gates, the idiom this script already applies to the tmarg gates. Skipping is now these gates' normal CPU state (3/27 and 7/24), and a count guard catches deselection, not skipping. Mutation-verified: with lal_path2cache hidden the e2e module skips ALL 24 tests and pytest exits 0 -- previously green, now a failure. - _run_ile's fourth positional silently changed meaning from a_coeff to expect_device. Nothing passes it positionally; `*` makes that permanent. - _cupy_or_skip tried only the default device while the e2e gate's gpu_slot scanned all of them, so on a host whose slot 0 is Blackwell the two files disagreed about whether a device was usable. Both scan now. - _dmarg_extra overwrote `extra` instead of extending it, unreachable today but the adjacent _needs_gpu branch extends; made them consistent. ldas-grid 41 passed 10 skipped, roster PASS; ldas-pcdev2 slot 0 (A100) 51 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…see its own blind spot The re-review confirmed all ten earlier fixes by mutation and found four more claims of exactly the class the last commit existed to sweep. Two of them I had written. - .travis/test-integrate.sh still said "the runners have no cupy" FOUR LINES ABOVE the new paragraph correcting it; the last commit said it fixed both places and there were three. - test_zero_likelihood_standin.py's device test still said the e2e gate pins CUDA_VISIBLE_DEVICES="" and the runners have no cupy, contradicting this file's own module docstring as rewritten one commit earlier. - The new standin skip guard justified itself with a case that CANNOT HAPPEN: a missing ILE executable is a collection ERROR, not a skip, because the parametrize reads the driver at import, so the module-level skipif is unreachable and the count guard already covers it. Verified by moving the executable aside. The guard stays as future-proofing; its stated reason was wrong and is now stated correctly. - "across all lanes" summarised a table whose device rows had been dropped. THE GUARD COULD NOT SEE ITS OWN BLIND SPOT. It classified skips by whether the reason names cupy/GPU/CUDA, which cannot distinguish "there is no GPU here" from "the GPU probe itself broke" -- and the second is exactly what would happen on the runner that PROMISES a device. Measured: an unimportable module inside _GPU_PROBE removed all 7 device lanes and scored zero bad. So with RIFT_CI_REQUIRE_GPU=1, ANY skip now fails: the preflight has already proved cupy works and a device computes, leaving nothing a skip can legitimately mean. Verified in both modes, including the exit path. A FIX THAT DID NOT WORK, caught by measuring it. The previous commit claimed `with cupy.cuda.Device(d):` instead of `.use()` stopped the probe leaking a CUDA context on every device it rejects. It does not: `with` restores the current device and destroys nothing, and both shapes leave two contexts on one pid (252 and 446 MB) for a two-device visible set. _cupy_or_skip now probes in a SUBPROCESS, as the e2e gate's gpu_slot already did, and touches only the winner in-process: measured 0 abandoned contexts instead of 2. Confirmed end to end with CUDA_VISIBLE_DEVICES=1,0 on ldas-pcdev2, the unusable card listed first -- 27 passed, correct re-index. Also: _invoke_ile was still positional past sampler_args while _run_ile was not, so the hardening was half applied; the guards now tee rather than capture, so a 4-minute gate is not silent on a runner; and removed a stray `python` symlink I left in the worktree root. ldas-grid 41 passed 10 skipped, counts 27/24 match, roster PASS; ldas-pcdev2 slot 0 (A100) 51 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

BLOCKING, AND MINE. `tee /dev/stderr` opens /proc/self/fd/2 with O_TRUNC, so when stderr is a regular file -- `bash .travis/test-integrate.sh > gate.log 2>&1`, which is how anyone runs a long gate, and precisely what the "do not look like a hang" rationale invites -- it truncated the log to zero and wrote NULs into the hole while the shell's fd 2 kept its offset. Every earlier gate's output, gone, exit status still 0. It fires twice, so the second guard destroyed the first one's record. With stderr closed it went on to overwrite the running script. Reproduced: old form 79 bytes with 12 NULs and both marker lines lost; `tee >(cat >&2)` 107 bytes, 0 NULs, capture intact, exit 1 still propagates under pipefail. CI itself was probably safe (a pipe, not a file), which is exactly why this would have sat there. THE RULE NOW LIVES WITH THE TESTS, not only in the shell. RIFT_CI_REQUIRE_GPU=1 made any skip fatal in test-integrate.sh, but `pytest <file>` on the GPU runner -- what you run to reproduce a CI failure -- still reported green with every device lane skipped. Both files gained _no_gpu(), which fails instead of skipping when the environment promised a device. Measured on a CPU host with RIFT_CI_REQUIRE_GPU=1: 3 failures in the standin gate, 7 errors in the e2e gate (errors, not failures, because gpu_slot is a fixture -- red either way, and noted on the line). On ldas-pcdev2 with a real device it stays green: 27 passed. Also from the review: a probe TIMEOUT is now cached, so a hung probe costs one 600 s wait rather than one per test (verified: 3 calls, 1 subprocess); the NOVERDICT fallback is flattened, in a file whose probe exists because multi-line messages ruin a verdict line; the "error not skip after a good verdict" asymmetry is stated as deliberate; the skip-count message says it counts grouped SKIPPED lines rather than tests; and the streaming comment sits on the gate that is actually slow. The review confirmed the re-indexing question I flagged: `_cupy_or_skip` using the probe's index in-process and `gpu_slot` mapping it back through CUDA_VISIBLE_DEVICES for a child are BOTH right, and each would be wrong in the other's place. It also reproduced the context measurement to the MiB: 2 contexts before, 1 now. ldas-grid 41 passed 10 skipped, counts 27/24, roster PASS. ldas-pcdev2: 27 passed at CVD=0 and at CVD=1,0 (unusable card first), e2e 24 passed, and 27 passed under RIFT_CI_REQUIRE_GPU=1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…tion #365 landed with three review-response commits this branch was not built on, all in the same files: sections renumbered (5 -> 4), _run_ile/_invoke_ile made keyword-only past sampler_args, a _no_gpu() that FAILS instead of skipping under RIFT_CI_REQUIRE_GPU=1, skip guards in test-integrate.sh, and the device rows made droppable in the calibration generator. Resolved by taking rift_O4d's version of all four conflicting files outright and re-applying the promotion on top of it, rather than merging hunk by hunk. Re-applied against the new text: _GPU_SAMPLERS gains portfolio and adaptive_cartesian, _GPU_LANE_COEFFS/_GPU_LANE_KW, _AC/_AC_N_MAX named once and reused by the CPU lane and the generator, _GPU_KNOWN_FAILING and its test deleted, the four new calibration rows, and the device-lane count 7 -> 9 in _no_gpu's docstring and in both skip-guard rationales. _E2E_EXPECTED 24 -> 26. Two of their corrections change what this branch has to say. The device lanes are NOT CPU-only: .gitlab-ci.yml's gpu_integration job runs test-integrate.sh with RIFT_CI_REQUIRE_GPU=1 and CUDA_VISIBLE_DEVICES=0, so the four lanes added here will run in CI on that runner and any skip there is a failure. And they had already recorded that "slot 0" is CUDA's ordering rather than nvidia-smi's on ldas-pcdev2, so the duplicate note this branch carried is dropped in favour of theirs. _invoke_ile's docstring justified itself by the known-failing lanes that are now gone; it says what it is instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

oshaughnessy-junior deployed to private-review-dispatch-rift September 17, 2026 20:21 — with GitHub Actions Active

oshaughnessy-junior deployed to private-review-dispatch-rift September 17, 2026 20:27 — with GitHub Actions Active

oshaughnessy-junior deployed to private-review-dispatch-rift September 17, 2026 23:43 — with GitHub Actions Active

oshaughnessy-junior and others added 3 commits September 18, 2026 05:10

oshaughnessy-junior force-pushed the claude/pr363-followups branch from 8ded3f0 to 7fd6494 Compare September 18, 2026 12:11

oshaughnessy-junior deployed to private-review-dispatch-rift September 18, 2026 12:11 — with GitHub Actions Active

oshaughnessy-junior deployed to private-review-dispatch-rift September 18, 2026 12:37 — with GitHub Actions Active

oshaughnessy-junior mentioned this pull request Sep 18, 2026

portfolio and adaptive_cartesian could not run with a GPU visible #368

Merged

oshaughnessy-junior deployed to private-review-dispatch-rift September 18, 2026 12:57 — with GitHub Actions Active

oshaughnessy-junior deployed to private-review-dispatch-rift September 18, 2026 13:26 — with GitHub Actions Active

oshaughnessy-junior marked this pull request as ready for review September 18, 2026 14:02

oshaughnessy-junior had a problem deploying to private-review-dispatch-rift September 18, 2026 14:02 — with GitHub Actions Error

oshaughnessy-junior merged commit 36e849a into rift_O4d Sep 18, 2026
33 of 34 checks passed

oshaughnessy-junior deployed to private-review-dispatch-rift September 18, 2026 14:03 — with GitHub Actions Active

Navigation Menu

close five review findings from PR #363, and measure the sixth away - #365

close five review findings from PR #363, and measure the sixth away#365
oshaughnessy-junior merged 6 commits into
rift_O4dfrom
claude/pr363-followups

oshaughnessy-junior commented Sep 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

oshaughnessy-junior commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

F2 is not a defect

The device path

Two pre-existing GPU defects, now recorded instead of hidden

Verification

Gates

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

oshaughnessy-junior commented Sep 17, 2026 •

edited

Loading