Add verification base and metrics judge scoring - #29
Conversation
Signed-off-by: Eugene Ng <ngeugene@google.com>
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @eugeneng04. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
needs-ok-to-test
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a model-backed metrics evaluation pipeline and a registry-driven verification package. The verification engine parses typed specs, executes leaf, sequence, and parallel checks under shared deadlines, and returns structured results. ChangesMetrics evaluation
Verification engine
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)Metrics judge generationsequenceDiagram
participant DeepEval
participant ModelLayerJudge
participant LLMClient
DeepEval->>ModelLayerJudge: generate(prompt)
ModelLayerJudge->>LLMClient: generate_content(contents)
LLMClient-->>ModelLayerJudge: response
ModelLayerJudge-->>DeepEval: text
Verification executionsequenceDiagram
participant Caller
participant VerifierAgent
participant VerificationSpec
participant BaseVerifier
Caller->>VerifierAgent: wait_for_condition(spec, timeout)
VerifierAgent->>VerificationSpec: parse root node
VerifierAgent->>BaseVerifier: verify(remaining deadline)
BaseVerifier-->>VerifierAgent: VerificationResult
VerifierAgent-->>Caller: aggregated VerificationResult
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 |
kubernetes-prow
Bot
added
the
size/XXL
| # Leaf verifiers populate the ``VERIFIERS`` registry via their | ||
| # ``@VERIFIERS.register`` decorators when their modules are imported. | ||
| from devops_bench.core import NotRegisteredError | ||
| from devops_bench.verification.base import VERIFIERS |
There was a problem hiding this comment.
Design note: the parser resolves verifier names against the VERIFIERS registry, which concrete verifiers populate via @VERIFIERS.register on import. This base intentionally imports no concrete verifiers yet — each verifier module adds its own import here as it lands, so the registry is populated without the base depending on any specific verifier.
| from devops_bench.metrics.base import ( | ||
| METRICS, | ||
| MetricContext, | ||
| ) |
There was a problem hiding this comment.
Design note: metric families register themselves against METRICS via @METRICS.register on import. The base pipeline intentionally imports no concrete families yet — each family (and any symbols this module re-exports for callers) is added here as it lands, keeping the pipeline free of hard dependencies on any specific metric.
| children.append(_skipped(child, "deadline exhausted")) | ||
| reasons.append(f"[{i}] skipped") | ||
| ok = False | ||
| continue |
There was a problem hiding this comment.
Should this break after the deadline has been hit so that all remaining children do not have to go through a full loop iteration to get marked "skipped"?
There was a problem hiding this comment.
If we were to break and to assign the skipped afterwards, it would be the same computationally as we still need to have a loop to assign every following child "skipped". If we were just to break, then the children would not be assigned skipped which would lead to the artifact not having information on checks after the one that timed out.
| return getattr(node, "name", None) | ||
|
|
||
|
|
||
| def _timed_out(node: Any, reason: str) -> VerificationResult: |
There was a problem hiding this comment.
_timed_out and _skipped are identical implementations with different names. Should we consolidate them to _failed(node, reason)?
There was a problem hiding this comment.
Yes, thank you for this catch, I will consolidate them
- consolidate the identical _timed_out/_skipped result builders into a single _failed(node, reason) helper; each call site keeps its specific reason string - scope the registry error-message assertion to the sequence builtin so the test only references verifiers that exist in this change Signed-off-by: Eugene Ng <ngeugene@google.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Log a single batch-level line with the result count at info, and move the per-result line to debug so large batches stay quiet by default. Signed-off-by: Eugene Ng <ngeugene@google.com>
janetkuo
added
ok-to-test
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
devops_bench/metrics/pipeline.py (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReflect non-string handling in the type signature.
The function explicitly handles non-string inputs by returning them as-is (line 62), but the signature strictly specifies
name: str -> str. Consider widening the type hint to match the implementation so type-checkers do not raise false positives when this handles dirty runtime data.♻️ Proposed fix
-def _canonical_tool_name(name: str) -> str: +def _canonical_tool_name(name: Any) -> Any:🤖 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 `@devops_bench/metrics/pipeline.py` at line 49, Update the _canonical_tool_name type signature to accept the non-string input types it explicitly returns unchanged, and widen the return annotation accordingly so it accurately reflects the implementation’s runtime behavior.tests/unit/verification/test_base.py (1)
22-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Nonereturn annotations to test functions. All test functions across these three new files omit return-type annotations; the shared root cause is that annotations weren't added when the tests were written.
tests/unit/verification/test_base.py#L22-L66: annotatetest_default_fields_match_contract,test_compound_result_carries_children_not_raw,test_leaf_result_carries_raw_not_children, andtest_no_details_fieldwith-> None.tests/unit/verification/test_verifier_registry.py#L33-L114: annotatetest_dummy_verifier_parses_without_union_edit,test_unknown_type_lists_registered_keys_in_error,test_already_parsed_unregistered_basemodel_is_rejected, andtest_unregistered_basemodel_as_checks_child_is_rejectedwith-> None.tests/unit/verification/test_package_import.py#L27-L39: annotatetest_import_pulls_no_heavy_sdkswith-> None.As per path instructions, "Ensure test functions have proper type annotations and clean structure" for
tests/**/*.py.🤖 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/verification/test_base.py` around lines 22 - 66, All listed test functions lack explicit return annotations. Add -> None to test_default_fields_match_contract, test_compound_result_carries_children_not_raw, test_leaf_result_carries_raw_not_children, and test_no_details_field in tests/unit/verification/test_base.py:22-66; test_dummy_verifier_parses_without_union_edit, test_unknown_type_lists_registered_keys_in_error, test_already_parsed_unregistered_basemodel_is_rejected, and test_unregistered_basemodel_as_checks_child_is_rejected in tests/unit/verification/test_verifier_registry.py:33-114; and test_import_pulls_no_heavy_sdks in tests/unit/verification/test_package_import.py:27-39.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 `@devops_bench/metrics/pipeline.py`:
- Around line 183-184: Update the ordered_keys construction in the pipeline
evaluator setup to include only builtin_keys that are present in METRICS, while
preserving the existing ordering and non-builtin metric inclusion. Keep
evaluators instantiation unchanged so every key passed to METRICS[k]() is
registered.
In `@devops_bench/verification/runner.py`:
- Around line 75-120: Update the wait_for_condition docstring to document that
timeout_sec values below _MIN_LEAF_BUDGET_SECONDS (1.0 second) cause bare leaf
checks to fail immediately without invoking verify(). Keep the existing deadline
and execution behavior unchanged.
---
Nitpick comments:
In `@devops_bench/metrics/pipeline.py`:
- Line 49: Update the _canonical_tool_name type signature to accept the
non-string input types it explicitly returns unchanged, and widen the return
annotation accordingly so it accurately reflects the implementation’s runtime
behavior.
In `@tests/unit/verification/test_base.py`:
- Around line 22-66: All listed test functions lack explicit return annotations.
Add -> None to test_default_fields_match_contract,
test_compound_result_carries_children_not_raw,
test_leaf_result_carries_raw_not_children, and test_no_details_field in
tests/unit/verification/test_base.py:22-66;
test_dummy_verifier_parses_without_union_edit,
test_unknown_type_lists_registered_keys_in_error,
test_already_parsed_unregistered_basemodel_is_rejected, and
test_unregistered_basemodel_as_checks_child_is_rejected in
tests/unit/verification/test_verifier_registry.py:33-114; and
test_import_pulls_no_heavy_sdks in
tests/unit/verification/test_package_import.py:27-39.
🪄 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: Pro
Run ID: 2eeaf720-e4a6-4874-bcf4-be7c3506317a
📒 Files selected for processing (11)
devops_bench/metrics/_skills.pydevops_bench/metrics/geval.pydevops_bench/metrics/pipeline.pydevops_bench/verification/__init__.pydevops_bench/verification/base.pydevops_bench/verification/runner.pydevops_bench/verification/spec.pytests/unit/metrics/test_metrics_geval.pytests/unit/verification/test_base.pytests/unit/verification/test_package_import.pytests/unit/verification/test_verifier_registry.py
- filter builtin metric keys to those registered in METRICS so the batch loop never instantiates a deferred (unregistered) family - document the sub-1s leaf budget floor on wait_for_condition - break the sequence loop once the deadline is hit and bulk-skip the remaining children via a shared _skip_rest helper - add self-contained runner tests (in-memory fake leaf) and pipeline tests (stub evaluators) that avoid importing un-migrated modules
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/unit/metrics/test_metrics_pipeline.py`:
- Around line 84-85: Tests and fixtures in
tests/unit/metrics/test_metrics_pipeline.py lack the required type annotations
and docstrings. At lines 25, import Iterator from collections.abc and Any from
typing; at lines 84-85, annotate registry with Iterator[Any]; at line 96, refine
_result to return dict[str, Any]; and at lines 101, 110, 122, 131, 142, and 154,
add the specified parameter and None return annotations to each test function
plus a brief docstring.
In `@tests/unit/verification/test_runner.py`:
- Line 92: The test functions in tests/unit/verification/test_runner.py at lines
92-92, 99-99, 106-106, 115-115, 128-128, 142-142, 165-165, 186-186, 198-198,
210-210, 225-225, and 236-236 all require proper annotations. Update each listed
test function to annotate agent as VerifierAgent and specify a None return type,
ensuring VerifierAgent is available through the existing imports.
- Line 84: Update the _leaf helper signature with explicit type annotations for
the keyword arguments and return value, replacing the generic dict return
annotation with a parameterized mapping type while preserving the helper’s
current behavior.
- Line 25: Update the typing imports in test_runner.py to include Any and
Iterator, then use them in the relevant fixture and helper function signatures
to satisfy the required type hints while preserving existing behavior.
- Around line 68-69: Add a return type annotation to the _register_fake_leaf
pytest fixture, using the appropriate type for its behavior while preserving its
autouse registration and existing implementation.
🪄 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: Pro
Run ID: 42a9527e-00ac-479a-82ac-859f1adcbad3
📒 Files selected for processing (4)
devops_bench/metrics/pipeline.pydevops_bench/verification/runner.pytests/unit/metrics/test_metrics_pipeline.pytests/unit/verification/test_runner.py
🚧 Files skipped from review as they are similar to previous changes (2)
- devops_bench/metrics/pipeline.py
- devops_bench/verification/runner.py
Address CodeRabbit maintainability review: annotate test functions, fixtures, and helpers, and add docstrings to satisfy the repo's type-hint and docstring guidelines for tests.
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eugeneng04, janetkuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
Flip the entries at blob parity with upstream, by upstream PR: - kubernetes-sigs/devops-bench#32 (model clients + CLI agent harnesses): agents/cli/gemini_cli/**, agents/cli/__init__.py - kubernetes-sigs/devops-bench#31 (chaos package): chaos/spec.py, chaos/__init__.py - kubernetes-sigs/devops-bench#29 (verification base + metrics judge): verification/base.py, verification/__init__.py, its base/package tests, metrics/geval.py, metrics/_skills.py, tests/unit/metrics/test_metrics_geval.py - kubernetes-sigs/devops-bench#30 (deployer abstraction + default stack): deployers/base.py, deployers/__init__.py, deployers/noop.py, both engine tests - kubernetes-sigs/devops-bench#34 (concrete judge metric families): metrics/__init__.py Drifted entries stay commented until gke-labs and upstream reconcile.
4 participants
Adds the verification framework base and the LLM-judge scoring pipeline.
Verification (
devops_bench/verification/)base.py— theVerifierabstract base and its registry.spec.py— pydantic models for declaring verification specs.runner.py— executes registered verifiers concurrently and aggregates results.Metrics judge (
devops_bench/metrics/)geval.py— G-Eval judge model wiring overdeepeval, resolving the judgeprovider/model from configuration.
pipeline.py— the scoring pipeline that runs judge metrics._skills.py— loads the packaged skill guides the judge reads.Unit tests for the base surface are co-located under
tests/unit/verification/and
tests/unit/metrics/.Concrete verifiers and concrete metric families build on these bases and land in
follow-ups; see the inline notes on
spec.pyandpipeline.pyfor how theyregister.
Summary by CodeRabbit
New Features
Tests