AGENTS.md: name pyre-module in the cargo test command, and record the module-ownership criteria - #1970
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 37 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: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (18)
💤 Files with no reviewable changes (7)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe interpreter now declares and registers ChangesMath module ownership
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No concrete issue remains that would prevent merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Math and complex-math become available in interpreter-only builds. The inspected numeric validation and optional-module restrictions remain in place, and no concrete security regression was found. Not every build configuration has been verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.21% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 9 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit reads the module trail, Comment |
Re-measured after the rebase onto
|
| gate | result |
|---|---|
cargo test --all --no-default-features --features dynasm,pyre-module |
10,171 passed / 217 suites, 0 failed |
pyre/check.py dynasm |
567/567 |
pyre/check.py cranelift |
565/565 |
pyre/check.py wasm |
2 jit-stats rows regressed |
FAIL wasm synth/gc_pypy_frontend regressed: guard_failures 331 -> 375
FAIL wasm synth/pickle_terminal_raise_resume regressed: loops_aborted 3 -> 4;
improved: loops_compiled 21 -> 24,
guard_failures 140 -> 139
Both belong to #1960, not to this PR, and the baselines are deliberately left
as they are. The chain:
- Both fixtures carry wasm-specific baselines (
*.wasm.jitstats) last recorded
at665ead5c7fb(listobject getslice on strategy storage; nursery-allocated module-dict mutable cells #1932). e73c823645d(wasm: dead frame, guard cells and residual-call signatures on their upstream owners #1960, "wasm: dead frame, guard cells and residual-call
signatures on their upstream owners") is newer than those baselines and became
an ancestor of this branch only through the rebase.- The tree where wasm measured 557/557 earlier in this work did not contain
wasm: dead frame, guard cells and residual-call signatures on their upstream owners #1960;git merge-base --is-ancestor e73c823645d eca6247b907is false. - Neither fixture imports
math, and this PR's move commit touches no wasm file
and nopyre-jit/src/eval.rs.
#1960 is also why this PR's conflict resolution looks like it lost something:
it deleted the wasm faithful/vouched-ABI block in pyre-jit/src/eval.rs
wholesale, along with the math_faithful_residual_call_addrs and
math_word_residual_call_addrs hook fields that existed only to feed it. The
two edits this PR originally made to that block were resolved by taking #1960's
deletion; the two surviving math_* fields are the ones the move removes.
Re-recording either baseline would hide a residual-call signature change rather
than explain it, so that is left to #1960's owner.
— commented by Claude
4dc4cfa to
25ce3d7
Compare
6476483 to
806a5bd
Compare
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 50d5174). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
Merging this PR will not alter performance
Comparing 
Footnotes
|
CI after the rebase onto
|
4afd71c to
9bf1322
Compare
The CI cargo test steps pass `pyre-module`, and the tests that need the modules that crate owns are gated on the feature, so the command recorded here ran a smaller set than CI without saying so. Assisted-by: Claude
youknowone
changed the title
9bf1322 to
50d5174
Compare
youknowone
changed the title
1 participant
Two
AGENTS.mdcorrections about thepyre-interpreter/pyre-modulesplit.Doc-only; no code changes.
1. The prescribed
cargo testcommand dropspyre-module"Before committing" recorded
cargo test --all --no-default-features --features dynasm. Sincepyre-modulebecame an optional dependency (#1954) that commanddrops the crate owning the optional builtin modules — it is a
pyrexdefault, so--no-default-featuresleaves it out of the launcher the integration tests run.The tests that need those modules are gated on the feature, so nothing fails; the
prescribed command just runs a smaller set than CI's cargo test steps, which pass
dynasm,cpyext,pyre-module. The bullet now names the feature and says why.2. The ownership criteria were only in commit messages
Which crate owns a module is decided by three criteria — an import by name from
the interpreter (
baseobjspace.py finish→atexit,warnings→_contextvars,os→errno,eval.rs build_template_op→_template), PyPyessential_modules(_opcode), or CPythonModules/Setup.bootstrap(_abc,_functools,_stat,_suggestions,_symtable,_tokenize,_typing,faulthandler,pwd). Those are stated in #1954's commit messages and nowherea reader of
AGENTS.mdwould find them, so the section describing the core buildnow carries them.
It also records why PyPy's
default_modulestier is not a fourth criterion:pypyoption.pydeclares every module asBoolOption(modname, default=modname in default_modules), so the tier means "onby default, switchable off" — exactly what
pyrex'sdefault = [..., "pyre-module"]already expresses.defaultis notnon-optional.Why this PR is doc-only
It started as a move of
math/cmathintopyre-interpreteron thedefault_modulesreasoning above. That reasoning was wrong and the commit isdropped:
math/cmatharedefault_modulesbut notessential_modules, theyare absent from
Modules/Setup.bootstrapat the pinned 3.14.6, and nothing callsimport_module("math"). #1954 had decided this deliberately — it kept them inpyre-module, marked the dependent fixturesskip-backends=craneliftandrewrote five others onto
errno.Re-running the three criteria over the whole registry afterwards found nothing
misplaced: of the 41 names
pyre-moduleregisters, none is imported by name bythe interpreter, none is in
essential_modules, and none is inModules/Setup.bootstrap. The boundary is already right, which is why the onlything left to do was write the rule down.
🤖 Generated with Claude Code