feat(sdk): declared-reuse ordering, strict provenance mode, evidence policy (AIC-1503, AIC-1219)#1374
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Arsene12358
changed the title
Arsene12358
force-pushed
the
yimingl/aic-1502-provenance-reuse
branch
from
2b73e49 to
cf0dc28
Compare
Arsene12358
force-pushed
the
yimingl/aic-1503-ordering-evidence
branch
from
10d12b1 to
d2bde0a
Compare
|
CI fixes pushed (head
|
Arsene12358
force-pushed
the
yimingl/aic-1502-provenance-reuse
branch
from
cf0dc28 to
b0eb31f
Compare
Arsene12358
force-pushed
the
yimingl/aic-1503-ordering-evidence
branch
from
d2bde0a to
24a6de5
Compare
Arsene12358
force-pushed
the
yimingl/aic-1503-ordering-evidence
branch
from
24a6de5 to
6562580
Compare
|
Round 4 (head |
Arsene12358
force-pushed
the
yimingl/aic-1502-provenance-reuse
branch
from
18e70d2 to
d641c8e
Compare
Arsene12358
requested review from
AichenF and
atrifex
and removed request for
a team
Arsene12358
force-pushed
the
yimingl/aic-1503-ordering-evidence
branch
from
6b818a7 to
42ad681
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/unit/sdk/database/test_database_helpers.py (1)
595-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise both strictness cache variants.
These tests update the tuple shape but only seed
strict_provenance=False. Add(..., True)entries so clearing and unloading are verified across the new cache dimension.Proposed test additions
perf_database.databases_cache[("root", "system", False, False)]["backend"]["1.0.0"] = clear_db + perf_database.databases_cache[("root", "system", False, True)]["backend"]["1.0.0"] = clear_db perf_database.databases_cache[("root", "system", True, False)]["backend"]["1.0.0"] = clear_dbkeep_db = FakeDatabase() unload_db = FakeDatabase() + strict_unload_db = FakeDatabase() @@ perf_database.databases_cache[("root", "system", False, False)]["backend"]["1.0.0"] = unload_db + perf_database.databases_cache[("root", "system", False, True)]["backend"]["1.0.0"] = strict_unload_db @@ assert unload_db.clear_count == 1 + assert strict_unload_db.clear_count == 1 + assert ("root", "system", False, True) not in perf_database.databases_cacheAs per path instructions, tests must “cover the changed behavior rather than only the happy path.”
Also applies to: 622-630
🤖 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 `@tests/unit/sdk/database/test_database_helpers.py` around lines 595 - 598, The test cache setup around the performance database cases only seeds strict_provenance=False entries. Add corresponding entries with strict_provenance=True for the clear_db and keep_db scenarios, including the related setup around the other referenced test section, so clearing and unloading are exercised for both strictness variants.Source: Path instructions
🤖 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 `@aic-core/src/aiconfigurator_core/sdk/perf_database.py`:
- Around line 687-693: Update _version_dir_data_filenames and its callers to
preserve os.listdir failures instead of treating them as an empty file list:
raise the inspection error when strict mode is enabled, and emit a warning while
continuing only in non-strict mode. Ensure the behavior also covers the related
handling at the indicated caller path.
- Line 1226: The cache update must not place an unvalidated worker-created
database under a strict-provenance key. In the worker-loading flow that assigns
`databases_cache[(systems_root, system, shared_flag,
database.strict_provenance)][backend][version]`, validate the database reference
before insertion, preserving unvalidated direct-construction behavior elsewhere.
Add a regression test covering a subsequent `get_database(...,
strict_provenance=True)` cache hit and confirming request validation still
occurs.
In `@tests/unit/sdk/test_perf_database_shared_layer.py`:
- Around line 389-390: Move the lazy database build, _gemm_lookup assertion, and
fallback warning assertions into the caplog.at_level(logging.WARNING,
logger="aiconfigurator.sdk.perf_database") block in the relevant test. Ensure
GEMM.load_data(db) executes while warning capture is active and retain the
expectation that exactly one low-fidelity fallback warning is recorded.
In `@tools/perf_database/evidence_check.py`:
- Around line 295-305: Validate touched generations before branching on reason
in the evidence-check flow: update tools/perf_database/evidence_check.py lines
295-305 so _touched_generations and unmapped-system validation run for case_plan
entries, while representative selection remains skipped only for case_plan.
Parameterize the unmapped-system regression in
tests/unit/tools/test_evidence_check.py lines 232-239 across all three reasons.
- Around line 140-151: Update the policy-loading validation around the resolved
mapping to reject any system name assigned to more than one generation. Track
systems seen while iterating raw_system_generations, raise EvidencePolicyError
identifying the conflicting system and generations, and only add entries to
resolved when ownership is unique.
- Around line 97-100: Update the thresholds.parquet_diff_median_pct validation
in the evidence policy parsing flow to reject negative values and non-finite
numbers such as NaN and infinity, while continuing to accept finite,
non-negative numeric values and reject booleans. Use the existing threshold
validation branch and an appropriate numeric-finiteness check.
- Around line 235-260: Update _parse_changed_entry to validate that every item
in tables and systems is a string, rejecting malformed entries such as [{}] and
[[]] with EvidenceManifestError before constructing ChangedEntry. Add
corresponding malformed-manifest cases to TestMalformedManifest in
tests/unit/tools/test_evidence_check.py.
---
Nitpick comments:
In `@tests/unit/sdk/database/test_database_helpers.py`:
- Around line 595-598: The test cache setup around the performance database
cases only seeds strict_provenance=False entries. Add corresponding entries with
strict_provenance=True for the clear_db and keep_db scenarios, including the
related setup around the other referenced test section, so clearing and
unloading are exercised for both strictness variants.
🪄 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: c1344228-7c0d-4e58-9394-ad6956bed3d1
📒 Files selected for processing (23)
.github/workflows/build-test.ymlaic-core/src/aiconfigurator_core/sdk/perf_database.pyaic-core/src/aiconfigurator_core/systems/data/l40s/attention/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/attention/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/attention/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/mla/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/mla_bmm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/quantize/trtllm/1.3.0rc15/reuse.yamlcollector/evidence_policy.yamldocs/perf_database/collector-v3-op-centric-design.mdtests/unit/sdk/database/test_data_loaders.pytests/unit/sdk/database/test_database_helpers.pytests/unit/sdk/database/test_reuse_ordering.pytests/unit/sdk/database/test_strict_mode.pytests/unit/sdk/test_perf_database_shared_layer.pytests/unit/tools/test_evidence_check.pytools/perf_database/evidence_check.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
**/*
📄 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:
aic-core/src/aiconfigurator_core/systems/data/l40s/mla_bmm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/quantize/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/vllm/0.22.0/reuse.yamlcollector/evidence_policy.yamltests/unit/sdk/database/test_database_helpers.pyaic-core/src/aiconfigurator_core/systems/data/l40s/attention/trtllm/1.3.0rc15/reuse.yamltests/unit/sdk/database/test_data_loaders.pyaic-core/src/aiconfigurator_core/systems/data/l40s/attention/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/mla/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/attention/sglang/0.5.12/reuse.yamldocs/perf_database/collector-v3-op-centric-design.mdtests/unit/sdk/test_perf_database_shared_layer.pytools/perf_database/evidence_check.pytests/unit/sdk/database/test_reuse_ordering.pyaic-core/src/aiconfigurator_core/sdk/perf_database.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.py
⚙️ 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:
aic-core/src/aiconfigurator_core/systems/data/l40s/mla_bmm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/moe/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/quantize/trtllm/1.3.0rc15/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/gemm/vllm/0.22.0/reuse.yamlcollector/evidence_policy.yamltests/unit/sdk/database/test_database_helpers.pyaic-core/src/aiconfigurator_core/systems/data/l40s/attention/trtllm/1.3.0rc15/reuse.yamltests/unit/sdk/database/test_data_loaders.pyaic-core/src/aiconfigurator_core/systems/data/l40s/attention/vllm/0.22.0/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/mla/sglang/0.5.12/reuse.yamlaic-core/src/aiconfigurator_core/systems/data/l40s/attention/sglang/0.5.12/reuse.yamldocs/perf_database/collector-v3-op-centric-design.mdtests/unit/sdk/test_perf_database_shared_layer.pytools/perf_database/evidence_check.pytests/unit/sdk/database/test_reuse_ordering.pyaic-core/src/aiconfigurator_core/sdk/perf_database.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.py
{collector/**,tests/unit/collector/**}
📄 CodeRabbit inference engine (.claude/rules/collector/failure_handling.md)
{collector/**,tests/unit/collector/**}: In collector failure handling, observe failures rather than predict them: do not use a declarative expected-failure layer or automatic skips; failing groups must be fixed.
Record every worker failure automatically in the relevant error and collection-summary JSON files with classification, case parameters, exception details, and model/dtype grouping.
Reset a worker after CUDA-fatal errors only after recording the failed task.
Treat missing data points as tolerable downstream because the SDK can interpolate, extrapolate, reuse sibling-version rows, or use HYBRID empirical estimates.
For a hanging case or one that kills the node, add a dated denylist.yaml entry with a reason.
For an entirely unvalidated operation/backend pair, mark the registry OpEntry as unverified=true.
For cases not validated on a specific SM, mark the registry OpEntry with unverified_sms=(sm,).
Represent physically impossible hardware capabilities with a positive floor in capabilities.yaml; do not use it for framework-version kernel gaps.
Treat out-of-memory failures as unclassified until reproduced on a clean GPU; only a genuine OOM may be excluded by the sanctioned generation-time memory filter.
Fix proven collector-code bugs in code rather than using skips, and re-check dispatch and skip rules.
Do not encode framework-version gaps in YAML; allow them to fail and be re-tested after version changes.
Treat isolated, explained, unclustered failures as acceptable; investigate around 10% unexpected failures or sooner when failures cluster.
Treat roughly one-third failing cases or an entire failing family as a systemic collector problem; stop collection and fix the collector.
Never hide failures with broad skips, retries, generic OOM labels, reduced coverage, synthetic rows, or weakened benchmarks.
Compare failure records with the previous run for the same backend and version; prioritize only new failure groups.
Do not mechanically translate failure records in...
Files:
collector/evidence_policy.yaml
collector/**
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Review the collector when generated configuration formats change or when generator parameter names used in benchmark configurations change.
collector/**: Keep collector-layer rules in their permitted locations: base operation YAML for sweep parameters and cited quant-mode SM limits; model case YAML for structural shapes and policies; capabilities YAML for positive hardware dtype/op minimum-SM floors; backend registries for version routes and unverified markers; collectors for dispatch, classified runtime errors, and memory-feasibility generation filters; and the denylist only for hangs or node-killing cases.
Do not silently skip queued cases in collector code. A queued case must be executed or produce a classified exception; only generation-time capability floors, registry unverified markers, and the hang denylist may prevent execution.
Use framework-selected kernel/backend dispatch for measurements. Prefer the framework builder or selector; manually pin a backend only with a pinned-version file-and-line source citation; record the actually invoked kernel inkernel_source; and re-verify pins andFIXME(kernel-limit)notes on framework version bumps.
Do not invent backend fallbacks or swap backends in exception handlers. If the framework-selected path cannot be constructed or invoked, raise a classified error. Replicate fallback chains only when the framework itself uses them, and record the actual path.
The only sanctioned in-collector filter is memory feasibility: perform it only during generation insideget_*_test_cases(), use size-versus-capacity arithmetic based on live device memory or an approved capacity fallback, never use framework-version or model-name predicates, and log dropped counts in the required format.
Keep framework kernel limits asFIXME(kernel-limit)comments at the owning collector invocation site, including the claimed limit, origin, and unverified status. Do not encode unverified limits in YAML; affected cases should fail at runtime...
Files:
collector/evidence_policy.yaml
⚙️ CodeRabbit configuration file
collector/**: - Enforce the collector rules from.claude/rules/collector/layer_permissions.md,failure_handling.md, andcase_authoring.md.
- Flag any silent case skip in collector code (a queued case may only execute or raise); the sole sanctioned filter is generation-time memory feasibility with counted drops.
- Flag invented fallbacks on both ends: generation must raise on unresolvable declarations (never substitute defaults or another model's geometry); collectors must never swap in a different backend/kernel than the framework's own dispatch selects — manual pins require framework source citations.
- Flag any reintroduction of selector/exception machinery (case_ids/contains/indices/ranges/limit/rules, sm_exceptions-style shape or version predicates) in YAML or code.
- Capability floors (
cases/capabilities.yaml) may hold hardware facts only: no shapes, no framework versions, no per-backend nesting.cases/denylist.yamlis for hang/node-killers only, dated.- Collector changes must stay within
collector/andtests/unit/collector/; flag producer+consumer contract changes (perf row schema, PerfFile names) unless the PR explicitly declares them.- Check collector changes for backend/runtime version accuracy, GPU resource assumptions, reproducible command construction, and clear failure evidence.
- Flag changes that make support-matrix or perf-data results harder to trace back to the command, model, system, quantization, or runtime version that produced them.
Files:
collector/evidence_policy.yaml
collector/**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
Changes under
collector/must follow.claude/rules/collector/layer_permissions.mdandfailure_handling.md; case YAML work must also followcase_authoring.md. Do not apply generator rules tocollector/case_generator.py.
Files:
collector/evidence_policy.yaml
collector/**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Before modifying case YAML files under
collector/**, read.claude/rules/collector/case_authoring.md.
Files:
collector/evidence_policy.yaml
.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
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/unit/sdk/database/test_database_helpers.pytests/unit/sdk/database/test_data_loaders.pytests/unit/sdk/test_perf_database_shared_layer.pytests/unit/sdk/database/test_reuse_ordering.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
tests/unit/sdk/database/test_database_helpers.pytests/unit/sdk/database/test_data_loaders.pytests/unit/sdk/test_perf_database_shared_layer.pytools/perf_database/evidence_check.pytests/unit/sdk/database/test_reuse_ordering.pyaic-core/src/aiconfigurator_core/sdk/perf_database.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.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/unit/sdk/database/test_database_helpers.pytests/unit/sdk/database/test_data_loaders.pytests/unit/sdk/test_perf_database_shared_layer.pytests/unit/sdk/database/test_reuse_ordering.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.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/unit/sdk/database/test_database_helpers.pytests/unit/sdk/database/test_data_loaders.pytests/unit/sdk/test_perf_database_shared_layer.pytests/unit/sdk/database/test_reuse_ordering.pytests/unit/tools/test_evidence_check.pytests/unit/sdk/database/test_strict_mode.py
docs/**
⚙️ CodeRabbit configuration file
docs/**: - Check that docs match changed CLI, SDK, generator, backend, and support-matrix behavior.
- Flag docs that describe unsupported runtimes, stale command names, or behavior not covered by tests or support-matrix evidence.
Files:
docs/perf_database/collector-v3-op-centric-design.md
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/perf_database.py
🪛 ast-grep (0.44.1)
tests/unit/tools/test_evidence_check.py
[error] 479-485: Command coming from incoming request
Context: subprocess.run(
["git", "rev-parse", "--is-inside-work-tree"],
cwd=REPO_ROOT,
capture_output=True,
text=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🔇 Additional comments (26)
collector/evidence_policy.yaml (2)
14-79: LGTM!
4-12: 📐 Maintainability & Code QualityNo module-boundary issue here.
collector/evidence_policy.yamlis a shared policy consumed bytools/perf_database/evidence_check.py, and the tests belong with that tool undertests/unit/tools/; nothing needs to move intocollector/.> Likely an incorrect or invalid review comment.tools/perf_database/evidence_check.py (1)
32-96: LGTM!Also applies to: 102-128, 154-224, 264-286, 307-393
tests/unit/tools/test_evidence_check.py (1)
23-224: LGTM!Also applies to: 242-412, 425-505
aic-core/src/aiconfigurator_core/sdk/perf_database.py (1)
650-686: LGTM!Also applies to: 694-722, 726-839, 848-848, 873-879, 896-907, 918-928, 940-941, 974-975, 1051-1051, 1064-1065, 1075-1075, 1239-1239, 1272-1272, 1803-1911, 1964-1964, 1981-1994, 2010-2017, 2181-2308, 2326-2326, 2347-2354, 2363-2363
aic-core/src/aiconfigurator_core/systems/data/l40s/gemm/vllm/0.22.0/reuse.yaml (1)
8-8: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/mla/sglang/0.5.12/reuse.yaml (1)
8-12: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/mla_bmm/sglang/0.5.12/reuse.yaml (1)
8-8: LGTM!tests/unit/sdk/database/test_data_loaders.py (1)
58-58: LGTM!docs/perf_database/collector-v3-op-centric-design.md (1)
328-333: LGTM!Also applies to: 344-347, 415-424
aic-core/src/aiconfigurator_core/systems/data/l40s/attention/sglang/0.5.12/reuse.yaml (1)
8-12: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/moe/sglang/0.5.12/reuse.yaml (1)
8-8: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/moe/trtllm/1.3.0rc15/reuse.yaml (1)
8-8: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/moe/vllm/0.22.0/reuse.yaml (1)
8-8: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/quantize/trtllm/1.3.0rc15/reuse.yaml (1)
8-12: LGTM!tests/unit/sdk/test_perf_database_shared_layer.py (2)
389-390: 🩺 Stability & AvailabilityNo issue here —
_write_gemm_csvcreatespath.parentbefore writing, so the newvllm/0.5CSV is created safely.> Likely an incorrect or invalid review comment.
372-390: 📐 Maintainability & Code QualityRun
pytest -m unitin an environment withpytestinstalled.aic-core/src/aiconfigurator_core/systems/data/l40s/attention/trtllm/1.3.0rc15/reuse.yaml (1)
8-12: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/attention/vllm/0.22.0/reuse.yaml (1)
8-12: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/gemm/sglang/0.5.12/reuse.yaml (1)
8-8: LGTM!aic-core/src/aiconfigurator_core/systems/data/l40s/gemm/trtllm/1.3.0rc15/reuse.yaml (1)
8-8: LGTM!tests/unit/sdk/database/test_reuse_ordering.py (2)
1-33: LGTM!Also applies to: 35-489
34-34: 📐 Maintainability & Code QualityRun the unit tests for both changed modules.
tests/unit/sdk/database/test_reuse_ordering.pyandtests/unit/sdk/database/test_strict_mode.pystill need to be exercised withpytest -m unit..github/workflows/build-test.yml (1)
98-100: LGTM!tests/unit/sdk/database/test_strict_mode.py (2)
1-142: LGTM!Also applies to: 159-600
143-157: 🗄️ Data Integrity & IntegrationNo issue:
PerfDatabaseconstruction intentionally skips provenance checks.> Likely an incorrect or invalid review comment.
| thresholds = raw.get("thresholds") | ||
| threshold_value = thresholds.get("parquet_diff_median_pct") if isinstance(thresholds, dict) else None | ||
| if not isinstance(threshold_value, (int, float)) or isinstance(threshold_value, bool): | ||
| raise EvidencePolicyError(f"evidence policy 'thresholds.parquet_diff_median_pct' must be a number: {path}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject negative and non-finite thresholds.
-1, NaN, and infinity currently pass validation. In particular, NaN can prevent the documented escalation comparison from ever succeeding.
Proposed fix
import argparse
+import math
import sys- if not isinstance(threshold_value, (int, float)) or isinstance(threshold_value, bool):
- raise EvidencePolicyError(f"evidence policy 'thresholds.parquet_diff_median_pct' must be a number: {path}")
+ if (
+ not isinstance(threshold_value, (int, float))
+ or isinstance(threshold_value, bool)
+ or not math.isfinite(threshold_value)
+ or threshold_value < 0
+ ):
+ raise EvidencePolicyError(
+ f"evidence policy 'thresholds.parquet_diff_median_pct' must be a finite non-negative number: {path}"
+ )📝 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.
| thresholds = raw.get("thresholds") | |
| threshold_value = thresholds.get("parquet_diff_median_pct") if isinstance(thresholds, dict) else None | |
| if not isinstance(threshold_value, (int, float)) or isinstance(threshold_value, bool): | |
| raise EvidencePolicyError(f"evidence policy 'thresholds.parquet_diff_median_pct' must be a number: {path}") | |
| thresholds = raw.get("thresholds") | |
| threshold_value = thresholds.get("parquet_diff_median_pct") if isinstance(thresholds, dict) else None | |
| if ( | |
| not isinstance(threshold_value, (int, float)) | |
| or isinstance(threshold_value, bool) | |
| or not math.isfinite(threshold_value) | |
| or threshold_value < 0 | |
| ): | |
| raise EvidencePolicyError( | |
| f"evidence policy 'thresholds.parquet_diff_median_pct' must be a finite non-negative number: {path}" | |
| ) |
🤖 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 `@tools/perf_database/evidence_check.py` around lines 97 - 100, Update the
thresholds.parquet_diff_median_pct validation in the evidence policy parsing
flow to reject negative values and non-finite numbers such as NaN and infinity,
while continuing to accept finite, non-negative numeric values and reject
booleans. Use the existing threshold validation branch and an appropriate
numeric-finiteness check.
There was a problem hiding this comment.
Fixed in a1f9fd6: policy load now fails closed (EvidencePolicyError) unless the threshold is finite and non-negative — -1, NaN, and inf each have a red-verified regression test.
| if reason == "case_plan": | ||
| evidence_systems: tuple[str, ...] = () | ||
| else: | ||
| touched = _touched_generations(entry, policy) | ||
| missing = touched - policy.evidence_systems.keys() | ||
| if missing: | ||
| raise EvidencePolicyError( | ||
| f"changed_ops entry ({entry.framework}/{entry.family}) touches SM generation(s) " | ||
| f"{sorted(missing)} with no representative in evidence policy 'evidence_systems'" | ||
| ) | ||
| evidence_systems = tuple(sorted(policy.evidence_systems[generation] for generation in touched)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate system mappings for case_plan entries too.
case_plan returns before _touched_generations, so an unmapped system produces a successful report despite the documented fail-closed contract.
tools/perf_database/evidence_check.py#L295-L305: call_touched_generationsbefore branching; only representative selection should be skipped forcase_plan.tests/unit/tools/test_evidence_check.py#L232-L239: parameterize the unmapped-system regression across all three reasons.
+ touched = _touched_generations(entry, policy)
if reason == "case_plan":
evidence_systems: tuple[str, ...] = ()
else:
- touched = _touched_generations(entry, policy)
missing = touched - policy.evidence_systems.keys()📝 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.
| if reason == "case_plan": | |
| evidence_systems: tuple[str, ...] = () | |
| else: | |
| touched = _touched_generations(entry, policy) | |
| missing = touched - policy.evidence_systems.keys() | |
| if missing: | |
| raise EvidencePolicyError( | |
| f"changed_ops entry ({entry.framework}/{entry.family}) touches SM generation(s) " | |
| f"{sorted(missing)} with no representative in evidence policy 'evidence_systems'" | |
| ) | |
| evidence_systems = tuple(sorted(policy.evidence_systems[generation] for generation in touched)) | |
| touched = _touched_generations(entry, policy) | |
| if reason == "case_plan": | |
| evidence_systems: tuple[str, ...] = () | |
| else: | |
| missing = touched - policy.evidence_systems.keys() | |
| if missing: | |
| raise EvidencePolicyError( | |
| f"changed_ops entry ({entry.framework}/{entry.family}) touches SM generation(s) " | |
| f"{sorted(missing)} with no representative in evidence policy 'evidence_systems'" | |
| ) | |
| evidence_systems = tuple(sorted(policy.evidence_systems[generation] for generation in touched)) |
📍 Affects 2 files
tools/perf_database/evidence_check.py#L295-L305(this comment)tests/unit/tools/test_evidence_check.py#L232-L239
🤖 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 `@tools/perf_database/evidence_check.py` around lines 295 - 305, Validate
touched generations before branching on reason in the evidence-check flow:
update tools/perf_database/evidence_check.py lines 295-305 so
_touched_generations and unmapped-system validation run for case_plan entries,
while representative selection remains skipped only for case_plan. Parameterize
the unmapped-system regression in tests/unit/tools/test_evidence_check.py lines
232-239 across all three reasons.
There was a problem hiding this comment.
Confirmed and fixed in a1f9fd6 with exactly the hoist proposed here: _touched_generations now runs before the reason branch, so unmapped systems fail closed for all three reasons; only the representative selection stays skipped for case_plan. The unmapped-system regression test is parameterized across pin_version / collector_code / case_plan (the case_plan case verified red pre-fix — it previously produced a successful report).
…r fallback, comm exclusion, source diagnostics (AIC-1503) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…yout comm exception (AIC-1503) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…cy-tier grace (AIC-1503) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…ict mode (AIC-1503) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…1503) Widened databases_cache key from 3 to 4 elements to include strict_provenance bool. Updated hardcoded cache keys in test_database_helpers.py and added strict_provenance attribute to DummyPerfDatabase stub in test_data_loaders.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
Rewrites the `reason` field on the 17 l40s reuse.yaml entries (12 files) whose declaring dir already holds native primary data for the same table they declare a donor for. Carried in from PR3 Task 3's mechanical SHARED_LAYER_REUSE.txt migration (all 22 l40s reuse.yaml entries shared one generic "migrated from ..." reason); this PR's ordering flip (design §6) makes the self-overlap distinction load-bearing, since these declarations now only matter for missing-shape forward-fill, not whole- table fill, and should read as such rather than implying full-file reuse. New reason (verbatim): "self-overlap: dir holds native data for this table; declaration covers missing-shape forward-fill only; mechanically derived from legacy SHARED_LAYER_REUSE.txt (newest-first effective donor); NOT kernel-identity-reviewed" Self-overlap detected mechanically: entry's declaring dir has its own `<table>.parquet` file alongside reuse.yaml. The other 5 entries in the same 22-file set (pure gap-fill: encoder_attention x2, linear_attention x2, mhc x1, mla/trtllm x2, mla/vllm x4, quantize/sglang x2, quantize/vllm x2 -- dirs with zero native data for the declared table) are untouched. Classification table (file -> table(s) touched): attention/sglang/0.5.12 context_attention_perf, generation_attention_perf attention/trtllm/1.3.0rc15 context_attention_perf, generation_attention_perf attention/vllm/0.22.0 context_attention_perf, generation_attention_perf gemm/sglang/0.5.12 gemm_perf gemm/trtllm/1.3.0rc15 gemm_perf gemm/vllm/0.22.0 gemm_perf mla/sglang/0.5.12 context_mla_perf, generation_mla_perf mla_bmm/sglang/0.5.12 mla_bmm_perf moe/sglang/0.5.12 moe_perf moe/trtllm/1.3.0rc15 moe_perf moe/vllm/0.22.0 moe_perf quantize/trtllm/1.3.0rc15 computescale_perf, scale_matrix_perf PR4 T3 prediction-regression-gate adjudication (full detail in .superpowers/sdd/pr4-task-3-report.md, not committed): the gate has no committed snapshot/expected-file state (pure old-vs-new, recomputed each run per design doc §2.1) -- there is no code artifact for this task to regenerate. Ran it for real anyway (git worktree at PR3 head 0d3375e as "old" baseline vs this branch as "new"): tier-1 (collect_static.py / grid.py) is 100% non-functional against the real committed tree on BOTH revisions -- a pre-existing, unrelated bug (grid.py's directory walk still assumes the pre-family-layout <backend>/<version> tree; the real tree has been <family>/<backend>/<version> since PR2 Task 5, commit 98bf1d9, before PR3). Out of this task's scope to fix; recommend a follow-up ticket. Ran tier-2 (16 curated configs) and the silicon/accuracy layer (35 curated anchors) directly instead: tier-2 shows zero divergences; silicon shows 2, both category (a) undeclared-forward-fill-removal for a historical version (design §6.2, intended) -- b200_sxm/sglang/0.5.9 moe lookup (OK -> PerfDataNotAvailableError, verified via traceback + dir listing) and h100_sxm/sglang/0.5.6.post2 quantize (TTFT drift, verified via PerfDatabase.data_provenance showing zero admitted sources on the new side for computescale_perf/scale_matrix_perf, where old newest-first merge pulled from a newer sibling). Zero divergences traced to comm exclusion (b) or same-backend kernel_source-filter widening (c) -- not a BLOCKED finding. Verification: audit_collector_data.py all 6 rules OK; get_supported_databases 10 systems / 89 versions unchanged; tests/unit/sdk + tests/unit/tools -m unit -n 2 -> 1531 passed, 1 known pre-existing torch-collection error (test_moe_dispatch.py, unrelated). Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…C-1219) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…tions (AIC-1219) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…ed, record generation-map policy (AIC-1503, AIC-1219) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…pe, doc alignment, resolver test gap (AIC-1503) Signed-off-by: Yiming Liu <yimingl@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
Same container condition as the changed_ops smoke test on the base branch: /workspace in the CI test image is a source COPY without .git, so compute_changed_ops(REPO_ROOT, HEAD, HEAD) exits 128 there. Gate the one real-repo integration test on the checkout being a git worktree; the fixture-based resolver tests are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…dirs, validate strict cache hits (AIC-1503) Three admission/strictness gaps from CodeRabbit review of PR #1374 (one posted on #1371 but owned by this PR's admission layer), plus a test robustness fix: - _build_op_sources: resolve_op_data_path skips partial FAMILY dirs but its legacy-layout fallback returns an existing file with no partial check, and the primary was then admitted unconditionally. The admission chokepoint now refuses an existing primary whose version dir is partial-for-reuse (collection_meta.yaml status: partial, or legacy INCOMPLETE.txt), logs a warning naming the dir, and lets channels 2-4 fill; data_provenance keeps listing admitted sources only. This can only ever fire for the legacy-layout fallback path -- pinned by a scope-guard test. - _version_dir_data_filenames swallowed every os.listdir failure, so strict coverage silently accepted an unreadable version dir. A missing dir stays a legitimate no-files answer; any other OSError now raises ValueError in strict mode and warns once in non-strict, inside _check_strict_provenance_coverage. - get_database returned strict cache hits before request validation, handing back worker-imported instances _store_loaded_database inserts under a strict key without ever validating. Strict cache hits now run _check_strict_provenance_for_request first, memoized per (sorted primary paths, backend) so repeated hits stay cheap; the memo clears with unload_database. - test_fallback_emits_warning: the lazy GEMM.load_data (triggered by _gemm_lookup) emits the low-fidelity warning, so the lookup and the warning assertions now run inside the caplog.at_level block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
…us generations, malformed list items (AIC-1219) Four fail-closed gaps in the evidence resolver, from CodeRabbit review of PR #1374: - thresholds.parquet_diff_median_pct accepted -1, NaN, and infinity; NaN in particular makes the escalation comparison never succeed. Policy load now requires a finite non-negative number. - a system listed under two generations in system_generations silently matched BOTH owning generations in _touched_generations. Policy load now enforces unique ownership, naming the system and both generations. - _parse_changed_entry accepted non-string items inside tables/systems lists (e.g. tables: [{}]), deferring the failure to an uncaught TypeError in deduplication/generation lookup. Element types are now validated at manifest load. - the case_plan branch in _requirement_for returned before _touched_generations, so an unmapped system in a case_plan-only entry bypassed the documented fail-closed check. The mapping validation now runs for every reason; only representative selection stays skipped for case_plan. The unmapped-system regression test is parameterized across all three reasons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Yiming Liu <yimingl@nvidia.com>
Arsene12358
force-pushed
the
yimingl/aic-1502-provenance-reuse
branch
from
d641c8e to
ae56b01
Compare
Arsene12358
force-pushed
the
yimingl/aic-1503-ordering-evidence
branch
from
42ad681 to
a1f9fd6
Compare
1 participant
PR 4 of 4 — the final PR of the Collector V3: op-centric collector management project (Linear: AIC-1503 + AIC-1219), implementing design §6 ordering, §7 strict mode + diagnostics, and §9 evidence policy. Stacked on #1372; merge order #1370 → #1371 → #1372 → this.
What this does
_build_op_sources): sources per op are now primary →reuse.yaml-declared donors (any direction, file order, deduped) → same-backend nearest-earlier siblings (newer-than-requested is never admitted implicitly) → cross-backend kernel-gated fill (unchanged mechanism). Thecommfamily is hard-excluded from all reuse regardless of declarations (rule 5 — a deliberate tightening forcustom_allreduce/trtllm_alltoall/wideep_deepep_*). Each admitted source is channel-tagged and exposed asPerfDatabase.data_provenance.strict_provenanceparam /AIC_STRICT_PROVENANCEenv (ON in CI, off for users this release). At load time, fail-closed on missing/uncovered/malformed sidecars for the requested version's dirs and declared donors;provenance: legacywarns (one-release grace); non-strict warns and degrades gracefully (including query-time resilience to malformed declarations). The database cache is keyed by the strict bool, so strict calls are deterministic.collector/evidence_policy.yaml(thresholds; a full-fleet authoredsystem_generationsmap — SM103 Ultra folded into blackwell as a policy decision — with one evidence representative per generation) +tools/perf_database/evidence_check.py, the pure deterministic resolver from the changed-ops manifest to required evidence, filtered to the SM generations the change actually touches (unmapped systems fail closed; combined reasons emit the union — evidence can't be dodged by bundling). Exception waivers (evidence_exceptions.yaml, approver + expiry) are applied by the AIC-1214 gate, not the resolver — expiry needs a clock, which would break resolver purity.Disclosures for reviewers
data_provenance: entries mean admitted sources, not rows actually used (first-wins merge decides usage); keys are op-file basenames (gemm_perf.parquet) — two tables sharing one file share one entry; entries exist for every op the engine touches, including ops a given model never queries.reasonfields disclosing they are mechanical migrations (missing-shape forward-fill only, not kernel-identity-reviewed). Under the new ordering they are consumed as declared donors after primary — behavior-preserving vs the legacy newest-first semantics.Test plan
uv run pytest tests/unit -m unit -n 2green, and again withAIC_STRICT_PROVENANCE=1(identical results) · ordering suite (15), strict suite (32+), evidence resolver (30+) all new · real-tree audit 6/6 OK · discovery parity 10 systems / 89 pairs unchanged · gate tier-2 + silicon adjudicated with root-cause traces viadata_provenance· every fix wave re-reviewed.Closes the Collector V3 implementation: with this merged, a quarterly pin bump computes its own work list (
changed_ops), the evidence resolver prices it, the audit + strict mode keep every byte provenanced, andcollector-snapshot-2026-08tags the first reproducible quarter.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor