Skip to content

fix(evaluation): reject parallelism=0 in EvaluateConfig/InferenceConfig - #7000

Open
gaurav-gandhi-2411 wants to merge 1 commit into
google:mainfrom
gaurav-gandhi-2411:fix/eval-parallelism-floor
Open

fix(evaluation): reject parallelism=0 in EvaluateConfig/InferenceConfig#7000
gaurav-gandhi-2411 wants to merge 1 commit into
google:mainfrom
gaurav-gandhi-2411:fix/eval-parallelism-floor

Conversation

@gaurav-gandhi-2411

Copy link
Copy Markdown
Contributor

Related

#6951 is where this gap was publicly established: a repo-wide sweep for
floor-less count/limit parameters in src/google/adk/evaluation/*.py,
posted as a comment on that issue, first flagged
EvaluateConfig.parallelism/InferenceConfig.parallelism alongside several
other candidates (a collaborator, @surajksharma07, engaged on the thread and
confirmed the related evaluate_eval_set empty-collection defect). A
follow-up comment on the same issue narrowed the sweep after two of the
other candidates turned out weaker than first stated, and reaffirmed this
one — EvaluateConfig.parallelism/InferenceConfig.parallelism at 0 — as
the confirmed, evidence-backed gap this PR fixes.

🔴 Required Information

Describe the Bug:
EvaluateConfig.parallelism and InferenceConfig.parallelism
(base_eval_service.py:46,72) have no lower-bound constraint. Both values
are forwarded, unclamped, straight to asyncio.Semaphore(value=parallelism)
(local_eval_service.py:186,216). A negative value fails loud —
asyncio.Semaphore itself raises ValueError immediately on construction.
A zero value fails silent: the semaphore constructs successfully, and every
subsequent .acquire() blocks forever, since a Semaphore(0) can never be
released by anything. In LocalEvalService.perform_inference/evaluate
this means the CLI (or any caller) hangs indefinitely with no exception, no
error message, and no evaluation ever completing.

Steps to Reproduce:

>>> import asyncio
>>> asyncio.run(asyncio.wait_for(asyncio.Semaphore(0).acquire(), timeout=5.0))
# TimeoutError after 5s — hangs, does not raise on construction

>>> asyncio.Semaphore(-1)
# ValueError('Semaphore initial value must be >= 0') — raised immediately

Expected Behavior:
Same as the existing convention for count/limit-like fields elsewhere in
this package — pydantic.ValidationError at construction time for a
degenerate value, not a silent hang at call time.

Observed Behavior:
EvaluateConfig(eval_metrics=[...], parallelism=0) and
InferenceConfig(parallelism=0) both construct successfully today, then
hang the first evaluation/inference run indefinitely.

Changes

  • src/google/adk/evaluation/base_eval_service.py: add ge=1 to
    EvaluateConfig.parallelism and InferenceConfig.parallelism, matching
    the existing convention for count-like fields in this package —
    JudgeModelOptions.parallelism_limit (eval_metrics.py),
    ToolThreadPoolConfig.max_workers (run_config.py), and
    ContextCacheConfig.cache_intervals (context_cache_config.py) are all
    already ge=1.
  • tests/unittests/evaluation/test_base_eval_service.py (new file): add
    test_evaluate_config_rejects_zero_parallelism and
    test_inference_config_rejects_zero_parallelism, each asserting
    parallelism=0 raises pydantic.ValidationError.

Testing

pytest tests/unittests/evaluation/test_base_eval_service.py tests/unittests/evaluation/test_local_eval_service.py -q
→ 26 passed

pytest tests/unittests/evaluation -q
→ 872 passed

No regressions in either run.

Risk & rollback

Additive validation only — narrows accepted input, does not change behavior
for any value ever actually used (the default is 4; no caller or test in the
repo constructs either config with parallelism<=0). Revert is a two-line
removal of ge=1.

parallelism is forwarded unclamped to asyncio.Semaphore(value=parallelism)
(local_eval_service.py:186,216). A negative value fails loud (Semaphore
itself raises ValueError); 0 fails silent -- the semaphore constructs fine
and every subsequent .acquire() hangs forever, since Semaphore(0) can never
be released. Add ge=1, matching JudgeModelOptions.parallelism_limit,
ToolThreadPoolConfig.max_workers, and ContextCacheConfig.cache_intervals,
which already carry the same constraint.

Gap first raised on google#6951.

Claude-Session: https://claude.ai/code/session_019SUgxvPkwsx4AasNXyU8V2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants