[WS1][C1] Land four-judgment dtype and tolerance contract - #290
[WS1][C1] Land four-judgment dtype and tolerance contract#290maxiaosong1124 wants to merge 4 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.
📝 WalkthroughWalkthroughThe PR adds the WS1 numerical tolerance contract, validation and resolution APIs, provenance-aware operator checks, chain-level log-probability aggregates, comprehensive tests, and gtest contributor documentation. ChangesWS1 numerical contract
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
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: 1
🧹 Nitpick comments (10)
rl_engine/kernels/gtest/tolerance.py (4)
879-895: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine the arch-override merge semantics before the first override lands.
_lookup_cellreturns the architecture cell whole and does not merge it with the baseby_op_classrow. A partial override such as{"atol": 1.0e-2}would then losestatus,rtol, andmode.resolve_tolerance_supportwould reject it withinvalid support status '', andresolve_tolerancewould reject it withcell missing atol/rtol.
arch_overrides.sm90is empty intolerance_contract.json, so nothing breaks today. Two options exist: merge the override over the base row, or document that an override must be a complete cell and enforce that in_validate_judgments.♻️ Option 1: merge the override over the base row
def _lookup_cell( judgment_root: Mapping[str, Any], *, op_class: str, dtype_name: str, arch_key: str | None, ) -> Mapping[str, Any] | None: + base = judgment_root.get("by_op_class", {}).get(op_class, {}).get(dtype_name) if arch_key is not None: arch_cell = ( judgment_root.get("arch_overrides", {}) .get(arch_key, {}) .get(op_class, {}) .get(dtype_name) ) if arch_cell is not None: - return arch_cell - return judgment_root.get("by_op_class", {}).get(op_class, {}).get(dtype_name) + if base is None: + return arch_cell + return {**base, **arch_cell} + return base🤖 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 879 - 895, Update _lookup_cell to merge an architecture-specific override over the corresponding by_op_class base cell, preserving base fields such as status, rtol, and mode when the override is partial. Use the base cell as the starting mapping and apply override values on top; retain the existing base-cell lookup behavior when no override exists.
384-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the FP8 guard; the second clause is dead logic.
Python evaluates this as
A or (B and C).Aisdtype_name in OUT_OF_SCOPE_DTYPES, andOUT_OF_SCOPE_DTYPESis("float8",).Cisdtype_name == "float8", which is the same condition asA. Thepolicy.fp8term can never change the result. The expression reduces todtype_name == "float8".The behavior is correct today, but the code implies that
policy.fp8is consulted. It is not. Ruff also reports RUF021 on this line.♻️ Proposed simplification
- if dtype_name in OUT_OF_SCOPE_DTYPES or policy.fp8 == "out_of_scope" and dtype_name == "float8": + if dtype_name in OUT_OF_SCOPE_DTYPES: raise ContractResolveError( f"dtype {dtype_name!r} is out of scope for WS1 (FP8 requests hard-fail)" )🤖 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 384 - 387, In the dtype guard near the contract-resolution error, remove the redundant policy.fp8 condition and use the existing OUT_OF_SCOPE_DTYPES membership check directly. Preserve the current hard-fail behavior for float8 and avoid implying that policy.fp8 affects this branch.Source: Linters/SAST tools
807-845: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the declared aggregate semantics, not only key presence.
_validate_chain_aggregatesrequiresdlogp_definitionbut never checks its value.compute_logprob_aggregateshardcodeslhs - rhsat line 560. The same gap applies to each metric'sformulaandpass_rule, which the validator does not read at all, and toactive_token_policyandclip_interval_field, which the validator does not require.The contract is documented as the single source of truth. Today an edit that flips
dlogp_definitiontocomparison_rhs_logp - comparison_lhs_logppasses validation while the implementation keeps computinglhs - rhs. The sign ofmax_abs_dlogpis unaffected, butapprox_kl0andclipfrac0change. That drift is silent.Pin the declared values that the implementation hardcodes.
🛡️ Proposed additional checks
if root["nan_inf_policy"] != "hard_fail": raise ContractSchemaError("nan_inf_policy must be hard_fail") if root["empty_active_token_set"] != "hard_fail": raise ContractSchemaError("empty_active_token_set must be hard_fail") + if root["dlogp_definition"] != "comparison_lhs_logp - comparison_rhs_logp": + raise ContractSchemaError( + "dlogp_definition must remain 'comparison_lhs_logp - comparison_rhs_logp'; " + "compute_logprob_aggregates implements exactly this direction" + ) if not root["require_all"]: raise ContractSchemaError("require_all must be true for chain logprob aggregates")for name in CHAIN_AGGREGATE_METRICS: if name not in metrics: raise ContractSchemaError(f"chain metrics missing {name!r}") + if metrics[name].get("pass_rule") != "value <= threshold": + raise ContractSchemaError( + f"metric {name!r} pass_rule must be 'value <= threshold'; " + "judge_logprob_aggregates implements exactly this rule" + ) by_dtype = metrics[name].get("by_execution_dtype")🤖 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 807 - 845, Update _validate_chain_aggregates to validate the declared semantics that compute_logprob_aggregates hardcodes: require dlogp_definition to match lhs - rhs, require active_token_policy and clip_interval_field, and verify each metric’s formula and pass_rule against the implementation’s expected values. Reject any mismatched declarations with ContractSchemaError so the contract remains the single source of truth.
780-789: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant inner condition.
The outer condition already excludes
"applicable"and"not_applicable". Inside that branch,status != "applicable"is always true. The innerifnever changes control flow.Enforcement is correct. Only the nesting is redundant.
♻️ Proposed simplification
- if dtype_name in MANDATORY_DTYPES and status not in { - "applicable", - "not_applicable", - }: - # BF16/FP32 must be explicitly applicable (or explicit N/A). - if status != "applicable": - raise ContractSchemaError( - f"mandatory dtype cell must be applicable: " - f"{judgment}/{op_class}/{dtype_name} status={status!r}" - ) + # BF16/FP32 must be explicitly applicable (or explicit N/A). + if dtype_name in MANDATORY_DTYPES and status not in { + "applicable", + "not_applicable", + }: + raise ContractSchemaError( + f"mandatory dtype cell must be applicable: " + f"{judgment}/{op_class}/{dtype_name} status={status!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/kernels/gtest/tolerance.py` around lines 780 - 789, Remove the redundant inner `if status != "applicable"` in the mandatory dtype validation branch, and unindent the existing `raise ContractSchemaError` so it executes directly whenever the outer condition matches. Preserve the current validation condition and error message.rl_engine/kernels/gtest/__init__.py (1)
5-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting
ContractErrorand the primary resolvers.The package re-exports
ContractResolveErrorandContractSchemaErrorbut not their base classContractError. A caller that wants to catch both subclasses at the package level must import fromrl_engine.kernels.gtest.tolerancedirectly.The same applies to the resolvers.
resolve_tolerance_supportis exported, butload_contract,resolve_tolerance, andresolve_dtype_policyare not. Thetolerancemodule docstring states that gates must obtain thresholds only through its resolvers, which argues for a consistent package-level surface.This is API polish. Nothing is broken.
♻️ Proposed export set
from .tolerance import ( BackendProvenance, + ContractError, ContractResolveError, ContractSchemaError, + load_contract, + resolve_dtype_policy, + resolve_tolerance, resolve_tolerance_support, validate_backend_provenance, ) __all__ = [ "CandidateSpec", "OperatorCase", "run_operator_suite", "BackendProvenance", + "ContractError", "ContractResolveError", "ContractSchemaError", + "load_contract", + "resolve_dtype_policy", + "resolve_tolerance", "resolve_tolerance_support", "validate_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 `@rl_engine/kernels/gtest/__init__.py` around lines 5 - 21, Update the package exports in __init__.py to include ContractError alongside ContractResolveError and ContractSchemaError, and re-export the primary tolerance resolvers load_contract, resolve_tolerance, and resolve_dtype_policy alongside resolve_tolerance_support. Add the corresponding imports and __all__ entries while preserving the existing public exports.tests/test_tolerance_contract.py (5)
314-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the chain aggregate thresholds instead of asserting non-negativity.
assert thr >= 0.0passes for any non-negative value. A change from5.0e-2to5.0formax_abs_dlogpwould not fail this test.The PR objectives describe these thresholds as a frozen judgment source for ablations and gates.
test_compat_accuracy_mirrors_forward_accuracypins the accuracy rows. No test pins the chain aggregate rows.💚 Proposed test
def test_chain_aggregate_named_resolve(): contract = load_contract() - for metric in CHAIN_AGGREGATE_METRICS: - thr = resolve_chain_aggregate_thresholds(contract, metric, "bfloat16") - assert thr >= 0.0 + expected = { + "max_abs_dlogp": {"bfloat16": 5.0e-2, "float32": 1.0e-5}, + "approx_kl0": {"bfloat16": 5.0e-2, "float32": 1.0e-5}, + "clipfrac0": {"bfloat16": 0.0, "float32": 0.0}, + } + assert set(expected) == set(CHAIN_AGGREGATE_METRICS) + for metric, by_dtype in expected.items(): + for dtype_name, threshold in by_dtype.items(): + assert resolve_chain_aggregate_thresholds(contract, metric, dtype_name) == threshold with pytest.raises(ContractResolveError, match="unknown chain aggregate"): resolve_chain_aggregate_thresholds(contract, "mean_abs_dlogp", "bfloat16")🤖 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 314 - 320, Update test_chain_aggregate_named_resolve to assert each resolved metric threshold equals its frozen contract value rather than merely being non-negative. Pin all entries in CHAIN_AGGREGATE_METRICS, including max_abs_dlogp, using the established contract threshold definitions, while preserving the existing unknown-metric ContractResolveError assertion.
385-412: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the
ratio0overflow branch and the inactive-NaN case.Both blocks place a non-finite value directly in
lhs, socompute_logprob_aggregatesfails at thedlogpcheck on line 561. The separateratio0check on lines 565-566 never runs in any test. That branch triggers whendlogpis finite butexp(dlogp)overflows float32, which happens above roughly88.7.The complementary case is also untested: a non-finite value at an inactive position must be ignored, because the contract filters to active tokens before the finiteness check.
💚 Proposed additional tests
def test_ratio_overflow_hard_fails(): # dlogp is finite but exp(dlogp) overflows float32. lhs = torch.tensor([200.0, 0.0], dtype=torch.float32) rhs = torch.zeros(2, dtype=torch.float32) mask = torch.ones(2, dtype=torch.bool) with pytest.raises(ContractResolveError, match="NaN/Inf"): compute_logprob_aggregates( lhs, rhs, mask, contract=load_contract(), report_kind="train_infer_logprob_parity", clip_interval=(0.8, 1.2), comparison_lhs_role="training_style_teacher_forcing", comparison_rhs_role="inference_style_rollout_decode", ) def test_inactive_nan_is_ignored(): lhs = torch.tensor([0.0, float("nan")], dtype=torch.float32) rhs = torch.zeros(2, dtype=torch.float32) mask = torch.tensor([True, False]) agg = compute_logprob_aggregates( lhs, rhs, mask, contract=load_contract(), report_kind="train_infer_logprob_parity", clip_interval=(0.8, 1.2), comparison_lhs_role="training_style_teacher_forcing", comparison_rhs_role="inference_style_rollout_decode", ) assert agg.active_token_count == 1 assert agg.max_abs_dlogp == 0.0🤖 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 385 - 412, Extend the tolerance contract tests around test_nan_inf_hard_fail by adding coverage for finite dlogp values whose float32 exp overflows, asserting compute_logprob_aggregates raises ContractResolveError matching “NaN/Inf”. Also add an inactive-NaN case with the NaN masked off, asserting successful aggregation with one active token and max_abs_dlogp equal to zero.
415-453: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case where exactly one metric fails.
The drift block uses
dlogp = [0.0, 1.0]. That fails all three metrics at once:max_abs_dlogpis1.0,approx_kl0is about0.359, andclipfrac0is0.5. Every BF16 threshold is exceeded.The test therefore does not prove the AND semantics that its name states. A verdict must fail when one metric fails and the other two pass.
clipfrac0has a BF16 threshold of0.0, so adlogpjust inside themax_abs_dlogpandapprox_kl0limits but outside the clip interval is not reachable. Use a value that fails onlymax_abs_dlogpandapprox_kl0whileclipfrac0stays at0.0.💚 Proposed additional test
def test_verdict_fails_when_clipfrac_alone_fails(): contract = load_contract() clip = default_clip_interval(contract) # dlogp = 0.0 for one token and 1.0 for the other -> clipfrac0 = 0.5. # Verify each metric verdict individually so the AND semantics are explicit. lhs = torch.tensor([0.0, 1.0], dtype=torch.float32) rhs = torch.zeros(2, dtype=torch.float32) mask = torch.ones(2, dtype=torch.bool) agg = compute_logprob_aggregates( lhs, rhs, mask, contract=contract, report_kind="train_infer_logprob_parity", clip_interval=clip, comparison_lhs_role="training_style_teacher_forcing", comparison_rhs_role="inference_style_rollout_decode", ) verdict = judge_logprob_aggregates(agg, contract, execution_dtype="bfloat16") by_metric = {m.metric: m for m in verdict.metrics} assert math.isclose(by_metric["clipfrac0"].value, 0.5, rel_tol=0.0, abs_tol=1e-6) assert by_metric["clipfrac0"].threshold == 0.0 assert not by_metric["clipfrac0"].passed assert not verdict.passed🤖 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 415 - 453, Update test_judge_requires_all_three_aggregates to use a drift case where only max_abs_dlogp and approx_kl0 fail while clipfrac0 remains 0.0, proving the verdict requires all metrics to pass. Add individual metric assertions as needed to verify those two failures, clipfrac0 passing, and the overall verdict failing; do not use the proposed clipfrac-only case because the requested scenario is a single failing metric.
114-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden the shared-threshold test and drop the redundant outer loop.
The outer loop runs twice and asserts the same fact both times with
aandbswapped. It resolves four specs to check one pair.Coverage is also narrow. The test checks only
forward_accuracy/logprob/bfloat16. The contract requires thatcuda_bf16andtriton_cuda_bf16share every row. A sweep over all judgments and operation classes proves that property and catches a future backend-specific row.💚 Proposed test
def test_cuda_and_triton_profiles_share_thresholds(): contract = load_contract() - for profile in ("cuda_bf16", "triton_cuda_bf16"): - a = resolve_tolerance( - contract, - judgment="forward_accuracy", - op_class="logprob", - dtype="bfloat16", - backend_profile=profile, - ) - b = resolve_tolerance( - contract, - judgment="forward_accuracy", - op_class="logprob", - dtype="bfloat16", - backend_profile="cuda_bf16" if profile != "cuda_bf16" else "triton_cuda_bf16", - ) - assert a.atol == b.atol and a.rtol == b.rtol and a.mode == b.mode + for judgment in JUDGMENTS: + for op_class in OP_CLASSES: + for dtype_name in ("float32", "bfloat16"): + specs = [ + resolve_tolerance( + contract, + judgment=judgment, + op_class=op_class, + dtype=dtype_name, + backend_profile=profile, + ) + for profile in ("cuda_bf16", "triton_cuda_bf16") + ] + cuda, triton = specs + assert cuda.atol == triton.atol + assert cuda.rtol == triton.rtol + assert cuda.mode == triton.mode🤖 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 114 - 131, Replace the redundant loop in test_cuda_and_triton_profiles_share_thresholds with a single sweep over every judgment and operation class defined by the contract, resolving each dtype/profile row for cuda_bf16 and triton_cuda_bf16 and asserting matching atol, rtol, and mode. Preserve the existing bfloat16 coverage while expanding the test to detect any backend-specific threshold row.
323-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case where
clipfrac0is nonzero.With
dlogp = [0.0, 0.1, -0.2]the ratios are[1.0, 1.105, 0.819]. All three fall inside(0.8, 1.2), soexpected_clipis0.0. The test also derivesexpected_clipfrom the same expression that the implementation uses, so it cannot detect a wrong interval comparison.No test in this file asserts a nonzero
clipfrac0value.test_judge_requires_all_three_aggregatesproducesclipfrac0 == 0.5but only asserts the verdict. The boundary rule incompute_logprob_aggregatesat line 570 treats a ratio exactly equal toloorhias inside. That rule is also unpinned.💚 Proposed additional test
def test_clipfrac0_counts_ratios_outside_the_interval(): # dlogp = [0.0, 1.0, -1.0] -> ratio = [1.0, e, 1/e]; two of three fall outside. lhs = torch.tensor([0.0, 1.0, -1.0], dtype=torch.float32) rhs = torch.zeros(3, dtype=torch.float32) mask = torch.ones(3, dtype=torch.bool) agg = compute_logprob_aggregates( lhs, rhs, mask, contract=load_contract(), report_kind="train_infer_logprob_parity", clip_interval=(0.8, 1.2), comparison_lhs_role="training_style_teacher_forcing", comparison_rhs_role="inference_style_rollout_decode", ) assert math.isclose(agg.clipfrac0, 2.0 / 3.0, rel_tol=0.0, abs_tol=1e-6) def test_clip_interval_endpoints_count_as_inside(): # ratio exactly at lo and hi must not be counted as clipped. lo, hi = 0.8, 1.2 lhs = torch.tensor([math.log(lo), math.log(hi)], dtype=torch.float32) rhs = torch.zeros(2, dtype=torch.float32) mask = torch.ones(2, dtype=torch.bool) agg = compute_logprob_aggregates( lhs, rhs, mask, contract=load_contract(), report_kind="train_infer_logprob_parity", clip_interval=(lo, hi), comparison_lhs_role="training_style_teacher_forcing", comparison_rhs_role="inference_style_rollout_decode", ) assert agg.clipfrac0 == 0.0Note:
test_clip_interval_endpoints_count_as_insidedepends on float32 rounding ofexp(log(lo)). If it proves flaky, assert the documented rule directly on a ratio computed in float32 instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tolerance_contract.py` around lines 323 - 347, Add tests in tests/test_tolerance_contract.py covering compute_logprob_aggregates with ratios outside clip_interval and assert clipfrac0 is 2/3, rather than deriving the expected value from the same expression as the implementation. Also add an endpoint case asserting ratios equal to lo and hi are treated as inside the interval, using a float32-stable setup if needed.
🤖 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 `@tests/test_op_checks.py`:
- Around line 224-248: Update
test_ws1_report_rejects_backend_provenance_mismatch so provenance.actual_backend
matches the declared backend required by the cuda_bf16 profile, while
CandidateSpec.backend remains a different backend. Preserve the existing
run_operator_suite invocation and ContractResolveError assertion so the test
exercises the declared-backend validation in _run_candidate.
---
Nitpick comments:
In `@rl_engine/kernels/gtest/__init__.py`:
- Around line 5-21: Update the package exports in __init__.py to include
ContractError alongside ContractResolveError and ContractSchemaError, and
re-export the primary tolerance resolvers load_contract, resolve_tolerance, and
resolve_dtype_policy alongside resolve_tolerance_support. Add the corresponding
imports and __all__ entries while preserving the existing public exports.
In `@rl_engine/kernels/gtest/tolerance.py`:
- Around line 879-895: Update _lookup_cell to merge an architecture-specific
override over the corresponding by_op_class base cell, preserving base fields
such as status, rtol, and mode when the override is partial. Use the base cell
as the starting mapping and apply override values on top; retain the existing
base-cell lookup behavior when no override exists.
- Around line 384-387: In the dtype guard near the contract-resolution error,
remove the redundant policy.fp8 condition and use the existing
OUT_OF_SCOPE_DTYPES membership check directly. Preserve the current hard-fail
behavior for float8 and avoid implying that policy.fp8 affects this branch.
- Around line 807-845: Update _validate_chain_aggregates to validate the
declared semantics that compute_logprob_aggregates hardcodes: require
dlogp_definition to match lhs - rhs, require active_token_policy and
clip_interval_field, and verify each metric’s formula and pass_rule against the
implementation’s expected values. Reject any mismatched declarations with
ContractSchemaError so the contract remains the single source of truth.
- Around line 780-789: Remove the redundant inner `if status != "applicable"` in
the mandatory dtype validation branch, and unindent the existing `raise
ContractSchemaError` so it executes directly whenever the outer condition
matches. Preserve the current validation condition and error message.
In `@tests/test_tolerance_contract.py`:
- Around line 314-320: Update test_chain_aggregate_named_resolve to assert each
resolved metric threshold equals its frozen contract value rather than merely
being non-negative. Pin all entries in CHAIN_AGGREGATE_METRICS, including
max_abs_dlogp, using the established contract threshold definitions, while
preserving the existing unknown-metric ContractResolveError assertion.
- Around line 385-412: Extend the tolerance contract tests around
test_nan_inf_hard_fail by adding coverage for finite dlogp values whose float32
exp overflows, asserting compute_logprob_aggregates raises ContractResolveError
matching “NaN/Inf”. Also add an inactive-NaN case with the NaN masked off,
asserting successful aggregation with one active token and max_abs_dlogp equal
to zero.
- Around line 415-453: Update test_judge_requires_all_three_aggregates to use a
drift case where only max_abs_dlogp and approx_kl0 fail while clipfrac0 remains
0.0, proving the verdict requires all metrics to pass. Add individual metric
assertions as needed to verify those two failures, clipfrac0 passing, and the
overall verdict failing; do not use the proposed clipfrac-only case because the
requested scenario is a single failing metric.
- Around line 114-131: Replace the redundant loop in
test_cuda_and_triton_profiles_share_thresholds with a single sweep over every
judgment and operation class defined by the contract, resolving each
dtype/profile row for cuda_bf16 and triton_cuda_bf16 and asserting matching
atol, rtol, and mode. Preserve the existing bfloat16 coverage while expanding
the test to detect any backend-specific threshold row.
- Around line 323-347: Add tests in tests/test_tolerance_contract.py covering
compute_logprob_aggregates with ratios outside clip_interval and assert
clipfrac0 is 2/3, rather than deriving the expected value from the same
expression as the implementation. Also add an endpoint case asserting ratios
equal to lo and hi are treated as inside the interval, using a float32-stable
setup if needed.
🪄 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: a476f8a8-edf6-4e38-9eba-71eb841e1b4a
📒 Files selected for processing (8)
docs/contributing/gtest-usage.mddocs/contributing/testing.mdrl_engine/kernels/gtest/__init__.pyrl_engine/kernels/gtest/op_checks.pyrl_engine/kernels/gtest/tolerance.pyrl_engine/kernels/gtest/tolerance_contract.jsontests/test_op_checks.pytests/test_tolerance_contract.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tolerance.py`:
- Around line 784-787: Update contract loading in the validation logic around
the mandatory dtype-cell check (lines 784-787) to validate every judgment base
and architecture-override cell, rejecting invalid statuses or modes and
non-finite or negative tolerances. Also update the metric-threshold and
clip-bound validation around lines 852-860 to require finite numeric values
before gate evaluation; both affected sites are in
rl_engine/kernels/gtest/tolerance.py.
🪄 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: d1217f01-fafb-452d-8118-3e75f28598f0
📒 Files selected for processing (6)
docs/contributing/testing.mdrl_engine/kernels/gtest/__init__.pyrl_engine/kernels/gtest/op_checks.pyrl_engine/kernels/gtest/tolerance.pytests/test_op_checks.pytests/test_tolerance_contract.py
💤 Files with no reviewable changes (1)
- docs/contributing/testing.md
🚧 Files skipped from review as they are similar to previous changes (4)
- rl_engine/kernels/gtest/init.py
- tests/test_tolerance_contract.py
- tests/test_op_checks.py
- rl_engine/kernels/gtest/op_checks.py
| raise ContractSchemaError( | ||
| f"mandatory dtype cell must be applicable: " | ||
| f"{judgment}/{op_class}/{dtype_name} status={status!r}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate all gate-calibration values during contract loading. The contract validator checks field presence but does not consistently validate value type, finiteness, and allowed ranges.
rl_engine/kernels/gtest/tolerance.py#L784-L787: validate all judgment base and architecture-override cells. Reject invalid statuses, modes, and non-finite or negative tolerances.rl_engine/kernels/gtest/tolerance.py#L852-L860: validate metric thresholds and clip bounds as finite numeric values before gate evaluation.
📍 Affects 1 file
rl_engine/kernels/gtest/tolerance.py#L784-L787(this comment)rl_engine/kernels/gtest/tolerance.py#L852-L860
🤖 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 784 - 787, Update contract
loading in the validation logic around the mandatory dtype-cell check (lines
784-787) to validate every judgment base and architecture-override cell,
rejecting invalid statuses or modes and non-finite or negative tolerances. Also
update the metric-threshold and clip-bound validation around lines 852-860 to
require finite numeric values before gate evaluation; both affected sites are in
rl_engine/kernels/gtest/tolerance.py.
1 participant
Summary
Closes #267
This PR lands the WS1 C1 numerical contract used by the gtest operator harness. It freezes the shared dtype policy, four judgment axes, comparison roles, logprob aggregates, and backend provenance checks.
Parent issue #266 remains open. C2–C11 are intentionally out of scope for this PR.
What changed
cuda_bf16andtriton_cuda_bf16profilesforward_accuracyforward_invariancegradient_accuracygradient_invariancemode=bitwiseatol=0rtol=0not_applicableandout_of_scopecells.comparison_lhs_rolecomparison_rhs_rolemax_abs_dlogpapprox_kl0clipfrac0op_checksso forward and gradient checks resolve thresholds independently and persist judgment/role/provenance fields in reports.Validation