Add ext_py matrices and vectors#262
Conversation
📝 WalkthroughWalkthroughAdds a Rust-backed Python extension exposing finite-field elements, vectors, matrices, subspaces, quasi-inverses, affine subspaces, and augmented matrices. It also adds dynamic Rust field elements, serialization and borrowing safeguards, comprehensive pytest coverage, packaging metadata, and CI execution. ChangesPython extension foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Python as Python tests
participant Ext as PyO3 ext.fp
participant Rust as fp Rust types
participant CI as GitHub Actions
Python->>Ext: construct fields, vectors, matrices
Ext->>Rust: validate and execute algebra operations
Rust-->>Ext: return wrapped results or mapped errors
Ext-->>Python: expose values, views, and iterators
CI->>Ext: build wheel and run pytest
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ext_py.yaml:
- Around line 5-11: Add a top-level permissions block for the workflow with
least-privilege access, granting only contents: read for checkout and build/test
operations. Place it alongside the workflow’s jobs definition so the test job no
longer inherits broader GITHUB_TOKEN permissions.
In `@ext_py/src/fp_mod.rs`:
- Around line 32-35: Validate the degree argument in small_fq before calling
DynSmallFq::new, rejecting values that violate SmallFq::new’s d > 1 and
field-size constraints with PyValueError; only construct DynSmallFq after
validation succeeds.
- Around line 330-338: Update the __richcmp__ implementations for PyFp,
PySmallFq, and PyFieldElement to return Python NotImplemented for ordering
operations (Lt, Le, Gt, Ge) instead of false, while preserving the existing Eq
and Ne behavior; adjust the return type as required by the PyO3 API.
In `@ext_py/src/fp_mod/matrices.rs`:
- Around line 209-211: Replace the eager Vec-based implementations of
Matrix::iter and Matrix::iter_mut with lazy #[pyclass] iterator types
implementing Python’s iterator protocol, yielding one row handle per __next__
call; update the methods’ return types and preserve existing row/row_mut
behavior, documenting or tracking the API change as needed.
In `@ext_py/tests/test_affine_subspace.py`:
- Around line 140-150: Update the xfail markers for invalid-construction tests
to use strict=True, including test_invalid_construction_raises in
test_affine_subspace.py and the corresponding markers in test_quasi_inverse.py
and test_augmented_matrix.py. Preserve the existing reasons so any unexpected
pass causes the test suite to fail and prompts removal of the stale xfail.
In `@ext_py/tests/test_matrix.py`:
- Around line 375-386: Update the Python bindings for compute_kernel and
compute_image in the matrix binding implementation to validate that row
reduction/pivot computation has occurred before invoking the Rust methods. When
the precondition is unmet, return a PyValueError rather than allowing a Rust
panic to propagate, then remove the xfail markers from
test_compute_kernel_requires_row_reduce and
test_compute_image_requires_row_reduce so they pass normally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d73961f-cd00-4163-b351-44711a848384
📒 Files selected for processing (20)
.github/workflows/ext_py.yamlext/crates/fp/src/field/dyn_field.rsext/crates/fp/src/field/mod.rsext/crates/fp/src/matrix/matrix_inner.rsext_py/.gitignoreext_py/Cargo.tomlext_py/pyproject.tomlext_py/python/ext/__init__.pyext_py/src/fp_mod.rsext_py/src/fp_mod/matrices.rsext_py/src/fp_mod/vectors.rsext_py/src/lib.rsext_py/tests/test_affine_subspace.pyext_py/tests/test_augmented_matrix.pyext_py/tests/test_fp_vector_slices.pyext_py/tests/test_matrix.pyext_py/tests/test_matrix_slice_mut.pyext_py/tests/test_quasi_inverse.pyext_py/tests/test_subquotient.pyext_py/tests/test_subspace.py
| jobs: | ||
| test: | ||
| if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }} | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| RUST_BACKTRACE: 1 | ||
| RUSTFLAGS: "-D warnings" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a least-privilege permissions: block.
The workflow inherits the repository's default GITHUB_TOKEN permissions. Scope it down explicitly, since this job only checks out and builds/tests.
🔒 Suggested change
jobs:
test:
if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }}
runs-on: ubuntu-latest
+ permissions:
+ contents: read
env:
RUST_BACKTRACE: 1
RUSTFLAGS: "-D warnings"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jobs: | |
| test: | |
| if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }} | |
| runs-on: ubuntu-latest | |
| env: | |
| RUST_BACKTRACE: 1 | |
| RUSTFLAGS: "-D warnings" | |
| jobs: | |
| test: | |
| if: ${{ github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository }} | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| env: | |
| RUST_BACKTRACE: 1 | |
| RUSTFLAGS: "-D warnings" |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 6-6: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ext_py.yaml around lines 5 - 11, Add a top-level
permissions block for the workflow with least-privilege access, granting only
contents: read for checkout and build/test operations. Place it alongside the
workflow’s jobs definition so the test job no longer inherits broader
GITHUB_TOKEN permissions.
Source: Linters/SAST tools
| fn small_fq(p: u32, degree: u32) -> PyResult<DynSmallFq> { | ||
| let p = valid_prime(p)?; | ||
| Ok(DynSmallFq::new(p, degree)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DynSmallFq::new / SmallFq::new implementation in the fp crate
rg -n -A 20 'fn new' ext/crates/fp/src/ --type rust | rg -A 20 -B 2 'SmallFq|DynSmallFq'Repository: SpectralSequences/sseq
Length of output: 1943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline ext/crates/fp/src/field/smallfq.rs ==\n'
ast-grep outline ext/crates/fp/src/field/smallfq.rs --view expanded || true
printf '\n== outline ext_py/src/fp_mod.rs ==\n'
ast-grep outline ext_py/src/fp_mod.rs --view expanded || true
printf '\n== relevant lines in ext/crates/fp/src/field/smallfq.rs ==\n'
nl -ba ext/crates/fp/src/field/smallfq.rs | sed -n '1,260p'
printf '\n== relevant lines in ext_py/src/fp_mod.rs ==\n'
nl -ba ext_py/src/fp_mod.rs | sed -n '1,120p'Repository: SpectralSequences/sseq
Length of output: 5132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SmallFq::new implementation ==\n'
sed -n '113,140p' ext/crates/fp/src/field/smallfq.rs
printf '\n== DynSmallFq definition and constructor ==\n'
rg -n -A 20 -B 5 'struct DynSmallFq|impl DynSmallFq|fn new' ext/crates/fp/src -g '*.rs'
printf '\n== small_fq wrapper in ext_py/src/fp_mod.rs ==\n'
sed -n '24,40p' ext_py/src/fp_mod.rsRepository: SpectralSequences/sseq
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== DynSmallFq definition and constructor ==\n'
rg -n -A 25 -B 8 'DynSmallFq' ext/crates/fp/src ext_py/src -g '*.rs' | head -n 200
printf '\n== pyo3 / panic configuration ==\n'
rg -n -A 3 -B 2 'panic *=|panic =|catch_unwind|PanicException|#[[:space:]]*pyfunction|#[[:space:]]*pymodule' Cargo.toml **/Cargo.toml pyproject.toml ext_py -g '*'
printf '\n== exact small_fq wrapper ==\n'
sed -n '20,40p' ext_py/src/fp_mod.rsRepository: SpectralSequences/sseq
Length of output: 5774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== DynSmallFq and related wrappers ==\n'
rg -n -A 12 -B 6 'DynSmallFq|small_fq\(|PySmallFq|#[[:space:]]*pyfunction|panic =|catch_unwind|PanicException' ext/crates/fp/src ext_py/src Cargo.toml pyproject.toml -g '*.rs' -g '*.toml'
printf '\n== file sizes ==\n'
wc -l ext/crates/fp/src/field/smallfq.rs ext_py/src/fp_mod.rsRepository: SpectralSequences/sseq
Length of output: 11956
Validate degree before calling DynSmallFq::new (ext_py/src/fp_mod.rs:32-35). SmallFq::new asserts d > 1 and the field-size bound, so invalid degrees currently raise a PanicException instead of a PyValueError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/src/fp_mod.rs` around lines 32 - 35, Validate the degree argument in
small_fq before calling DynSmallFq::new, rejecting values that violate
SmallFq::new’s d > 1 and field-size constraints with PyValueError; only
construct DynSmallFq after validation succeeds.
| pub fn __richcmp__(&self, other: &Bound<'_, PyAny>, op: CompareOp) -> bool { | ||
| let eq = other | ||
| .extract::<PyRef<Self>>() | ||
| .is_ok_and(|other| self.0 == other.0); | ||
| match op { | ||
| CompareOp::Eq => eq, | ||
| CompareOp::Ne => !eq, | ||
| _ => false, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the surrounding implementation.
wc -l ext_py/src/fp_mod.rs
sed -n '1,80p' ext_py/src/fp_mod.rs
printf '\n--- around __richcmp__ ---\n'
sed -n '300,360p' ext_py/src/fp_mod.rs
# Search for other rich comparison implementations in the repository for comparison.
printf '\n--- other __richcmp__ occurrences ---\n'
rg -n "__richcmp__|CompareOp::(Lt|Le|Gt|Ge|Eq|Ne)" ext_py/src -SRepository: SpectralSequences/sseq
Length of output: 5002
🌐 Web query:
PyO3 __richcmp__ return type PyResult<bool> CompareOp ordering operators TypeError unsupported comparison
💡 Result:
In PyO3, the richcmp method is used to implement Python comparison operators [1][2]. The return type of richcmp is typically PyResult or bool, though it can technically return any Python object [3][4][1]. If your implementation is raising a TypeError: unsupported comparison, it is often because your method is not returning NotImplemented when it encounters an unsupported type [3][5][6]. Key points for implementation: 1. Return NotImplemented: When you cannot compare your type with the provided other object, you should return NotImplemented [3][5]. This allows the Python interpreter to try the reversed operation on the other object [7]. If you return an error (such as a TypeError from failed argument extraction or custom code), the comparison fails immediately rather than allowing Python to fall back [8][7]. 2. Handling Other Types: If your function signature specifies a concrete type (e.g., &Self), PyO3's generated code will automatically return NotImplemented if the other argument cannot be extracted into that type [3][5][6]. If you instead use a generic type or PyAny as the argument, you must manually handle type checking and return NotImplemented if the type is incompatible [3][5]. 3. Recommended Approach: Use PyResult or returning a PyObject directly allows you to return py.NotImplemented explicitly when needed [5][9]. Example using PyResult to return NotImplemented: fn richcmp(&self, other: &Bound<', PyAny>, op: CompareOp, py: Python<'>) -> PyResult { if let Ok(other) = other.extract::<&Self> { // Perform comparison and return boolean let result = op.matches(self.0.cmp(&other.0)); Ok(result.into_pyobject(py)?.into_any) } else { // Return NotImplemented so Python can try the reversed operation Ok(py.NotImplemented.into_any) } } Alternatively, if you are only comparing against the same type, relying on the automatic type extraction (by setting the argument type to &Self) is the simplest way to get automatic NotImplemented behavior [3][1][2].
Citations:
- 1: https://pyo3.rs/main/class/object
- 2: https://pyo3.rs/main/class/object.html
- 3: https://pyo3.rs/latest/class/protocols
- 4: https://pyo3.rs/main/class/protocols
- 5: https://pyo3.github.io/pyo3/v0.20.0/class/protocols.html
- 6: https://pyo3.rs/v0.18.0/class/protocols
- 7: #1064: Comparisons with __eq__ should not raise TypeError PyO3/pyo3#1072
- 8: Comparisons with __eq__ should not raise TypeError PyO3/pyo3#1064
- 9:
__richcmp__API docs could be expanded PyO3/pyo3#1625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether these field types are documented as unordered / whether any
# comparison helpers or NotImplemented handling already exist.
rg -n "NotImplemented|partial_cmp|Ord|PartialOrd|richcmp|ordering comparisons" ext_py/src fp src -S || true
printf '\n--- nearby docs for PyFp/PySmallFq/PyFieldElement ---\n'
sed -n '360,470p' ext_py/src/fp_mod.rsRepository: SpectralSequences/sseq
Length of output: 3883
Reject ordering comparisons for these field types. PyFp, PySmallFq, and PyFieldElement are only equality-comparable; returning false for <, <=, >, and >= makes Python treat them as ordered and can hide bugs in callers like sorted() or min(). Return NotImplemented so Python raises TypeError for those operators instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/src/fp_mod.rs` around lines 330 - 338, Update the __richcmp__
implementations for PyFp, PySmallFq, and PyFieldElement to return Python
NotImplemented for ordering operations (Lt, Le, Gt, Ge) instead of false, while
preserving the existing Eq and Ne behavior; adjust the return type as required
by the PyO3 API.
| pub fn iter(&self, py: Python<'_>) -> PyResult<Vec<PyFpSlice>> { | ||
| (0..self.rows_span()).map(|i| self.row(py, i)).collect() | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Eager row-handle materialization on iter/iter_mut.
iter/iter_mut allocate a Vec of per-row handles (as the TODOs note), which scales linearly with row count and defeats laziness for large matrices. Not a correctness issue, but worth converting to a lazy #[pyclass] iterator later. Want me to open a tracking issue?
Also applies to: 219-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/src/fp_mod/matrices.rs` around lines 209 - 211, Replace the eager
Vec-based implementations of Matrix::iter and Matrix::iter_mut with lazy
#[pyclass] iterator types implementing Python’s iterator protocol, yielding one
row handle per __next__ call; update the methods’ return types and preserve
existing row/row_mut behavior, documenting or tracking the API change as needed.
| @pytest.mark.xfail(reason="panics") | ||
| def test_invalid_construction_raises(): | ||
| linear = make_subspace(2, [[0, 1, 0]]) | ||
|
|
||
| # Offset length must match the linear part's ambient dimension. | ||
| with pytest.raises(ValueError): | ||
| fp.AffineSubspace(fp.FpVector.from_py(2, [1, 0]), linear) | ||
|
|
||
| # Offset prime must match the linear part's prime. | ||
| with pytest.raises(ValueError): | ||
| fp.AffineSubspace(fp.FpVector.from_py(3, [1, 0, 0]), linear) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add strict=True to xfail markers to catch stale panics.
The @pytest.mark.xfail(reason="panics") without strict=True means that if the Rust backend is later fixed to raise ValueError instead of panicking, the test will silently XPASS without causing a failure. Adding strict=True ensures the test suite fails when the underlying issue is resolved, prompting removal of the marker.
This same pattern appears in test_quasi_inverse.py (lines 152, 162, 178) and test_augmented_matrix.py (line 170).
The underlying Rust backend panics on invalid construction are a stability concern — panics in a PyO3 extension can abort the Python process. Would you like help adding input-validation guards in the Rust backend to return ValueError instead?
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 145-145: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 149-149: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_affine_subspace.py` around lines 140 - 150, Update the
xfail markers for invalid-construction tests to use strict=True, including
test_invalid_construction_raises in test_affine_subspace.py and the
corresponding markers in test_quasi_inverse.py and test_augmented_matrix.py.
Preserve the existing reasons so any unexpected pass causes the test suite to
fail and prompts removal of the stale xfail.
| @pytest.mark.xfail(reason="Raises PanicException") | ||
| def test_compute_kernel_requires_row_reduce(): | ||
| m = fp.AugmentedMatrix2.from_py(3, [[1, 0, 1], [0, 1, 1]]) | ||
| with pytest.raises(ValueError): | ||
| m.compute_kernel() | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="Raises PanicException") | ||
| def test_compute_image_requires_row_reduce(): | ||
| m = fp.AugmentedMatrix2.from_py(3, [[1, 0, 1], [0, 1, 1]]) | ||
| with pytest.raises(ValueError): | ||
| m.compute_image() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
xfail tests document PanicException instead of ValueError — bindings should convert panics to Python exceptions.
Both test_compute_kernel_requires_row_reduce and test_compute_image_requires_row_reduce expect ValueError but are marked xfail because the Rust bindings panic with PanicException when compute_kernel/compute_image are called before row_reduce. A Rust panic unwinds through PyO3 and produces a PanicException in Python, which is not a catchable ValueError and can crash the interpreter in some configurations. The bindings should validate the precondition (e.g., check that pivots have been computed) and return a PyValueError before calling the Rust method.
Would you like me to help implement the guard in ext_py/src/fp_mod/matrices.rs so these xfail tests can be flipped to passing?
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 378-378: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 385-385: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_matrix.py` around lines 375 - 386, Update the Python
bindings for compute_kernel and compute_image in the matrix binding
implementation to validate that row reduction/pivot computation has occurred
before invoking the Rust methods. When the precondition is unmet, return a
PyValueError rather than allowing a Rust panic to propagate, then remove the
xfail markers from test_compute_kernel_requires_row_reduce and
test_compute_image_requires_row_reduce so they pass normally.
Adds a basic Python project scafold including basic vector bindings and tests, and CI integration.
| # Stale-`dimension` quirk: `set_to_full` makes gens the entire space and | ||
| # clears the quotient, but upstream does NOT update the cached `dimension` | ||
| # counter. So on a fresh Subquotient these are inconsistent today: | ||
| # dimension()/len(sq) report 0 (stale), while gens() actually has 3 rows. | ||
| # Pin the surprising current behavior so a future upstream fix (syncing the | ||
| # cached dimension) trips this test and prompts a revisit. | ||
| assert sq.dimension == 0 | ||
| assert len(sq) == 0 | ||
| assert len(sq.gens) == 3 |
There was a problem hiding this comment.
Maybe we should fix this...
There was a problem hiding this comment.
I agree. Should we merge this first or fix first?
|
|
||
| def test_from_parts(): | ||
| sub = fp.Subspace(2, 3) | ||
| sub.add_vector(fp.FpVector.from_py(2, [1, 0, 0])) |
There was a problem hiding this comment.
Maybe add_vector should also accept a list directly.
| /// Apply `matrix` to each generator of `source`, then reduce the image | ||
| /// in `target`, returning the coefficient lists. | ||
| #[staticmethod] | ||
| pub fn reduce_matrix( |
There was a problem hiding this comment.
A bit niche maybe we should skip binding this for now.
There was a problem hiding this comment.
I agree it's of limited use even on the Rust side
| def test_basis_matches_iter(): | ||
| m = fp.Matrix.from_py(3, [[1, 0, 0], [0, 1, 2]]) | ||
| s = fp.Subspace.from_matrix(m) | ||
| assert [list(v) for v in s.basis] == [[1, 0, 0], [0, 1, 2]] |
There was a problem hiding this comment.
Would be nice:
| assert [list(v) for v in s.basis] == [[1, 0, 0], [0, 1, 2]] | |
| assert s.basis.to_py() == [[1, 0, 0], [0, 1, 2]] |
to_py() could be to_list() instead I suppose.
| s = fp.Subspace.from_matrix(m) | ||
| v = fp.FpVector.from_py(3, [1, 0, 0]) | ||
| # An owned FpVector and an FpSlice over it behave identically. | ||
| assert s.contains(v) |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ext_py.yaml:
- Line 14: Add persist-credentials: false to the actions/checkout@v5 step in the
workflow, ensuring the checkout action does not store the GITHUB_TOKEN in Git
configuration before subsequent build and installation steps run.
In `@ext_py/.gitignore`:
- Around line 1-2: Update the ext_py ignore rules to include target/ and dist/
alongside the existing Python cache and shared-library patterns, preventing
Cargo and maturin build artifacts from being committed.
In `@ext_py/tests/test_fp_vector_slices.py`:
- Around line 232-235: Update the pytest.raises(ValueError) assertions around
target.add(same_prime_short, 1) and target.add(other_prime, 1) to include match=
patterns for the expected prime/length mismatch error messages, narrowing each
assertion to the specific failure and satisfying Ruff PT011.
- Line 77: Ruff B018 flags the standalone getter accesses used to trigger
stale-slice errors. Update the corresponding accesses in the tests around
s.prime and the other stale-slice checks to either assign the result to _ or add
# noqa: B018, preserving the intentional property access.
In `@ext_py/tests/test_matrix.py`:
- Around line 17-28: Remove the xfail in test_matrix_to_py_zero_dimensions by
fixing the Rust matrix bindings to support zero-row and zero-column matrices
without panicking. Update the implementations backing Matrix.to_py(),
Matrix.row(), and the rows/columns accessors to return the expected empty
results and dimensions, including [] for zero rows and empty row lists for zero
columns.
In `@ext_py/tests/test_quasi_inverse.py`:
- Around line 152-191: Replace the xfail markers on
test_compute_quasi_inverse_requires_row_reduce, test_inconsistent_image_raises,
and test_pivot_out_of_range_raises by adding Python-binding validation in the
corresponding compute_quasi_inverse and QuasiInverse constructor implementations
in matrices.rs. Validate that row-reduction pivot metadata is initialized, the
image length matches the preimage row count, and every non-negative pivot is
within the preimage row range; raise PyValueError before calling the Rust
backend, then make these tests expect and pass with ValueError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 973094fa-1ef9-4e46-98b1-19f8b9c7c4fc
📒 Files selected for processing (20)
.github/workflows/ext_py.yamlext/crates/fp/src/field/dyn_field.rsext/crates/fp/src/field/mod.rsext/crates/fp/src/matrix/matrix_inner.rsext_py/.gitignoreext_py/Cargo.tomlext_py/pyproject.tomlext_py/python/ext/__init__.pyext_py/src/fp_mod.rsext_py/src/fp_mod/matrices.rsext_py/src/fp_mod/vectors.rsext_py/src/lib.rsext_py/tests/test_affine_subspace.pyext_py/tests/test_augmented_matrix.pyext_py/tests/test_fp_vector_slices.pyext_py/tests/test_matrix.pyext_py/tests/test_matrix_slice_mut.pyext_py/tests/test_quasi_inverse.pyext_py/tests/test_subquotient.pyext_py/tests/test_subspace.py
| RUSTFLAGS: "-D warnings" | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable credential persistence on checkout.
actions/checkout persists the GITHUB_TOKEN in .git/config by default. Subsequent steps (pip install, maturin build) execute third-party build scripts that could exfiltrate the persisted token. Add persist-credentials: false to eliminate this attack vector.
🔒 Proposed fix
- uses: actions/checkout@v5
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v5 | |
| - uses: actions/checkout@v5 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ext_py.yaml at line 14, Add persist-credentials: false to
the actions/checkout@v5 step in the workflow, ensuring the checkout action does
not store the GITHUB_TOKEN in Git configuration before subsequent build and
installation steps run.
| __pycache__ | ||
| *.so |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add target/ and dist/ to ignore build artifacts.
ext_py/Cargo.toml will cause cargo build/maturin build to create ext_py/target/, and maturin build --out dist creates ext_py/dist/. Neither is ignored, risking accidental commits of large, platform-specific artifacts.
♻️ Proposed addition
__pycache__
*.so
+target/
+dist/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| __pycache__ | |
| *.so | |
| __pycache__ | |
| *.so | |
| target/ | |
| dist/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/.gitignore` around lines 1 - 2, Update the ext_py ignore rules to
include target/ and dist/ alongside the existing Python cache and shared-library
patterns, preventing Cargo and maturin build artifacts from being committed.
| v.set_scratch_vector_size(2) | ||
|
|
||
| with pytest.raises(IndexError): | ||
| s.prime |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff B018 will fail the new CI lint step.
Lines 77, 273, and 277 access a getter property purely to trigger the stale-slice IndexError, which Ruff flags as a useless expression (B018). Since this PR adds a CI job that runs Ruff (and B is clearly selected, as it was reported), these will fail the lint. Add # noqa: B018 or bind to _ to keep the side-effecting access.
💚 Example fix
- with pytest.raises(IndexError):
- s.prime
+ with pytest.raises(IndexError):
+ _ = s.prime📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.prime | |
| _ = s.prime |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 77-77: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_fp_vector_slices.py` at line 77, Ruff B018 flags the
standalone getter accesses used to trigger stale-slice errors. Update the
corresponding accesses in the tests around s.prime and the other stale-slice
checks to either assign the result to _ or add # noqa: B018, preserving the
intentional property access.
Source: Linters/SAST tools
| with pytest.raises(ValueError): | ||
| target.add(same_prime_short, 1) | ||
| with pytest.raises(ValueError): | ||
| target.add(other_prime, 1) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the pytest.raises(ValueError) (Ruff PT011).
Ruff flags these broad ValueError expectations (PT011); add a match= substring so the assertions confirm the specific failure (prime/length mismatch) and pass the CI lint step.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 232-232: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 234-234: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_fp_vector_slices.py` around lines 232 - 235, Update the
pytest.raises(ValueError) assertions around target.add(same_prime_short, 1) and
target.add(other_prime, 1) to include match= patterns for the expected
prime/length mismatch error messages, narrowing each assertion to the specific
failure and satisfying Ruff PT011.
Source: Linters/SAST tools
| @pytest.mark.xfail(reason="Needs fix") | ||
| def test_matrix_to_py_zero_dimensions(): | ||
| # Regression: a matrix with zero columns and nonzero rows shouldn't panic | ||
| assert fp.Matrix(2, 1, 0).to_py() == [[]] | ||
| assert fp.Matrix(2, 3, 0).to_py() == [[], [], []] | ||
| # Zero rows (with or without columns) is the empty list of rows. | ||
| assert fp.Matrix(2, 0, 3).to_py() == [] | ||
| assert fp.Matrix(2, 0, 0).to_py() == [] | ||
| # Sibling row-materializing access must also not panic on zero columns. | ||
| assert fp.Matrix(2, 1, 0).rows == 1 | ||
| assert fp.Matrix(2, 1, 0).columns == 0 | ||
| assert list(fp.Matrix(2, 1, 0).row(0)) == [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Zero-dimension matrix operations panic instead of returning empty results.
The xfail marker documents that constructing matrices with zero columns or zero rows and calling to_py(), row(), or accessing rows/columns panics in the Rust backend. The bindings should handle zero-dimension matrices gracefully (returning empty lists) rather than panicking, since zero-dimension matrices are valid mathematical objects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_matrix.py` around lines 17 - 28, Remove the xfail in
test_matrix_to_py_zero_dimensions by fixing the Rust matrix bindings to support
zero-row and zero-column matrices without panicking. Update the implementations
backing Matrix.to_py(), Matrix.row(), and the rows/columns accessors to return
the expected empty results and dimensions, including [] for zero rows and empty
row lists for zero columns.
| @pytest.mark.xfail(reason="panics") | ||
| def test_compute_quasi_inverse_requires_row_reduce(): | ||
| # Without row_reduce the matrix has uninitialized (empty) pivots, which | ||
| # upstream would slice out of bounds and panic. The guard turns that into | ||
| # a clean ValueError. | ||
| m = fp.AugmentedMatrix2.from_py(3, [[1, 0, 1], [0, 1, 1]]) | ||
| with pytest.raises(ValueError): | ||
| m.compute_quasi_inverse() | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="panics") | ||
| def test_inconsistent_image_raises(): | ||
| # preimage has 4 rows; supply an image with 5 non-negative pivots. | ||
| preimage = fp.Matrix.from_py( | ||
| 2, | ||
| [ | ||
| [1, 0, 1, 1], | ||
| [1, 1, 0, 0], | ||
| [0, 1, 0, 1], | ||
| [1, 1, 1, 0], | ||
| ], | ||
| ) | ||
| with pytest.raises(ValueError): | ||
| fp.QuasiInverse([0, 1, 2, 3, 0], preimage) | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="panics") | ||
| def test_pivot_out_of_range_raises(): | ||
| # preimage has 4 rows; a non-negative pivot of 4 is an invalid row index. | ||
| preimage = fp.Matrix.from_py( | ||
| 2, | ||
| [ | ||
| [1, 0, 1, 1], | ||
| [1, 1, 0, 0], | ||
| [0, 1, 0, 1], | ||
| [1, 1, 1, 0], | ||
| ], | ||
| ) | ||
| with pytest.raises(ValueError): | ||
| fp.QuasiInverse([0, 1, 2, 4], preimage) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Three xfail tests document panics instead of ValueError — bindings should validate preconditions.
test_compute_quasi_inverse_requires_row_reduce, test_inconsistent_image_raises, and test_pivot_out_of_range_raises all expect ValueError but are marked xfail because the Rust bindings panic. The upstream compute_quasi_inverse (matrix_inner.rs:793-809) relies on pivot metadata from row reduction, and the QuasiInverse constructor doesn't validate that pivot indices are in range or that the image dimension matches the preimage row count. Each of these should be guarded in the Python bindings (ext_py/src/fp_mod/matrices.rs) to return PyValueError before invoking the Rust backend.
Would you like me to help implement the validation guards so these xfail tests can be flipped to passing?
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 158-158: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 174-174: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
[warning] 190-190: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext_py/tests/test_quasi_inverse.py` around lines 152 - 191, Replace the xfail
markers on test_compute_quasi_inverse_requires_row_reduce,
test_inconsistent_image_raises, and test_pivot_out_of_range_raises by adding
Python-binding validation in the corresponding compute_quasi_inverse and
QuasiInverse constructor implementations in matrices.rs. Validate that
row-reduction pivot metadata is initialized, the image length matches the
preimage row count, and every non-negative pivot is within the preimage row
range; raise PyValueError before calling the Rust backend, then make these tests
expect and pass with ValueError.
| /// over a different prime/degree) is a genuine failure, not a value: the binary operators report it | ||
| /// as [`None`]/[`Err`] rather than silently coercing one operand or returning a default element. | ||
| #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] | ||
| pub enum DynFieldElement { |
There was a problem hiding this comment.
So the only reason we can't just have DynField be the enum and then type DynFieldElement = FieldElement<DynField> is because we want fallible field operations when the fields are different.
That makes me think we should fix that API in the fp crate itself.
| /// Python `repr` of the field an erased element lives in (e.g. `Fp(2)` or | ||
| /// `SmallFq(2, 3)`), matching the `__repr__` of the `Fp`/`SmallFq` classes. | ||
| /// Used to build the mismatched-field error message. | ||
| fn field_repr(x: DynFieldElement) -> String { |
There was a problem hiding this comment.
This can just call the Display impl of SmallFq, and there should also be a Display impl for Fp.
| fn borrow_error(err: impl ToString) -> PyErr { | ||
| PyRuntimeError::new_err(err.to_string()) | ||
| } | ||
|
|
||
| /// Map any stringifiable error (e.g. `std::io::Error` from | ||
| /// (de)serialization) into the `RuntimeError` used uniformly across the | ||
| /// `to_bytes`/`from_bytes` methods. | ||
| fn io_err(e: impl ToString) -> PyErr { | ||
| PyRuntimeError::new_err(e.to_string()) | ||
| } |
There was a problem hiding this comment.
I get that they're semantically different but do we really need both?
| /// on the wrapped value, `Eq`/`Ne` are the only supported operators, and | ||
| /// hashes go through the shared `py_hash` helper. | ||
| /// | ||
| /// This crate does not enable PyO3's `multiple-pymethods` feature, so each |
There was a problem hiding this comment.
Why do we not just enable that for simplicity?
| } | ||
|
|
||
| pub fn __repr__(&self) -> String { | ||
| format!("Fp({})", self.characteristic()) |
There was a problem hiding this comment.
Likewise, we can just use Display.
| format!("Fp({})", self.characteristic()) | |
| self.to_string() |
| /// | ||
| /// The upstream `Subspace::iter_all_vectors` iterator borrows the subspace, | ||
| /// so it cannot be stored alongside an owned subspace in a `#[pyclass]` | ||
| /// without a self-referential struct. Instead we own a clone of the |
There was a problem hiding this comment.
We can have self-referential structs with e.g. the ouroboros crate
| target_slice.as_slice().prime().as_u32(), | ||
| )?; | ||
| checked_equal_len(target_slice.as_slice().len(), self.0.source_dimension())?; | ||
| // Reduce `coeff` mod p before calling upstream to avoid overflow |
There was a problem hiding this comment.
Do we really need to do that?
| build-backend = "maturin" | ||
|
|
||
| [project] | ||
| name = "sseq_ext" |
There was a problem hiding this comment.
Is that the name we're settling on? I suppose it fits.
|
|
||
|
|
||
| def test_augmented_matrix2_compute_image_and_quasi_inverse(): | ||
| # [I | I] over F2: A is the 2x2 identity, the second block is the identity. |
There was a problem hiding this comment.
| # [I | I] over F2: A is the 2x2 identity, the second block is the identity. | |
| # [I | I] over F2: both blocks are the identity. |
| } | ||
|
|
||
| #[staticmethod] | ||
| pub fn from_py(p: u32, entries: Vec<u32>) -> PyResult<Self> { |
There was a problem hiding this comment.
Should we make this an overloaded constructor? This would simplify the tests at least.
2 participants
Adds a basic Python project scafold including basic vector bindings and tests, and CI integration.
Summary by CodeRabbit