[https://nvbugs/6506920][fix] Add stderr, stderr_margin_sigmas fields to HypothesisTestingParams… - #16858
[https://nvbugs/6506920][fix] Add stderr, stderr_margin_sigmas fields to HypothesisTestingParams…#16858trtllm-agent wants to merge 2 commits into
stderr, stderr_margin_sigmas fields to HypothesisTestingParams…#16858Conversation
WalkthroughThe accuracy verification flow now accepts per-metric stderr values, adjusts hypothesis-testing reference accuracy around its original anchor, reports the adjustment, and updates the GSM8K DSpark reference while removing its DGX_B200 waiver. ChangesAccuracy stderr handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReferenceConfig
participant AccuracyTask
participant HypothesisTestingParams
participant AccuracyReport
ReferenceConfig->>AccuracyTask: provide entry stderr
AccuracyTask->>HypothesisTestingParams: pass stderr
HypothesisTestingParams->>HypothesisTestingParams: anchor and adjust ref_accuracy
HypothesisTestingParams->>AccuracyReport: include anchor and adjustment
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
84-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary — needs follow-up.
- Test functions added, modified, or removed: none in the provided diff.
- Test-list membership: not applicable to changed test functions; no test-list entry is shown.
- Coverage verdict: needs follow-up. Add focused tests for higher- and lower-is-better metrics, zero/invalid stderr, configurable sigma margins, and report output. Integration validation requires GPU/model access with
LLM_MODELS_ROOTset.As per path instructions, changed test infrastructure requires an explicit coverage assessment.
Also applies to: 204-213
🤖 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/integration/defs/accuracy/accuracy_core.py` around lines 84 - 126, Add focused tests covering __post_init__ for higher- and lower-is-better metrics, including zero, invalid, and configured stderr margin values, and verify that ref_accuracy and threshold are adjusted correctly. Add report tests validating anchor and adjustment output when stderr is positive and unchanged output otherwise. Run the integration validation with GPU/model access and LLM_MODELS_ROOT configured.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 `@tests/integration/defs/accuracy/accuracy_core.py`:
- Line 72: Make stderr_margin_sigmas configurable in the HypothesisTestingParams
API instead of relying only on STDERR_MARGIN_SIGMAS. Update the relevant
parameter construction and reference-entry handling, including
get_hypothesis_testing_params(), to forward entry.get("stderr_margin_sigmas",
STDERR_MARGIN_SIGMAS) so entries can override the default while preserving 2.0
when unspecified.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 84-126: Add focused tests covering __post_init__ for higher- and
lower-is-better metrics, including zero, invalid, and configured stderr margin
values, and verify that ref_accuracy and threshold are adjusted correctly. Add
report tests validating anchor and adjustment output when stderr is positive and
unchanged output otherwise. Run the integration validation with GPU/model access
and LLM_MODELS_ROOT configured.
🪄 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: a65bcf1e-d9f5-4950-814a-52357f15d345
📒 Files selected for processing (3)
tests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| return ref_accuracy - z_alpha * scale | ||
|
|
||
|
|
||
| STDERR_MARGIN_SIGMAS = 2.0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Implement the advertised stderr_margin_sigmas parameter.
The PR contract describes stderr_margin_sigmas as configurable, but this class only exposes the hard-coded STDERR_MARGIN_SIGMAS = 2.0; reference entries cannot select another margin.
Suggested fix
class HypothesisTestingParams:
stderr: Optional[float] = None
+ stderr_margin_sigmas: float = STDERR_MARGIN_SIGMAS
def __post_init__(self) -> None:
self.ref_accuracy_anchor = self.ref_accuracy
if self.stderr is not None and self.stderr > 0:
sign = -1 if self.higher_is_better else 1
- self.ref_accuracy += sign * STDERR_MARGIN_SIGMAS * self.stderr
+ self.ref_accuracy += sign * self.stderr_margin_sigmas * self.stderrAlso forward entry.get("stderr_margin_sigmas", STDERR_MARGIN_SIGMAS) from get_hypothesis_testing_params().
Based on the PR objectives, stderr_margin_sigmas is part of the intended HypothesisTestingParams API.
Also applies to: 84-96, 204-213
🤖 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/integration/defs/accuracy/accuracy_core.py` at line 72, Make
stderr_margin_sigmas configurable in the HypothesisTestingParams API instead of
relying only on STDERR_MARGIN_SIGMAS. Update the relevant parameter construction
and reference-entry handling, including get_hypothesis_testing_params(), to
forward entry.get("stderr_margin_sigmas", STDERR_MARGIN_SIGMAS) so entries can
override the default while preserving 2.0 when unspecified.
0bed403 to
1d833dd
Compare
…ic stderr The DSpark GSM8K test's strict outer floor sat inside lm-eval's own reported per-metric stderr (~0.55 on 1319 samples), so natural evaluation noise straddled the 96.0 gate. Add optional 'stderr' and 'stderr_margin_sigmas' fields to HypothesisTestingParams and thread them from yaml entries in get_hypothesis_testing_params. When set, ref_accuracy is treated as an anchor and adjusted by stderr_margin_sigmas * stderr for strict-outer comparisons; the original is retained as ref_accuracy_anchor and surfaced in the hypothesis-testing report. The DSpark GSM8K reference is updated to record the true anchor (96.475) and lm-eval's own reported stderr (0.55); the default 2-sigma margin yields an effective floor of 95.375, above the observed spread. Backward-compatible: entries without a stderr field are unchanged. Remove the DSpark test from waives.txt now that the strict floor is stderr-aware. Verified: score 96.058 >= adjusted ref 95.375 -> PASS. Signed-off-by: handongl <handongl@nvidia.com>
Signed-off-by: handongl <handongl@nvidia.com>
1d833dd to
5ce3400
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_lists/waives.txt (1)
91-104: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep unrelated waiver changes out of this PR.
Besides the intended DGX_B200 DSpark waiver removal, this diff removes the unrelated DGX_H100 scaffolding waiver (
nvbugs/6529626) and changes several unrelated skips. These alter CI coverage without being part of the GSM8K stderr fix; revert or split them into separate changes.As per coding guidelines, keep each pull request focused on one concern.
Also applies to: 142-144, 186-187, 238-238, 255-255, 340-342, 370-371
🤖 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/integration/test_lists/waives.txt` around lines 91 - 104, Revert unrelated waiver additions, removals, and skip changes in the waiver list, including the DGX_H100 scaffolding waiver and the listed test entries. Keep only the intended DGX_B200 DSpark waiver removal associated with the GSM8K stderr fix, leaving all other CI coverage unchanged.Source: Coding guidelines
♻️ Duplicate comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
72-74:⚠️ Potential issue | 🟠 Major
stderr_margin_sigmasis still not configurable.The PR contract advertises this field, but the implementation only hard-codes
STDERR_MARGIN_SIGMASand forwardsstderr; reference entries cannot override the sigma margin. Add the field with a2.0default and forward the per-entry value while preserving that default.Also applies to: 84-96, 204-213
🤖 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/integration/defs/accuracy/accuracy_core.py` around lines 72 - 74, Make stderr_margin_sigmas configurable in the accuracy definition and reference-entry flow: add the field with a default of 2.0, then forward each entry’s value instead of always using STDERR_MARGIN_SIGMAS. Preserve the 2.0 behavior when the field is omitted, updating the relevant definitions and forwarding logic near the reference-entry handling.
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
84-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused regression tests for stderr adjustment and reporting.
The supplied change adds no direct coverage for anchor preservation, positive/zero/None stderr, lower-is-better behavior, invalid values, or the report text. The existing downstream integration assertion does not cover these branches.
Also applies to: 110-126
🤖 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/integration/defs/accuracy/accuracy_core.py` around lines 84 - 96, Add focused tests for the accuracy configuration’s stderr handling and reporting, covering anchor preservation, positive/zero/None stderr, higher- and lower-is-better adjustments, and invalid stderr values. Also assert the generated report text reflects the adjusted threshold and preserved reference anchor, using the relevant accuracy class initialization and reporting symbols.
🤖 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/integration/defs/accuracy/accuracy_core.py`:
- Around line 87-96: Update __post_init__ to validate stderr whenever it is
provided, rejecting negative or non-finite values such as NaN before calculating
the accuracy adjustment. Preserve the existing margin behavior for finite,
non-negative stderr values and leave ref_accuracy unchanged only when stderr is
None.
---
Outside diff comments:
In `@tests/integration/test_lists/waives.txt`:
- Around line 91-104: Revert unrelated waiver additions, removals, and skip
changes in the waiver list, including the DGX_H100 scaffolding waiver and the
listed test entries. Keep only the intended DGX_B200 DSpark waiver removal
associated with the GSM8K stderr fix, leaving all other CI coverage unchanged.
---
Duplicate comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 72-74: Make stderr_margin_sigmas configurable in the accuracy
definition and reference-entry flow: add the field with a default of 2.0, then
forward each entry’s value instead of always using STDERR_MARGIN_SIGMAS.
Preserve the 2.0 behavior when the field is omitted, updating the relevant
definitions and forwarding logic near the reference-entry handling.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 84-96: Add focused tests for the accuracy configuration’s stderr
handling and reporting, covering anchor preservation, positive/zero/None stderr,
higher- and lower-is-better adjustments, and invalid stderr values. Also assert
the generated report text reflects the adjusted threshold and preserved
reference anchor, using the relevant accuracy class initialization and reporting
symbols.
🪄 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: 772d5e6b-3699-4709-9daf-0f99aff72242
📒 Files selected for processing (3)
tests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/test_lists/waives.txt
| stderr: Optional[float] = None | ||
| ref_accuracy_anchor: float = field(init=False) | ||
| theta: float = field(init=False) | ||
| threshold: float = field(init=False) | ||
|
|
||
| def __post_init__(self) -> None: | ||
| self.ref_accuracy_anchor = self.ref_accuracy | ||
| if self.stderr is not None and self.stderr > 0: | ||
| sign = -1 if self.higher_is_better else 1 | ||
| self.ref_accuracy += sign * STDERR_MARGIN_SIGMAS * self.stderr |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid stderr values instead of silently ignoring them.
Negative or NaN values fail the > 0 check and silently disable the adjustment, potentially restoring flaky strict-gate failures. Validate that stderr is finite and non-negative before applying the margin.
🤖 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/integration/defs/accuracy/accuracy_core.py` around lines 87 - 96,
Update __post_init__ to validate stderr whenever it is provided, rejecting
negative or non-finite values such as NaN before calculating the accuracy
adjustment. Preserve the existing margin behavior for finite, non-negative
stderr values and leave ref_accuracy unchanged only when stderr is None.
3 participants
Summary
stderr,stderr_margin_sigmasfields to HypothesisTestingParams (defaults keep old behavior); in post_init, when stderr is set, save the original as ref_accuracy_anchor and subtract 2σ from ref_accuracy so the strict outer read is stderr-aware. Thread from yaml viaentry.get("stderr"). Record anchor 96.475 and stderr 0.55 in gsm8k.yaml's DSpark entry (effective floor 95.375). Remove the DGX_B200 DSpark waiver.Test plan
Links
Dev Engineer Review
stderrhandling to hypothesis-testing–based accuracy verification:HypothesisTestingParamswith optionalstderr: Optional[float]and a retained internalref_accuracy_anchorto preserve the original reference accuracy whenstderris set.STDERR_MARGIN_SIGMAS: float = 2.0(configurable viastderr_margin_sigmas) to scale how much positivestderrrelaxes the strict outer comparison reference.__post_init__, whenstderr > 0, the originalaccuracyreference is treated as the anchor and the strict outer comparison reference is adjusted by the sigma margin (direction respectshigher_is_better).stderr > 0.stderris omitted.AccuracyTask.get_hypothesis_testing_paramsnow forwards per-referencestderrvalues intoHypothesisTestingParams.tests/integration/defs/accuracy/references/gsm8k.yaml, updated the DeepSeek-V4-Pro DSpark reference toaccuracy: 96.475withstderr: 0.55, resulting in an effective strict floor of95.375under the default 2-sigma margin.tests/integration/test_lists/waives.txtby removing the DGX_B200 DSpark GSM8K skip entry corresponding to the old gating behavior (and also removed an additional DGX_H100/unittest scaffolding skip listed in the changes).QA Engineer Review
tests/integration/defs/accuracy/accuracy_core.py: modified hypothesis-testing accuracy verification to accept/propagate per-referencestderr, adjust strict threshold computation via sigma-scaled relaxation, and surface the anchor/adjustment in the report.tests/integration/defs/accuracy/references/gsm8k.yaml: updated the targeted GSM8K DSpark reference entry to includestderr: 0.55with an anchor accuracy of96.475.tests/integration/test_lists/waives.txt: removed SKIP waiver entries (including the DGX_B200 DSpark GSM8K skip and another DGX_H100/unittest scaffolding skip).