feat: harden aic-core wheel and crate API contracts#1375
Conversation
6564991 to
3fd72d2
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
🧰 Additional context used📓 Path-based instructions (6).github/workflows/**⚙️ CodeRabbit configuration file
Files:
tests/**/*.{py,yaml,txt,sh}📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Files:
**/*📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*.{py,pyi}📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/**⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.44.1)tests/cross_package/test_core_public_api.py[error] 44-44: Command coming from incoming request (subprocess-from-request) 🔇 Additional comments (3)
WalkthroughChangesCore Public API Contract
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
3fd72d2 to
fdae85f
Compare
jasonqinzhou
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/build-test.yml:
- Around line 33-36: Add an explicit read-only permissions block to the
core-public-api-contract job, scoped to the job definition alongside runs-on and
timeout-minutes. Grant only the contents access needed for checkout and test
reads, with no write permissions.
In `@aic-core/rust/aiconfigurator-core/src/py.rs`:
- Around line 429-449: Document in
aic-core/rust/aiconfigurator-core/README.md:25-49 that standalone Rust callers
using AicEngineBuilder::build must enable the embed-python/pyo3::auto-initialize
prerequisite or initialize a host Python interpreter before calling it. The
AicEngineBuilder::build implementation at
aic-core/rust/aiconfigurator-core/src/py.rs:429-449 requires no direct code
change; update the README example and setup guidance to make this runtime
requirement explicit.
In `@tests/cross_package/test_core_public_api.py`:
- Around line 11-16: Add an isolated lazy-import assertion in the test module,
using a fresh interpreter or subprocess before importing the facade target
modules. Verify that importing the SDK facade alone does not preload its target
modules, then retain the existing API behavior checks; avoid top-level imports
of those targets that would mask eager initialization.
In `@tools/verify_installed_package_layers.py`:
- Around line 72-86: The verification flow must import aiconfigurator_core.sdk
in a fresh interpreter before preloading or importing any implementation
modules. Update the logic around the sdk facade check to assert the protected
implementation modules are absent from sys.modules immediately after that
import, then perform the existing direct submodule-import validation. Preserve
the expected_facade and public-name checks after confirming the lazy-import
boundary.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15e84acb-fce6-44c7-b195-14209564de0e
📒 Files selected for processing (21)
.github/workflows/build-test.ymlCargo.tomlaic-core/API.mdaic-core/rust/aiconfigurator-core/Cargo.tomlaic-core/rust/aiconfigurator-core/README.mdaic-core/rust/aiconfigurator-core/src/common/system_spec.rsaic-core/rust/aiconfigurator-core/src/lib.rsaic-core/rust/aiconfigurator-core/src/operators/attention.rsaic-core/rust/aiconfigurator-core/src/py.rsaic-core/rust/aiconfigurator-core/tests/embedded_round_trip.rsaic-core/rust/aiconfigurator-core/tests/memory_round_trip.rsaic-core/rust/aiconfigurator-core/tests/pyo3_smoke.rsaic-core/rust/public-api-tests/Cargo.tomlaic-core/rust/public-api-tests/src/lib.rsaic-core/src/aiconfigurator_core/_aiconfigurator_core.pyiaic-core/src/aiconfigurator_core/py.typedaic-core/src/aiconfigurator_core/sdk/__init__.pytests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.pytools/verify_installed_package_layers.pytools/verify_release_wheels.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Build and Test (unit)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Cargo Deny
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
**/*: Treat this file as the only always-injected rule file; do not add new always-on rules here without human approval. New rule files must includepaths:frontmatter.
When reviewing changes in a governed area, read that area's rule files first, even if path-based auto-loading does not occur during a read-only review. Rule violations are review findings even when the code works.
A task must remain within its module: collector tasks may change onlycollector/and its tests, generator tasks onlysrc/aiconfigurator/generator/and its tests, and SDK tasks must not modify either. Cross-module contract changes require explicit human approval.
Files:
Cargo.tomlaic-core/rust/aiconfigurator-core/Cargo.tomlaic-core/rust/aiconfigurator-core/tests/pyo3_smoke.rsaic-core/rust/aiconfigurator-core/src/common/system_spec.rsaic-core/rust/aiconfigurator-core/src/operators/attention.rsaic-core/src/aiconfigurator_core/py.typedaic-core/rust/public-api-tests/Cargo.tomltests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.pyaic-core/rust/aiconfigurator-core/src/lib.rsaic-core/rust/public-api-tests/src/lib.rsaic-core/rust/aiconfigurator-core/tests/memory_round_trip.rsaic-core/API.mdtools/verify_installed_package_layers.pyaic-core/rust/aiconfigurator-core/README.mdaic-core/src/aiconfigurator_core/_aiconfigurator_core.pyiaic-core/src/aiconfigurator_core/sdk/__init__.pytools/verify_release_wheels.pyaic-core/rust/aiconfigurator-core/tests/embedded_round_trip.rsaic-core/rust/aiconfigurator-core/src/py.rs
⚙️ CodeRabbit configuration file
**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.
- Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
- If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.
Files:
Cargo.tomlaic-core/rust/aiconfigurator-core/Cargo.tomlaic-core/rust/aiconfigurator-core/tests/pyo3_smoke.rsaic-core/rust/aiconfigurator-core/src/common/system_spec.rsaic-core/rust/aiconfigurator-core/src/operators/attention.rsaic-core/src/aiconfigurator_core/py.typedaic-core/rust/public-api-tests/Cargo.tomltests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.pyaic-core/rust/aiconfigurator-core/src/lib.rsaic-core/rust/public-api-tests/src/lib.rsaic-core/rust/aiconfigurator-core/tests/memory_round_trip.rsaic-core/API.mdtools/verify_installed_package_layers.pyaic-core/rust/aiconfigurator-core/README.mdaic-core/src/aiconfigurator_core/_aiconfigurator_core.pyiaic-core/src/aiconfigurator_core/sdk/__init__.pytools/verify_release_wheels.pyaic-core/rust/aiconfigurator-core/tests/embedded_round_trip.rsaic-core/rust/aiconfigurator-core/src/py.rs
tests/**/*.{py,yaml,txt,sh}
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Use integration tests for the full input-to-artifacts pipeline, comparing output against golden snapshots without external dependencies.
Files:
tests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
tests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.pytools/verify_installed_package_layers.pyaic-core/src/aiconfigurator_core/_aiconfigurator_core.pyiaic-core/src/aiconfigurator_core/sdk/__init__.pytools/verify_release_wheels.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run unit tests with
pytest -m unit; usepytest -m "unit or build"for the build-test subset when required data is available.
Files:
tests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: - Check that tests cover the changed behavior rather than only the happy path.
- Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.
Files:
tests/cross_package/test_core_public_api.pytests/cross_package/test_core_version_contract.py
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: - Review workflow edits for least-privilege permissions, safe trigger scopes, secret exposure, fork behavior, and release/cherry-pick side effects.
- Verify matrix jobs, artifact uploads, and scheduled support-matrix workflows still produce debuggable evidence.
Files:
.github/workflows/build-test.yml
aic-core/src/aiconfigurator_core/sdk/**
⚙️ CodeRabbit configuration file
aic-core/src/aiconfigurator_core/sdk/**: - Verify core SDK API changes remain compatible with legacy aiconfigurator.sdk imports, generator inputs, profiler data flow, and documented examples.
- Flag upper-layer dependencies or silent schema drift introduced into the minimal core distribution.
Files:
aic-core/src/aiconfigurator_core/sdk/__init__.py
🪛 ast-grep (0.44.1)
tests/cross_package/test_core_version_contract.py
[warning] 30-30: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(rf"^pub const {name}: u32 = (\d+);$", source, re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🪛 zizmor (1.26.1)
.github/workflows/build-test.yml
[warning] 33-67: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🔇 Additional comments (21)
aic-core/API.md (1)
1-120: LGTM!aic-core/src/aiconfigurator_core/_aiconfigurator_core.pyi (1)
1-41: LGTM!aic-core/src/aiconfigurator_core/py.typed (1)
1-1: LGTM!aic-core/src/aiconfigurator_core/sdk/__init__.py (1)
4-67: LGTM!tests/cross_package/test_core_public_api.py (1)
18-64: LGTM!aic-core/rust/public-api-tests/src/lib.rs (1)
1-71: LGTM!Cargo.toml (1)
5-8: LGTM!tests/cross_package/test_core_version_contract.py (1)
1-46: LGTM!tools/verify_release_wheels.py (1)
15-29: LGTM!Also applies to: 115-162
aic-core/rust/aiconfigurator-core/src/py.rs (1)
38-38: LGTM!Also applies to: 291-428, 453-493, 879-879
aic-core/rust/aiconfigurator-core/src/lib.rs (1)
59-59: LGTM!aic-core/rust/public-api-tests/Cargo.toml (1)
1-12: LGTM!aic-core/rust/aiconfigurator-core/Cargo.toml (1)
10-12: LGTM!aic-core/rust/aiconfigurator-core/README.md (1)
131-138: LGTM!aic-core/rust/aiconfigurator-core/src/common/system_spec.rs (1)
164-164: LGTM!aic-core/rust/aiconfigurator-core/src/operators/attention.rs (1)
346-352: LGTM!aic-core/rust/aiconfigurator-core/tests/embedded_round_trip.rs (1)
39-39: LGTM!Also applies to: 54-55, 88-89
aic-core/rust/aiconfigurator-core/tests/memory_round_trip.rs (1)
32-32: LGTM!Also applies to: 45-46, 128-129
aic-core/rust/aiconfigurator-core/tests/pyo3_smoke.rs (1)
12-13: LGTM!tools/verify_installed_package_layers.py (2)
51-53: LGTM!
43-86: 📐 Maintainability & Code QualityNo action needed.
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
| .expect("b200_sxm.yaml parse must succeed"); | ||
| assert_eq!(spec.data_dir, PathBuf::from("data/b200_sxm")); | ||
| assert_eq!(spec.gpu.mem_bw, 8_000_000_000_000.0); | ||
| assert_eq!(spec.gpu.mem_bw, 7_700_000_000_000.0); |
There was a problem hiding this comment.
Is this change needed by the PR? Seems unrelated?
| // Validate against the loaded system spec so fixture calibration | ||
| // updates do not leave a stale duplicate constant in this test. |
There was a problem hiding this comment.
Let's remove this block comment as the code is self-explanatory
| /// to Python's `compile_engine` defaults. [`build_aic_engine`] remains available | ||
| /// for source compatibility with existing callers. | ||
| #[derive(Clone, Debug)] | ||
| pub struct AicEngineBuilder { |
There was a problem hiding this comment.
AicEngineBuilder mostly re-wraps the flat build_aic_engine(...) kwargs. Would a AicEngineBuildConfig struct + build_aic_engine(config) be easier to understand? Would like to hear @tedzhouhk's preference as well.
// Current (Option A)
AicEngineBuilder::new("Qwen/Qwen3-32B", "h200_sxm", BackendKind::Vllm)
.tp_size(8)
.attention_dp_size(4)
.build()?;
// Proposed (Option B)
build_aic_engine(AicEngineBuildConfig {
model_path: "Qwen/Qwen3-32B".into(),
system: "h200_sxm".into(),
backend: BackendKind::Vllm,
tp_size: 8,
attention_dp_size: 4,
..Default::default()
})?;
If we keep Option A, we should remove the pub function build_aic_engine and just keep the AicEngineBuilder::build instead:
impl AicEngineBuilder {
pub fn build(self) -> Result<AicEngine, AicError> {
let engine = compile_engine_from_flat(
&self.model_path,
&self.system,
self.backend.as_str(),
self.backend_version.as_deref(),
self.tp_size,
// ... rest from self
)?;
Ok(AicEngine::new(engine))
}
}
| - compiled engine: `AicEngineBuilder`, `AicEngine`, `build_aic_engine`, | ||
| `AicError`; |
There was a problem hiding this comment.
Might need to change here too - some duplication can be removed
There was a problem hiding this comment.
Is it really needed? 👀
There was a problem hiding this comment.
I might be missing something, but why do we put public-api-tests under root instead of keeping it under tests?
2 participants
Summary
aiconfigurator_core.sdkfacade for the supported Python APIpy.typedplus a native-extension stub and verify both in built/installed wheelsAicEngineBuilderwhile preservingbuild_aic_enginecompatibilityValidation
84 passed: Python facade, import identity, version/schema, and package-layer testscargo test --workspace --no-default-features: 192 core + 3 external-consumer testscargo test --workspace --all-features: 197 core + 3 embedding integration + 3 external-consumer testsAIC_REQUIRE_EMBEDDED_ROUND_TRIP=1so skips are impossiblecargo package -p aiconfigurator-core --allow-dirtypackaged and verified the publishable crateSummary by CodeRabbit
AicEngineBuilderwith fluent configuration for model/backend, parallelism, quantization, speculative decoding, and cache sizing.aiconfigurator_core.sdkPython facade with lazy-loaded exports.*.pyistubs andpy.typed.