Restructure Rust plugin monorepo and CI#8
Conversation
cf5ea60 to
7a6418b
Compare
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
bac0658 to
a4c7fde
Compare
Signed-off-by: lucarlig <luca.carlig@ibm.com>
|
Interesting change, and better to make it now. How does this approach work from a release management standpoint? I'd wanted to keep each plugin in its own Python Package with its own versioning system. Is that still viable? It looks on first blush like all packages will get built/versioned/released in lockstep. (Maybe we can do something with tags...) |
Current state: plugins are still independently versioned and released, not lockstepped. Each plugin keeps its own version in its own |
gandhipratik203
left a comment
There was a problem hiding this comment.
What's Good
Catalog-driven architecture with strong convention enforcement, efficient CI that only builds affected plugins, and a lightweight fail-fast gate before expensive release builds. The pii_filter plugin is production-quality with RegexSet parallel matching, Luhn/SSN structural validation, and deterministic overlap resolution. The repo-contract test suite is exceptional (47+ tests covering catalog validation, CI selection, release resolution, stub exports, and workflow content assertions). The rate limiter migration to Rust plugin core is complete and faithful, with double-layer fail-open semantics preserved.
Findings
F1 — Orphan stub file in python/pii_filter_rust/
Two .pyi stubs exist for the pii_filter extension module. cpex_pii_filter/pii_filter_rust/__init__.pyi is the authoritative one (curated by stub_gen.rs, includes both PIIDetectorRust and PIIFilterPluginCore). python/pii_filter_rust/__init__.pyi is an orphan from pyo3_stub_gen's default output path — it's missing PIIFilterPluginCore, isn't used by maturin (python-source = "."), and isn't checked by verify-stubs. It will confuse IDEs and silently diverge as the API evolves. Delete it and add python/ to .gitignore, or have stub_gen.rs clean it up after generation.
F2 — stub_gen.rs curates stubs via fragile string replacement
curate_extension_stub() injects PIIFilterPluginCore using content.replace("\"PIIDetectorRust\",\n]", ...) — a literal match that assumes exact formatting from pyo3_stub_gen. If the library changes its output formatting in a future version, the replacement silently fails and the stub becomes incomplete. The existing verify-stubs step (git diff --exit-code) won't catch a first-time silent failure. Add a post-curation assertion that the output contains both "PIIFilterPluginCore" in __all__ and class PIIFilterPluginCore:.
F3 — Benchmark function detect_pii() skips validation and whitelist
The standalone detect_pii() used by all benchmarks does simple RegexSet matching. The production detect_internal() adds whitelist filtering, structural validation (SSN format, Luhn), and overlap resolution (O(n log n) sort + sweep). Benchmark results are artificially fast and won't catch regressions in the validation/dedup path. The benchmark named "whitelist_filtering" configures whitelist patterns but calls a function that ignores them. Either unify the two functions or switch benchmarks to use detect_internal().
F4 — No .gitignore updates for restructured paths
Build artifacts moved to plugins/rust/python-package/*/target/, .venv/, dist/ but no .gitignore changes are included. Verify the existing .gitignore has global pattern coverage; if it uses path-specific entries like rate_limiter/target/, the new locations lack coverage.
Verdict
Well-architected restructure with strong tooling and thorough testing. F1 and F2 should be addressed before merge — both are straightforward fixes. F3 is worth fixing so benchmarks measure real behavior. F4 needs a quick verification. None of these are heavy lifts.
|
Addressed Pratik's findings on the current PR head (
Validation run locally:
|
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Summary
plugins/rust/python-package, add the top-level Cargo workspace, and moverate_limiterinto the managed layout with the shared framework bridgepii_filterplugin as a managed package, including its Rust core, Python compatibility shims, manifests, stubs, benchmarks, and plugin testsplugin_catalog.py,install_built_wheel.py, rootMakefile), and add repo-contract tests for layout, release tags, and wheel selectionWhat Changed
rate_limiter/package intoplugins/rust/python-package/rate_limiter/and updated its Rust/Python surface to fit the managed plugin contractcpex_framework_bridgecrate for PyO3 framework object constructionplugins/rust/python-package/pii_filter/package with Rust detection/masking logic, manifests, generated stubs, and plugin testsREADME.md,DEVELOPING.md,TESTING.md) to describe the managed plugin layout, testing flow, and release modelValidation
python3 -m unittest tests/test_plugin_catalog.py tests/test_install_built_wheel.pypython3 tools/plugin_catalog.py validate .make verify-stubsinplugins/rust/python-package/pii_filter