check: report every unusable fixture header at once and ask it in the fmt job, plus two residual bridges that declared a pointer parameter - #1500
Conversation
… any of them `run_synthetic_suite` parsed each fixture's directives as it reached it and exited on the first `ValueError`, and `run_synthetic_bench` parsed the rest inside the bench run. A change that reds N fixtures reported one name per run. - `synth_fixture_headers` reads every directive a fixture owes, with the rule for which readers apply to which kind in one place rather than at two callers. - `run_synthetic_suite` parses every matched fixture first and collects the failures; `report_unusable_headers` prints them grouped by error, so a reader's guidance appears once per kind instead of once per fixture. - `run_synthetic_bench` takes the parsed headers instead of re-reading them. - `ungated-jitstats=` and `jitstats-band=` are read in that pass too. They are read per backend inside `_apply_snapshot_gate`, which catches nothing, so an unusable value in either arrived as a traceback out of a backend run. No fixture runs when any header is unusable; previously the ones before the first offender ran first. Assisted-by: Claude
The header pass reads files and builds nothing -- measured at 0.2s over 479 fixtures -- so it does not have to wait behind a backend build to answer. `--check-headers` runs that pass and exits. `parse_args` answers the flag ahead of the backend resolution, which probes the toolchain for the wasm32 target. The step goes in `cargo-fmt`, the job every expensive job in this workflow already hangs off through `needs: cargo-fmt`, so a fixture whose header cannot be read reds a run in seconds rather than after the build. `synthetic_bench_paths` and `read_synthetic_headers` are the halves `run_synthetic_suite` and the new mode share. Assisted-by: Claude
…as i64 `bh_w_exception_get_kind` and `bh_exception_object_matches_stop_iteration` declared their parameter as `pyre_object::PyObjectRef`, which is `*mut PyObject` -- four bytes on wasm32 and eight on the native targets. A residual call supplies the integer arg slot, and every other `bh_*` bridge in this file takes `i64` and casts; these two now do the same. Dropping the pointer parameter drops the `#[allow(improper_ctypes_definitions)]` it needed. Assisted-by: Claude
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
![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: 9aaa484dc5
ℹ️ 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".
| # reads files and builds nothing (0.2 s over 479 fixtures), while the | ||
| # synthetic suite cannot answer until the backend build it sits behind | ||
| # has finished. | ||
| run: python3 pyre/check.py --check-headers |
There was a problem hiding this comment.
In the inspected cargo-fmt job, no actions/setup-python step installs CPython 3.14 before this command, so the Ubuntu 24.04 system python3 (3.12) exits while importing check.py: PYTHON3 = _resolve_python3() runs before parse_args() can handle --check-headers and rejects every non-3.14 interpreter. This makes cargo-fmt fail even for valid headers and, because the expensive jobs declare needs: cargo-fmt, blocks the rest of this workflow; set up 3.14 here or make the header-only path avoid oracle initialization.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 0899ec8). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
`PYTHON3 = _resolve_python3()` ran at import and `SystemExit`s when no CPython 3.14 is on PATH, so `--check-headers` could not run on the `cargo fmt --check` job -- which carries rustfmt and a 3.12 `python3` and has no reason to carry an oracle. `PYRE_STDLIB` is deferred with it, because its out-of-tree fallback spawns that interpreter. `python3()` and `pyre_stdlib()` resolve once and remember. A run that measures anything still refuses without CPython 3.14, one step later than before. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0899ec8657
ℹ️ 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".
| synth_ungated_jitstats(path) | ||
| synth_jitstats_bands(path) |
There was a problem hiding this comment.
When a non-selfcheck fixture has a malformed # pyre-check: max-wasm-ratio= value (for example, nan), this new prepass reports success because synth_fixture_headers never calls wasm_ratio_gate. That directive is first parsed only from the wasm performance path after the backend build and fixture execution, so the cheap cargo-fmt gate misses exactly this class of unusable header and the expensive wasm job can still fail later; validate it here alongside the other per-backend directives.
AGENTS.md reference: AGENTS.md:L76-L87
Useful? React with 👍 / 👎.
| if _PYTHON3 is None: | ||
| _PYTHON3 = _resolve_python3() |
There was a problem hiding this comment.
For an ordinary invocation on a host without CPython 3.14, lazy resolution now delays the deterministic configuration error until warmup or measure_startups, but main builds every selected backend first at lines 5138–5140. Previously the import-time resolution failed before any build, whereas now the user can spend minutes building—or receive an unrelated build failure—before learning that the required oracle is absent; retain the early return for --check-headers, then force python3() before starting backend builds for all normal runs.
Useful? React with 👍 / 👎.
1 participant
check.py — every unusable fixture header in one run
run_synthetic_suiteparsed each fixture's directives as it reached it andsys.exit(1)d on the firstValueError;run_synthetic_benchparsed the restinside the bench run.
pyre/bench/synth/is shared, so a directive this scriptrequires reds every fixture that lands without one — and a run named exactly
one of them.
That happened on main this morning. #1462 made
selfcheck-compiles=required,#1479 merged two selfcheck fixtures without it about an hour later, and
pyre/check.py craneliftat1579a6a2411nameda_profiler_installed_from_a_call_event_keeps_c_events.pyand stopped, witha_raising_trace_hook_still_owes_the_leave_event.pyinvisible behind it.#1492 has since declared both — the fixture half of this work was measured
independently and reached the identical
selfcheck-compiles=hot, so it is notin this branch.
synth_fixture_headersreads every directive a fixture owes, with the rulefor which readers apply to which kind in one place instead of restated at two
callers.
run_synthetic_suiteparses every matched fixture before running any of themand collects the failures.
report_unusable_headersprints them grouped bythe error text with the path substituted out, so fixtures failing the same
way share one block and the reader's guidance is printed once rather than
once per fixture.
run_synthetic_benchtakes the parsed record instead of re-reading it.ungated-jitstats=andjitstats-band=join that pass._apply_snapshot_gatereads them per backend with nothing catching a
ValueError, so an unusablevalue in either arrived as a traceback out of a backend run rather than
as an authoring error.
Behaviour change: no fixture runs when any header is unusable. Previously the
ones ahead of the first offender ran first, and the same
exit(1)discardedtheir results.
…and asked where it costs nothing
The pass reads files and builds nothing — measured at 0.2 s over 479
fixtures — so it does not have to wait behind a backend build to answer.
--check-headersruns it and exits, and the step goes incargo-fmt, the jobevery expensive job in this workflow already hangs off through
needs: cargo-fmt. A fixture whose header cannot be read now reds a run inseconds instead of after the build.
parse_argsanswers the flag ahead of the backend resolution:DEFAULT_BACKENDSis computed at import and shells out to
rustup target list --installed, and acheck that only reads files should not depend on either.
opcode_ops — two residual bridges declared a pointer parameter
bh_w_exception_get_kindandbh_exception_object_matches_stop_iterationtookpyre_object::PyObjectRef, which is*mut PyObject: four bytes on wasm32 andeight on the native targets. A residual call supplies the integer arg slot, and
every other
bh_*bridge in the file already takesi64and casts. Droppingthe pointer parameter also drops the
#[allow(improper_ctypes_definitions)]itneeded.
Verification
Two real fixtures cannot grade the grouped report — they fail the same way. Six
hand-written ones do: two missing the directive, an unknown compile arm, a count
where a name belongs,
max-pypy-ratio=nan, and askip-backends=typo. Thereport splits them four ways and merges the identical pair.
Gate on
ef42e062da9after the change: dynasm 489/489, cranelift 489/489,wasm 481/481, and
cargo test -p pyrex --test gate_triage_complete6/6.