fix(evaluation): reject num_samples=0 in JudgeModelOptions at construction time - #6939
Open
gaurav-gandhi-2411 wants to merge 1 commit into
Open
Conversation
…ction time parallelism_limit already has ge=1; num_samples never did, despite being the same kind of count-like field in the same class. num_samples=0 is never an intentional value anywhere in this codebase, and silently causes LlmAsJudge.evaluate_invocations to drop the affected invocation from results entirely with no error and no NOT_EVALUATED marker.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔴 Required Information
Describe the Bug:
JudgeModelOptions.parallelism_limithasge=1but its sibling fieldnum_sampleshas no lower-bound constraint at all.ge=1was added toparallelism_limitby commit 0dbb88c ("feat: parallelize LLM-as-judgeevaluation using asyncio.gather()", 2026-08-19), but that commit did not touch
num_samples. Across this file's entire visible history,num_sampleshasnever carried a lower bound.
num_samples=0is never an intentional value anywhere in this codebase — arepo-wide grep of every
num_samplesusage insrc/andtests/finds only1, 3, or the default 5; nothing ever sets or derives 0. It is, however, a real
live bug surface:
LlmAsJudge.evaluate_invocationssilently drops anyinvocation with zero samples from the result set entirely, with no error and
no NOT_EVALUATED marker. (The "judge model yields no response" exception path
is a separate, already-fixed case on
main; this PR is narrower andorthogonal, targeting only the
num_samples=0misconfiguration itself.)Steps to Reproduce:
JudgeModelOptions(num_samples=0) # succeeds silently today
Expected Behavior:
Same as
JudgeModelOptions(parallelism_limit=0)today — apydantic.ValidationErrorat construction time.Observed Behavior:
Constructs successfully, then silently drops every invocation evaluated by it
from the aggregated result.
Changes
src/google/adk/evaluation/eval_metrics.py: addge=1toJudgeModelOptions.num_samples, matching the existing convention forcount-like fields in this package — two other examples:
run_config.py: max_workersandcontext_cache_config.py:cache_intervals, bothge=1.tests/unittests/evaluation/test_eval_config.py: addtest_judge_model_options_rejects_zero_num_samples, assertingJudgeModelOptions(num_samples=0)raisespydantic.ValidationError.Note for reviewers: this makes a defensive
if not samples: return NOT_EVALUATEDbranch insimulation/per_turn_user_simulator_quality_v1.py(~lines 365-369) unreachable via the public constructor — that branch was
never exercised by an intentional
num_samples=0, so removing it is optionalcleanup, not required here; left untouched to keep this diff minimal.
Testing
No regressions in either file.
Risk & rollback
Additive validation only — narrows accepted input, does not change behavior
for any value ever actually used (1, 3, 5 across every existing caller/test).
Revert is a single-line removal of
ge=1.