Give a clear error for non-positive concurrency max_running#6282
Conversation
📝 WalkthroughWalkthroughThis change adds eager validation for non-positive ChangesRelated issues: Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConcurrencyLimit
participant ConcurrencyLimiter
Caller->>ConcurrencyLimit: __init__(max_running)
ConcurrencyLimit->>ConcurrencyLimit: __post_init__ validates max_running
ConcurrencyLimit-->>Caller: raise UserError (if max_running < 1)
Caller->>ConcurrencyLimiter: __init__(max_running)
ConcurrencyLimiter->>ConcurrencyLimiter: validate max_running before CapacityLimiter
ConcurrencyLimiter-->>Caller: raise UserError (if max_running < 1)
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@pydantic_ai_slim/pydantic_ai/concurrency.py`:
- Around line 78-81: The validation for max_running < 1 is duplicated in both
ConcurrencyLimit.__post_init__ and ConcurrencyLimiter.__init__, so consolidate
it into a shared helper or delegation point and reuse the same error message
from both places. Keep the eager validation behavior unchanged, but remove the
repeated inline checks so the logic lives in one identifiable function or method
used by both classes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e089df48-c793-4648-8af1-9c4707f2763b
📒 Files selected for processing (2)
pydantic_ai_slim/pydantic_ai/concurrency.pytests/test_concurrency.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)pydantic/pydantic(auto-detected)
UserError is pydantic-ai's convention for a developer-misconfiguration of a public API (see the model/output-type checks in agent/__init__.py and _output.py); a bare ValueError reads as an internal error and is what users would otherwise get opaquely from anyio. The message now names the remedy (max_concurrency=None) for the exact 0-vs-None confusion the issue calls out. Add a regression test that None still means no limiting.
|
David's AICA here: 🔍 Local review suite (automated, pre-merge)
Verdict: ✅ all 3 waves passed — 0 blocking, 0 required (8 reviewers ran; independent runtime probe confirmed the bug and the fix)
|
|
David's AICA here: thanks for the fix (and for reporting the issue). I pushed one commit to this branch as a maintainer edit with three small refinements:
The shared Note: |
|
@dsfaccini Thanks for the review and the follow-up commit. The For future PRs, is there already a short doc or rule for this kind of case? For example: if a public pydantic-ai API gets a config value that would create invalid internal state, should we fail early with If there is no rule yet, would it be useful to write it down somewhere small, maybe in the contributing docs, a simple check where possible, or the repo context used by AI assistants? That would make future PRs easier to keep consistent. Since the PR is green now and CodeRabbit is happy, do you think it is ready to merge? |
|
anyio rejects it anyway so I don't think this is needed apart from maybe a friendlier error message? |
Match the '>= 1, got N' phrasing used by OnlineEvaluator (pydantic#6267), note the constraint in the ConcurrencyLimit and ConcurrencyLimiter docstrings, and drop the stale 'before a run can hang' test wording.
max_runningmax_running
|
@adtyavrdhn Thanks a lot for taking a look, and for tightening the wording/docstrings here. I agree with your framing that this is mainly about giving a clearer public API error instead of letting lower-level limiter behavior leak through. I rechecked one detail locally though: So negative values are rejected by AnyIO, but Could you reproduce a different result on your side? If not, I think the Thanks again for the maintainer edits. CI is green now after the formatting fix, and I don't think there is anything else I need to change on my side. |
Eagerly reject non-positive concurrency
max_runningwith a clearUserErrorat construction, across every path:ConcurrencyLimit,ConcurrencyLimiter,from_limit,normalize_to_limiter, andAgent(..., max_concurrency=...).Noneremains the explicit no-limit value. Wording matches theOnlineEvaluator.max_concurrencyvalidation from #6267.Why
Since anyio 4.12,
CapacityLimiter(0)is allowed by design (agronholm/anyio#1019 —total_tokensis a mutable pause/resume "valve"; allowed in sync construction too since 4.14, agronholm/anyio#1183). pydantic-ai never mutates the limiter, soAgent(..., max_concurrency=0)constructs silently and the firstrun()hangs forever — reproduced onmainwith anyio 4.13. On anyio <= 4.11 the failure is anyio's opaquetotal_tokens must be >= 1instead. This PR turns both into an immediate, actionable error, uniform across the supported range (anyio>=4.5.0).Tests
tests/test_concurrency.pycovers0and negatives through every path above, plusNonestill meaning no limiting.Checklist