Repository navigation
fix: Clamp the default desired_concurrency to min/max concurrency - #2397
PrashanthLande wants to merge 1 commit into
Conversation
|
I am questioning whether this is really needed? The error is pretty obvious and it comes right at the start. While this might be convenient for someone, it will make the function try to guess the users intent and adapt the input accordingly. The unconditional default becomes quite blurry as it will become a conditional default - which is hidden inside a code. I would prefer to keep it as it is. |
|
Thanks for the quick review. I see the concern about a conditional default, but from the user side the current behavior feels like the hidden part: setting only max_concurrency=5 is a complete, valid intent (“never run more than 5”), yet it fails because of a default of 10 that the user never set and may not know exists. To fix it, they need to know about that internal default and pass a third parameter just to satisfy it. Lowering max_concurrency below 10 is a common case (polite crawling, memory-heavy Playwright runs), and before #1419 it worked without extra arguments. Crawlee JS also clamps in this situation (Math.min(desiredConcurrency ?? minConcurrency, maxConcurrency)). The change keeps 10 as the default whenever it fits, and explicit conflicting values still raise. That said, it’s your call. If you’d still prefer to keep it as is, I’m happy to close this, or to reduce it to a clearer error message. |
Description
ConcurrencySettings(max_concurrency=5)andConcurrencySettings(min_concurrency=20)raiseValueErrorbecause the hard-coded defaultdesired_concurrency=10is checked againstthe bounds, even when the user never set it.
desired_concurrencytoNoneand resolve it to 10, clamped betweenmin_concurrencyandmax_concurrency.ConcurrencySettings()still gives 10, and an explicitly passed out-of-range value still raises the sameValueError.desired_concurrencyfell back tomin_concurrency. fix: Set default desired concurrency for non-browser crawlers to 10 #1419 raised the default to 10 and added the bounds checks in the same change. I couldn't find anydiscussion of whether rejecting bounds-only configs was intended.
Issues
Testing
tests/unit/test_types.py; 3 of its 8 cases fail without the fix.Checklist