[WS1][C3] Land forward config-invariance harness and backend provenance - #296
[WS1][C3] Land forward config-invariance harness and backend provenance#296maxiaosong1124 wants to merge 10 commits into
Conversation
Freeze the WS1 numerical SSOT for issue RL-Align#267: four-judgment tolerance rows, dtype/TF32/FP8 policy, comparison roles, chain logprob aggregates, shared resolver, and op_checks wiring so forward and gradient accuracy no longer share one threshold path. Add schema tests, usage docs, and a migration checklist for remaining private-atol call sites (C3/C4/C8). Closes RL-Align#267
Record acceptance-criteria mapping, verification commands, and residual scope so issue RL-Align#267 can close without implying full RL-Align#266 exit.
Freeze the full Qwen3-8B Dense logical workload SSOT for WS1 closeout C2: manifest pins (config fingerprint, weight content hash, 2x2 Batch/Chunk matrix, varlen fixtures, packing, dual backend profiles, representative case_ids), logical identity restore after pad/pack/chunk, singleton_aggregate vs BN multiset plan, registry-resolved candidate binding, and a single reference command. Document registry-vs-runtime actual boundary and Triton missing_required reds without silent fallback. Closes RL-Align#268
Add the shared forward accuracy/invariance API, C2 config matrix, backend provenance fail-closed checks, selected-logprob smoke, GPU gate CLI, CPU tests, and closeout evidence for WS1 C3.
|
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: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds WS1 workload definitions, a versioned tolerance contract, forward configuration-invariance checks, backend provenance validation, GPU evidence CLIs, documentation, and CPU/GPU test coverage. ChangesWS1 validation infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Fixed issue severity: Medium Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (11)
rl_engine/testing/ws1_manifest.json (1)
417-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the unused
seq_lenonrepresentative_full_model_fixture.
_validate_fixture_case_bindingsderives this fixture's expected shapes fromfixtures.samples(primary_total_tokens=59,primary_max_seq=19), not from theseq_lendeclared here. The declaredseq_len: 16andprompt_len: 8are never consumed, andfixture_id: "rep_full_model_seq16"conflicts with the bound case IDs that pinsq19.Consider removing the unused scalars, or renaming the fixture to reflect the varlen sample set it actually represents.
🤖 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 `@rl_engine/testing/ws1_manifest.json` around lines 417 - 427, Clarify the representative_full_model_fixture metadata by removing the unused seq_len and prompt_len fields and renaming fixture_id from rep_full_model_seq16 to reflect the variable-length samples bound by fixtures.samples, including the sq19 case.docs/design/ws1-c2-268-workload-plan.md (2)
63-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
scripts/ws1_candidate_evidence.pyto the deliverables table.The manifest pins
runtime_evidence_commandvalues that invokescripts/ws1_candidate_evidence.pyfor every representative case, anddocs/design/ws1-c2-268-closeout-evidence.mdline 14 lists it as a deliverable. This table omits it.🤖 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 `@docs/design/ws1-c2-268-workload-plan.md` around lines 63 - 69, Add scripts/ws1_candidate_evidence.py as a deliverable row in the table, describing its role as the candidate runtime evidence command referenced by the manifest and closeout evidence document.
143-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the API sketch with the implemented signatures.
Several entries do not match
rl_engine/testing/ws1_workload.py:
apply_padding(batch)requires the keyword-onlypad_side.apply_chunking(batch)requires the keyword-onlychunk_size.profile_required_nodes(profile_id)isprofile_required_nodes(manifest=None, profile_id="cuda_bf16").get_case(case_id)isget_case(manifest=None, case_id="").The section is titled "Workload API (Python)", so readers will treat it as normative.
🤖 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 `@docs/design/ws1-c2-268-workload-plan.md` around lines 143 - 154, Update the “Workload API (Python)” sketch to match the implemented signatures: add keyword-only pad_side to apply_padding, keyword-only chunk_size to apply_chunking, and manifest=None before profile_id in profile_required_nodes and before case_id in get_case, preserving their documented defaults.scripts/ws1_candidate_evidence.py (1)
143-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute the resolved case ID set once.
{case["case_id"] for case in cases}is built twice, at line 149 and line 150.♻️ Proposed refactor
- if selected_ids - {case["case_id"] for case in cases}: - unknown = sorted(selected_ids - {case["case_id"] for case in cases}) + resolved_ids = {case["case_id"] for case in cases} + if selected_ids - resolved_ids: + unknown = sorted(selected_ids - resolved_ids) raise WorkloadError(f"unknown or profile-filtered case IDs: {unknown}")🤖 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 `@scripts/ws1_candidate_evidence.py` around lines 143 - 151, In the case-selection flow, compute the resolved case ID set once after building cases, store it in a local variable, and reuse it for the unknown/profile-filtered ID check and sorting in the WorkloadError branch. Update the surrounding logic without changing its filtering or error behavior.tests/test_ws1_workload.py (1)
308-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead
chunk_sizefrom the manifest instead of hardcoding7.Line 310 pins
chunk_size=7and line 317 asserts the last segment length is5. Both derive fromprimary_matrix.chunk.chunk_size_tokensandprimary_seq_len. If the manifest pin changes, this test asserts a stale expectation.test_chunk_positions_cover_logical_keysat line 297 already reads the value from the manifest.♻️ Proposed refactor
def test_chunk_and_pack_layouts_restore_identity(manifest): batch = build_logical_batch(manifest) - chunked = apply_chunking(batch, chunk_size=7) + chunk_size = int(manifest.primary_matrix["chunk"]["chunk_size_tokens"]) + chunked = apply_chunking(batch, chunk_size=chunk_size) packed = apply_packing(batch) for layout in (chunked, packed): values = [f"{sid}@{pos}" for sid, pos in layout.restore_map] restored = restore_logical_order(layout, values) assert set(restored) == set(batch.logical_keys()) assert len(layout.physical_token_ids) == len(layout.restore_map) - assert chunked.segment_lengths[-1] == 5 + longest = max(sample.seq_len for sample in batch.samples) + assert chunked.segment_lengths[-1] == longest % chunk_size or chunk_size assert packed.segment_lengths == (11, 16, 13, 19)🤖 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/test_ws1_workload.py` around lines 308 - 318, Update test_chunk_and_pack_layouts_restore_identity to derive chunk_size from manifest.primary_matrix.chunk.chunk_size_tokens instead of hardcoding 7, and calculate the expected final chunk length from manifest.primary_seq_len and that chunk size instead of asserting 5. Keep the identity, token/map length, and packed segment assertions unchanged.tests/test_op_checks.py (1)
191-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParametrize the two provenance persistence tests.
test_ws1_report_persists_roles_and_backend_provenanceandtest_ws1_report_accepts_triton_backend_provenancediffer only inbackend_profile, the backend string, the candidate name, and the seed. Collapse them withpytest.mark.parametrizeso a future provenance field is added in one place.♻️ Proposed refactor
-def test_ws1_report_persists_roles_and_backend_provenance(): +@pytest.mark.parametrize( + ("backend_profile", "backend", "seed"), + [("cuda_bf16", "cuda", 12), ("triton_cuda_bf16", "triton", 15)], +) +def test_ws1_report_persists_roles_and_backend_provenance(backend_profile, backend, seed): provenance = BackendProvenance( - backend_profile="cuda_bf16", - requested_backend="cuda", - actual_backend="cuda", + backend_profile=backend_profile, + requested_backend=backend, + actual_backend=backend, execution_dtype="bfloat16", accumulation_dtype="float32", output_dtype="bfloat16", reference_dtype="float32", candidate_tf32_enabled=False, reference_tf32_enabled=False, ) report = run_operator_suite( "logp", candidates=[ CandidateSpec( - name="cuda-logp", - backend="cuda", + name=f"{backend}-logp", + backend=backend, fn=NativeLogpOp(), provenance=provenance, ) ], - cases=[_logp_case("bf16", torch.bfloat16, seed=12)], + cases=[_logp_case("bf16", torch.bfloat16, seed=seed)], ) output = report.candidates[0].cases[0].outputs[0] assert output.judgment == "forward_accuracy" assert output.comparison_lhs_role == "bf16_candidate" assert output.comparison_rhs_role == "fp32_reference" data = report.to_dict()["candidates"][0] - assert data["backend_provenance"]["actual_backend"] == "cuda" + assert data["backend_provenance"]["actual_backend"] == backend assert "baseline" not in data["cases"][0]["outputs"][0] - - -def test_ws1_report_accepts_triton_backend_provenance(): - ...🤖 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/test_op_checks.py` around lines 191 - 254, Parametrize the two provenance persistence tests into one test function using pytest.mark.parametrize, supplying the differing backend_profile, backend, candidate name, and seed as parameters. Reuse those parameters when constructing BackendProvenance, CandidateSpec, and the test case, while preserving all existing assertions and expected provenance behavior.tests/test_tolerance_contract.py (1)
152-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for silent and cross-profile backend fallback.
Lines 166-170 cover an
actual_backendmismatch only. The PR objective requires failing on undeclared or silent fallback and rejecting cross-profile fallback. Two cases are untested: arequested_backendmismatch, and a provenance whosebackend_profileiscuda_bf16while both backends reporttriton.validate_backend_provenancehandles both at Lines 269-277 ofrl_engine/kernels/gtest/tolerance.py, so these tests should pass as written.💚 Proposed additional coverage
with pytest.raises(ContractResolveError, match="candidate_tf32_enabled"): validate_backend_provenance( contract, BackendProvenance(**{**provenance.to_dict(), "candidate_tf32_enabled": True}), ) + with pytest.raises(ContractResolveError, match="requested_backend"): + validate_backend_provenance( + contract, + BackendProvenance(**{**provenance.to_dict(), "requested_backend": "triton"}), + ) + with pytest.raises(ContractResolveError, match="backend provenance mismatch"): + validate_backend_provenance( + contract, + BackendProvenance( + **{ + **provenance.to_dict(), + "requested_backend": "triton", + "actual_backend": "triton", + } + ), + )🤖 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/test_tolerance_contract.py` around lines 152 - 180, Extend test_backend_provenance_checks_profile_backend_and_all_dtypes with ContractResolveError cases for a requested_backend mismatch and for cuda_bf16 provenance where both requested and actual backends are triton. Match the relevant backend fields in each error and preserve the existing provenance validation cases.rl_engine/kernels/gtest/tolerance.py (2)
921-996: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare one alias table between the two branches.
The string branch at Lines 925-942 and the torch branch at Lines 952-969 define nearly identical mappings. A new dtype alias must be added in two places. Define one module-level table and normalize
str(dtype)through it in both branches.🤖 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 `@rl_engine/kernels/gtest/tolerance.py` around lines 921 - 996, Refactor _dtype_name to use one module-level dtype alias table for both string inputs and torch dtype values. Normalize string(dtype) through that shared mapping in the torch branch while preserving the existing torch identity checks and unsupported-dtype errors; remove the duplicated local mappings.
636-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject
require_all=falseinstead of weakening the verdict.
_validate_chain_aggregatesat Line 837 rejects any contract with a falsyrequire_all. Theany(...)branch is therefore unreachable for a validated contract. For an unvalidated contract the branch turns the gate into "pass if one metric passes", which contradicts the docstring at Line 605. Hard-fail instead.♻️ Proposed refactor
- require_all = bool(contract["chain_logprob_aggregates"].get("require_all", True)) - passed = all(m.passed for m in metrics) if require_all else any(m.passed for m in metrics) + if not bool(contract["chain_logprob_aggregates"].get("require_all", True)): + raise ContractResolveError("require_all must be true for chain logprob aggregates") + passed = all(m.passed for m in metrics)🤖 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 `@rl_engine/kernels/gtest/tolerance.py` around lines 636 - 637, Update the verdict logic in the function containing require_all and passed to reject any falsy chain_logprob_aggregates.require_all value instead of using an any(...) fallback. Preserve all(...) as the only valid metric aggregation behavior, and raise the established validation error consistently with _validate_chain_aggregates for unvalidated contracts.rl_engine/kernels/gtest/op_checks.py (2)
246-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
"judgments" in contractbranch duplicates the routing inside_resolve_tolerance.Line 248 tests
"judgments" in contract._resolve_tolerancetests the same condition at Line 509 and routes toresolve_toleranceitself. The outer branch exists only to keepgradient_specfor the role fields at Lines 280-285. The same pattern is repeated at Lines 314-365.The fallback roles at Lines 281-285 also hardcode
"bf16_candidate"and"fp32_reference", which duplicates the contract values intolerance_contract.json.🤖 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 `@rl_engine/kernels/gtest/op_checks.py` around lines 246 - 291, The gradient-check setup around _resolve_tolerance should use a single tolerance-resolution path instead of branching on "judgments" in contract; retain the resolved specification needed for comparison_lhs_role and comparison_rhs_role, adapting _resolve_tolerance or its caller as necessary. Remove the hardcoded fallback roles and source both roles from the resolved contract tolerance definition, applying the same consolidation to the repeated pattern in the later gradient-check block.
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPromote the shared dtype normalizer instead of importing a private symbol.
Line 13 imports
_dtype_namefromrl_engine/kernels/gtest/tolerance.py. That symbol is private and is not listed in the__all__oftolerance.py. This module also defines its own_dtype_nameat Line 537 with different behavior: it rejects strings and FP8 names, and it raisesValueErrorinstead ofContractResolveError. Two normalizers with near-identical names now coexist, and Lines 162-163 use one while Lines 340-341 use the other.Export a public normalizer from
tolerance.pyand use it in both places.🤖 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 `@rl_engine/kernels/gtest/op_checks.py` around lines 12 - 18, Promote tolerance.py’s shared _dtype_name implementation to a public dtype normalizer, add it to __all__, and import that public symbol in op_checks.py instead of the private alias. Remove the local _dtype_name definition in op_checks.py and update both call sites around lines 162-163 and 340-341 to use the shared normalizer, preserving its string/FP8 handling and ContractResolveError behavior.
🤖 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 `@rl_engine/kernels/gtest/forward_invariance.py`:
- Around line 524-532: Validate runtime-observed candidate provenance in
_validate_provenance and the metadata gate, including backend, kernel identity,
and output dtype, and fail reports on mismatches; add regression coverage in
tests/test_forward_invariance.py. In scripts/check_forward_invariance.py lines
215-225, populate actual_backend and kernel identity from candidate runtime
telemetry rather than the CLI candidate string. Update
docs/contributing/gtest-usage.md lines 298-303 to retain the claim only once
runtime backend and output-dtype validation exists, and revise
docs/design/ws1-c3-269-closeout-evidence.md lines 41-43 to remove or qualify the
closeout claim until selected-candidate fallback is detectable.
In `@rl_engine/kernels/gtest/op_checks.py`:
- Around line 338-351: Use the public dtype normalizer from tolerance.py
throughout op_checks.py. In rl_engine/kernels/gtest/op_checks.py lines 338-351,
normalize candidate.provenance.output_dtype and reference_dtype before comparing
them with observed dtype names; at lines 12-18, replace the private import with
the exported normalizer, and remove the local _dtype_name near line 537 so only
the shared implementation remains.
In `@rl_engine/kernels/gtest/tolerance.py`:
- Around line 553-560: In the validation flow around the lhs/rhs/mask tensor
conversions, compare the original input shapes before applying reshape(-1), and
raise ContractResolveError for any logical-shape mismatch. Keep the existing
flattened tensors for subsequent processing, while ensuring equal element counts
with different dimensions cannot pass validation.
In `@rl_engine/testing/ws1_workload.py`:
- Around line 703-714: Ensure manifest-derived failures consistently use the
CLIs’ handled exception contract: in rl_engine/testing/ws1_workload.py:703-714,
replace the direct expected_shapes indexing with guarded .get() lookups that
raise WorkloadError for unknown fixture_id or uncovered family; in
scripts/ws1_reference.py:81-93, catch WorkloadError, OSError, and
json.JSONDecodeError; in scripts/ws1_candidate_evidence.py:34-68, raise
WorkloadError from _case_args when a per-family shape key is missing, or
alternatively include KeyError in the handler at line 159.
- Around line 443-450: Update the cell validation loop around the
comparison_lhs_role check to also reject singleton_aggregate when it appears in
comparison_rhs_role. Apply the same validation rule to both comparison role
fields while preserving the existing batch_mode checks and WorkloadError
behavior.
In `@tests/test_ws1_workload.py`:
- Around line 472-476: Update test_missing_matrix_cell_rejected so its
pytest.raises match pattern escapes the dot in “primary_matrix.cells” as a
literal period, satisfying Ruff RUF043 while preserving the existing validation
assertion.
---
Nitpick comments:
In `@docs/design/ws1-c2-268-workload-plan.md`:
- Around line 63-69: Add scripts/ws1_candidate_evidence.py as a deliverable row
in the table, describing its role as the candidate runtime evidence command
referenced by the manifest and closeout evidence document.
- Around line 143-154: Update the “Workload API (Python)” sketch to match the
implemented signatures: add keyword-only pad_side to apply_padding, keyword-only
chunk_size to apply_chunking, and manifest=None before profile_id in
profile_required_nodes and before case_id in get_case, preserving their
documented defaults.
In `@rl_engine/kernels/gtest/op_checks.py`:
- Around line 246-291: The gradient-check setup around _resolve_tolerance should
use a single tolerance-resolution path instead of branching on "judgments" in
contract; retain the resolved specification needed for comparison_lhs_role and
comparison_rhs_role, adapting _resolve_tolerance or its caller as necessary.
Remove the hardcoded fallback roles and source both roles from the resolved
contract tolerance definition, applying the same consolidation to the repeated
pattern in the later gradient-check block.
- Around line 12-18: Promote tolerance.py’s shared _dtype_name implementation to
a public dtype normalizer, add it to __all__, and import that public symbol in
op_checks.py instead of the private alias. Remove the local _dtype_name
definition in op_checks.py and update both call sites around lines 162-163 and
340-341 to use the shared normalizer, preserving its string/FP8 handling and
ContractResolveError behavior.
In `@rl_engine/kernels/gtest/tolerance.py`:
- Around line 921-996: Refactor _dtype_name to use one module-level dtype alias
table for both string inputs and torch dtype values. Normalize string(dtype)
through that shared mapping in the torch branch while preserving the existing
torch identity checks and unsupported-dtype errors; remove the duplicated local
mappings.
- Around line 636-637: Update the verdict logic in the function containing
require_all and passed to reject any falsy chain_logprob_aggregates.require_all
value instead of using an any(...) fallback. Preserve all(...) as the only valid
metric aggregation behavior, and raise the established validation error
consistently with _validate_chain_aggregates for unvalidated contracts.
In `@rl_engine/testing/ws1_manifest.json`:
- Around line 417-427: Clarify the representative_full_model_fixture metadata by
removing the unused seq_len and prompt_len fields and renaming fixture_id from
rep_full_model_seq16 to reflect the variable-length samples bound by
fixtures.samples, including the sq19 case.
In `@scripts/ws1_candidate_evidence.py`:
- Around line 143-151: In the case-selection flow, compute the resolved case ID
set once after building cases, store it in a local variable, and reuse it for
the unknown/profile-filtered ID check and sorting in the WorkloadError branch.
Update the surrounding logic without changing its filtering or error behavior.
In `@tests/test_op_checks.py`:
- Around line 191-254: Parametrize the two provenance persistence tests into one
test function using pytest.mark.parametrize, supplying the differing
backend_profile, backend, candidate name, and seed as parameters. Reuse those
parameters when constructing BackendProvenance, CandidateSpec, and the test
case, while preserving all existing assertions and expected provenance behavior.
In `@tests/test_tolerance_contract.py`:
- Around line 152-180: Extend
test_backend_provenance_checks_profile_backend_and_all_dtypes with
ContractResolveError cases for a requested_backend mismatch and for cuda_bf16
provenance where both requested and actual backends are triton. Match the
relevant backend fields in each error and preserve the existing provenance
validation cases.
In `@tests/test_ws1_workload.py`:
- Around line 308-318: Update test_chunk_and_pack_layouts_restore_identity to
derive chunk_size from manifest.primary_matrix.chunk.chunk_size_tokens instead
of hardcoding 7, and calculate the expected final chunk length from
manifest.primary_seq_len and that chunk size instead of asserting 5. Keep the
identity, token/map length, and packed segment assertions unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 127615a4-c8bc-4467-b409-d9c16aae0cfd
📒 Files selected for processing (22)
.github/workflows/ci.ymldocs/contributing/gtest-usage.mddocs/contributing/testing.mddocs/design/ws1-c2-268-closeout-evidence.mddocs/design/ws1-c2-268-workload-plan.mddocs/design/ws1-c3-269-closeout-evidence.mdrl_engine/kernels/gtest/__init__.pyrl_engine/kernels/gtest/forward_invariance.pyrl_engine/kernels/gtest/op_checks.pyrl_engine/kernels/gtest/tolerance.pyrl_engine/kernels/gtest/tolerance_contract.jsonrl_engine/testing/__init__.pyrl_engine/testing/ws1_manifest.jsonrl_engine/testing/ws1_workload.pyscripts/check_forward_invariance.pyscripts/ws1_candidate_evidence.pyscripts/ws1_reference.pytests/test_forward_invariance.pytests/test_op_checks.pytests/test_tolerance_contract.pytests/test_ws1_candidate_evidence.pytests/test_ws1_workload.py
| provenance_valid = _validate_provenance(loaded_contract, provenance, backend_profile) | ||
| if not provenance_valid and fallback_reason is None: | ||
| fallback_reason = "missing or contract-invalid backend provenance" | ||
| metadata_valid = ( | ||
| candidate_id != "unspecified" | ||
| and device != "unspecified" | ||
| and compute_capability is not None | ||
| and fallback_reason is None | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Record and validate runtime-observed provenance.
The gate validates values that the caller declares. Line 218 derives actual_backend from --candidate, and the harness never validates observed candidate output dtypes. A CUDA candidate that silently routes to Triton or PyTorch, or returns FP32 output, can produce a passing BF16 CUDA report.
rl_engine/kernels/gtest/forward_invariance.py#L524-L532: validate observed candidate backend, kernel identity, and output dtype againstBackendProvenance; fail the report on a mismatch. Add regression coverage intests/test_forward_invariance.py.scripts/check_forward_invariance.py#L215-L225: populateactual_backendand kernel identity from candidate runtime telemetry, not from the CLI candidate string.docs/contributing/gtest-usage.md#L298-L303: keep this claim only after runtime backend and output-dtype validation exists.docs/design/ws1-c3-269-closeout-evidence.md#L41-L43: revise the closeout claim until the CLI can detect fallback inside a selected candidate.
📍 Affects 4 files
rl_engine/kernels/gtest/forward_invariance.py#L524-L532(this comment)scripts/check_forward_invariance.py#L215-L225docs/contributing/gtest-usage.md#L298-L303docs/design/ws1-c3-269-closeout-evidence.md#L41-L43
🤖 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 `@rl_engine/kernels/gtest/forward_invariance.py` around lines 524 - 532,
Validate runtime-observed candidate provenance in _validate_provenance and the
metadata gate, including backend, kernel identity, and output dtype, and fail
reports on mismatches; add regression coverage in
tests/test_forward_invariance.py. In scripts/check_forward_invariance.py lines
215-225, populate actual_backend and kernel identity from candidate runtime
telemetry rather than the CLI candidate string. Update
docs/contributing/gtest-usage.md lines 298-303 to retain the claim only once
runtime backend and output-dtype validation exists, and revise
docs/design/ws1-c3-269-closeout-evidence.md lines 41-43 to remove or qualify the
closeout claim until selected-candidate fallback is detectable.
| if candidate.provenance is not None: | ||
| for candidate_output, gold_output in zip(candidate_outputs, gold_outputs, strict=True): | ||
| candidate_dtype = _dtype_name(candidate_output.dtype) | ||
| gold_dtype = _dtype_name(gold_output.dtype) | ||
| if candidate_dtype != candidate.provenance.output_dtype: | ||
| raise ContractResolveError( | ||
| f"candidate output dtype {candidate_dtype!r} disagrees with provenance " | ||
| f"output_dtype {candidate.provenance.output_dtype!r}" | ||
| ) | ||
| if gold_dtype != candidate.provenance.reference_dtype: | ||
| raise ContractResolveError( | ||
| f"gold output dtype {gold_dtype!r} disagrees with provenance " | ||
| f"reference_dtype {candidate.provenance.reference_dtype!r}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
One dtype normalizer is private, so op_checks.py keeps a second one and applies it inconsistently. tolerance._dtype_name is private and absent from the __all__ of tolerance.py, so this module imports it as _normalize_dtype_name and also keeps a narrower local _dtype_name at Line 537. The two normalizers differ in accepted inputs and in raised error type, and the provenance dtype comparison normalizes only one side.
rl_engine/kernels/gtest/op_checks.py#L338-L351: normalizecandidate.provenance.output_dtypeandcandidate.provenance.reference_dtypethrough the shared normalizer before comparing them to the observed tensor dtype names. A provenance built with an accepted alias such as"bf16"currently passesvalidate_backend_provenanceat Line 155 and then fails here with a misleading dtype-disagreement error.rl_engine/kernels/gtest/op_checks.py#L12-L18: replace the private import at Line 13 with a public normalizer exported fromtolerance.py, and delete the local_dtype_nameat Line 537 so one implementation remains.
📍 Affects 1 file
rl_engine/kernels/gtest/op_checks.py#L338-L351(this comment)rl_engine/kernels/gtest/op_checks.py#L12-L18
🤖 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 `@rl_engine/kernels/gtest/op_checks.py` around lines 338 - 351, Use the public
dtype normalizer from tolerance.py throughout op_checks.py. In
rl_engine/kernels/gtest/op_checks.py lines 338-351, normalize
candidate.provenance.output_dtype and reference_dtype before comparing them with
observed dtype names; at lines 12-18, replace the private import with the
exported normalizer, and remove the local _dtype_name near line 537 so only the
shared implementation remains.
| lhs = torch.as_tensor(lhs_logp).detach().float().reshape(-1) | ||
| rhs = torch.as_tensor(rhs_logp).detach().float().reshape(-1) | ||
| mask = torch.as_tensor(active_mask).detach().reshape(-1).bool() | ||
| if lhs.shape != rhs.shape or lhs.shape != mask.shape: | ||
| raise ContractResolveError( | ||
| f"lhs/rhs/mask shape mismatch: {tuple(lhs.shape)} vs " | ||
| f"{tuple(rhs.shape)} vs {tuple(mask.shape)}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the original shapes before flattening.
lhs, rhs, and mask are reshaped to 1-D at Lines 553-555. The check at Line 556 therefore compares element counts only. Inputs with different logical shapes but equal element counts (for example (2, 3) and (3, 2)) pass and produce a silently misaligned dlogp. This module is the fail-closed authority for chain aggregates, so compare the shapes before the reshape.
🛡️ Proposed fix
- lhs = torch.as_tensor(lhs_logp).detach().float().reshape(-1)
- rhs = torch.as_tensor(rhs_logp).detach().float().reshape(-1)
- mask = torch.as_tensor(active_mask).detach().reshape(-1).bool()
- if lhs.shape != rhs.shape or lhs.shape != mask.shape:
+ lhs_t = torch.as_tensor(lhs_logp).detach().float()
+ rhs_t = torch.as_tensor(rhs_logp).detach().float()
+ mask_t = torch.as_tensor(active_mask).detach().bool()
+ if lhs_t.shape != rhs_t.shape or lhs_t.shape != mask_t.shape:
raise ContractResolveError(
- f"lhs/rhs/mask shape mismatch: {tuple(lhs.shape)} vs "
- f"{tuple(rhs.shape)} vs {tuple(mask.shape)}"
+ f"lhs/rhs/mask shape mismatch: {tuple(lhs_t.shape)} vs "
+ f"{tuple(rhs_t.shape)} vs {tuple(mask_t.shape)}"
)
+ lhs, rhs, mask = lhs_t.reshape(-1), rhs_t.reshape(-1), mask_t.reshape(-1)📝 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.
| lhs = torch.as_tensor(lhs_logp).detach().float().reshape(-1) | |
| rhs = torch.as_tensor(rhs_logp).detach().float().reshape(-1) | |
| mask = torch.as_tensor(active_mask).detach().reshape(-1).bool() | |
| if lhs.shape != rhs.shape or lhs.shape != mask.shape: | |
| raise ContractResolveError( | |
| f"lhs/rhs/mask shape mismatch: {tuple(lhs.shape)} vs " | |
| f"{tuple(rhs.shape)} vs {tuple(mask.shape)}" | |
| ) | |
| lhs_t = torch.as_tensor(lhs_logp).detach().float() | |
| rhs_t = torch.as_tensor(rhs_logp).detach().float() | |
| mask_t = torch.as_tensor(active_mask).detach().bool() | |
| if lhs_t.shape != rhs_t.shape or lhs_t.shape != mask_t.shape: | |
| raise ContractResolveError( | |
| f"lhs/rhs/mask shape mismatch: {tuple(lhs_t.shape)} vs " | |
| f"{tuple(rhs_t.shape)} vs {tuple(mask_t.shape)}" | |
| ) | |
| lhs, rhs, mask = lhs_t.reshape(-1), rhs_t.reshape(-1), mask_t.reshape(-1) |
🤖 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 `@rl_engine/kernels/gtest/tolerance.py` around lines 553 - 560, In the
validation flow around the lhs/rhs/mask tensor conversions, compare the original
input shapes before applying reshape(-1), and raise ContractResolveError for any
logical-shape mismatch. Keep the existing flattened tensors for subsequent
processing, while ensuring equal element counts with different dimensions cannot
pass validation.
| for cell in cells: | ||
| mode = cell["batch_mode"] | ||
| if mode not in ("singleton_aggregate", "batched"): | ||
| raise WorkloadError(f"unknown batch_mode {mode!r}") | ||
| if mode == "singleton_aggregate" and "singleton_aggregate" in str( | ||
| cell.get("comparison_lhs_role", "") | ||
| ): | ||
| raise WorkloadError("singleton_aggregate must not be used as a comparison role") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check comparison_rhs_role as well as comparison_lhs_role.
The manifest declares that singleton_aggregate must never populate comparison_lhs_role or comparison_rhs_role (ws1_manifest.json line 97). The cell validation only inspects comparison_lhs_role. A cell that sets comparison_rhs_role: "singleton_aggregate" passes validation.
🐛 Proposed fix
for cell in cells:
mode = cell["batch_mode"]
if mode not in ("singleton_aggregate", "batched"):
raise WorkloadError(f"unknown batch_mode {mode!r}")
- if mode == "singleton_aggregate" and "singleton_aggregate" in str(
- cell.get("comparison_lhs_role", "")
- ):
- raise WorkloadError("singleton_aggregate must not be used as a comparison role")
+ for role_key in ("comparison_lhs_role", "comparison_rhs_role"):
+ role = str(cell.get(role_key, ""))
+ if any(bad in role for bad in _FORBIDDEN_COMPARISON_ROLES):
+ raise WorkloadError(
+ f"cell {cell.get('cell_id')!r}: {role_key} must not use a "
+ f"forbidden comparison role, got {role!r}"
+ )🤖 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 `@rl_engine/testing/ws1_workload.py` around lines 443 - 450, Update the cell
validation loop around the comparison_lhs_role check to also reject
singleton_aggregate when it appears in comparison_rhs_role. Apply the same
validation rule to both comparison role fields while preserving the existing
batch_mode checks and WorkloadError behavior.
| for case in cases: | ||
| required = expected_shapes[case["fixture_id"]][case["family"]] | ||
| mismatched = { | ||
| key: (case["shape"].get(key), value) | ||
| for key, value in required.items() | ||
| if case["shape"].get(key) != value | ||
| } | ||
| if mismatched: | ||
| raise WorkloadError( | ||
| f"case {case['case_id']} shape does not derive from fixture " | ||
| f"{case['fixture_id']}: {mismatched}" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Manifest-derived failures do not consistently surface as WorkloadError. The _require helper documents that this module must "never [raise a] bare KeyError", and both CLIs rely on that contract by catching a narrow exception tuple. Several manifest-driven paths still raise LookupError or OSError, which escape those handlers and produce a traceback instead of the error: ... message and exit code 2.
rl_engine/testing/ws1_workload.py#L703-L714: replace the doubleexpected_shapes[...][...]index with guarded.get()lookups that raiseWorkloadErrorfor an unknownfixture_idor an uncoveredfamily.scripts/ws1_reference.py#L81-L93: extend the handler toexcept (WorkloadError, OSError, json.JSONDecodeError)so a missing or malformed--manifestpath exits cleanly.scripts/ws1_candidate_evidence.py#L34-L68: raiseWorkloadErrorfrom_case_argswhen a per-family shape key is absent, or addKeyErrorto the handler at line 159.
📍 Affects 3 files
rl_engine/testing/ws1_workload.py#L703-L714(this comment)scripts/ws1_reference.py#L81-L93scripts/ws1_candidate_evidence.py#L34-L68
🤖 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 `@rl_engine/testing/ws1_workload.py` around lines 703 - 714, Ensure
manifest-derived failures consistently use the CLIs’ handled exception contract:
in rl_engine/testing/ws1_workload.py:703-714, replace the direct expected_shapes
indexing with guarded .get() lookups that raise WorkloadError for unknown
fixture_id or uncovered family; in scripts/ws1_reference.py:81-93, catch
WorkloadError, OSError, and json.JSONDecodeError; in
scripts/ws1_candidate_evidence.py:34-68, raise WorkloadError from _case_args
when a per-family shape key is missing, or alternatively include KeyError in the
handler at line 159.
| def test_missing_matrix_cell_rejected(): | ||
| raw = json.loads(default_manifest_path().read_text(encoding="utf-8")) | ||
| raw["primary_matrix"]["cells"] = raw["primary_matrix"]["cells"][:3] | ||
| with pytest.raises(WorkloadError, match="primary_matrix.cells"): | ||
| validate_manifest(raw) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape the match= pattern.
match= takes a regular expression. The . in "primary_matrix.cells" matches any character. Ruff flags this as RUF043.
💚 Proposed fix
- with pytest.raises(WorkloadError, match="primary_matrix.cells"):
+ with pytest.raises(WorkloadError, match=re.escape("primary_matrix.cells")):
validate_manifest(raw)Add import re to the imports.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 475-475: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
🤖 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/test_ws1_workload.py` around lines 472 - 476, Update
test_missing_matrix_cell_rejected so its pytest.raises match pattern escapes the
dot in “primary_matrix.cells” as a literal period, satisfying Ruff RUF043 while
preserving the existing validation assertion.
Source: Linters/SAST tools
1 participant
Summary
Closes #269
This PR lands the WS1 C3 shared forward accuracy/invariance harness used by later gates (C8/C10). It provides one API/report schema for batch/chunk/padding/layout config sweeps, C2 logical identity after unpadding, C1-only thresholds, selected-logprob smoke, and fail-closed backend provenance for the independent
cuda_bf16andtriton_cuda_bf16profiles.Parent issue #266 remains open. C4–C11 are intentionally out of scope for this PR.
Depends on / stacks with: C1 PR #290 and C2 PR #292 (this branch is based on
feat/ws1-c2-canonical-workload-268). Merge #290 then #292 first, or land as a stack.What changed
assert_forward_batch_invariant(...)→ForwardInvarianceReportbuild_config_matrix:BN/full,BN/chunked, B1 singleton full/chunked)restore_logical_order/restore_logical_order_from_paddedforward_invariance,atol=0,rtol=0)forward_accuracytolerances onlyscripts/check_forward_invariance.py:cuda_bf16+ declared CUDAlogpcandidatetriton_cuda_bf16+ declared Tritonbatch_invariant_logpcandidatemissing_requirednodes as redtests/test_forward_invariance.py)rl_engine.kernels.gtestdocs/design/ws1-c3-269-closeout-evidence.mdValidation
The full suite was run with:
Scope boundaries
This PR does not claim completion of:
Those remain tracked by the C4–C11 child issues under #266.
This closes only C3. It supplies the report and canonicalization contract that C10 must reuse. It does not invent private thresholds, private logical keys, or alternate report semantics.
Review notes
run_operator_suiteis deferred and out of scope for C3.actual_backendin the C3 CLI is declared from the selected candidate family and validated against the C1 profile contract / C2 expected backend id; full-model dispatch provenance remains owned by C8/C10/C11.fused_logp fallback kernelis the generic CUDA candidate path name, not cross-profile silent fallback.Summary by CodeRabbit
New Features
Documentation
Tests