Repository navigation
Conversation
There was a problem hiding this comment.
As I mentioned above, this is much less convenient than before. You now need an additional import and an isinstance() check.
Maybe we could introduce helper methods for checking statuses, so determining whether an RQ is in a particular status would be a single method call?
There was a problem hiding this comment.
ok, then let's use literal comparison at least, so users aren't pushed into an extra import
There was a problem hiding this comment.
@janbuchar Have you considered keeping is_finished as a simple helper that wraps
isinstance(await rq.check_readiness(), FinishedStatus)?
We could add similar helpers for the other statuses where they make sense.
I mean, the new API is a bit worse in terms of ergonomics (maybe with AI agents it doesn't matter that much?).
There was a problem hiding this comment.
ok, then let's use literal comparison at least, so users aren't pushed into an extra import
Implemented in 883a68c
3629140 to
a30518d
Compare
|
|
||
| request_manager = await self.get_request_manager() | ||
| return not await request_manager.is_empty() | ||
| return (await request_manager.check_readiness()).status == 'ready' |
There was a problem hiding this comment.
Note: The pool calls __is_finished_function and then __is_task_ready_function on every pass, and both now run check_readiness(). On a queue that is empty but not finished, each call makes two backend calls (is_empty() + is_finished()), so a pass costs 4 calls where it used to cost 2. That covers the end of a crawl, waiting on in-progress requests, and throttler cooldowns. On the platform these are API calls, and ThrottlingRequestManager repeats them for every domain queue. Could the status from __is_finished_function be reused here, or is that too tied to the call order in the pool?
✍️ Drafted by Claude Code
| if first.status == 'waiting': | ||
| if second.status != 'waiting' or second.ready_at is None: | ||
| return first | ||
| if first.ready_at is not None and first.ready_at <= second.ready_at: | ||
| return first | ||
| return second |
There was a problem hiding this comment.
Question: a waiting status with ready_at=None means requests are in progress or being added, so work can show up any moment. Joining it with ready_at=T returns T, which promises nothing before T. Nothing reads ready_at yet, but it's public. Shouldn't None win here? I see test_join_keeps_known_ready_at pins the current behavior, so maybe I'm missing the intent.
✍️ Drafted by Claude Code
There was a problem hiding this comment.
It's intentional, and JS joins them the same way ("the earlier of the two known wake-up times, unknown only if neither source announced one"). ready_at isn't a promise that nothing shows up before it. It says when a wake-up is worth scheduling, on top of the regular polling AutoscaledPool does anyway. I added a comment to the join and clarified the ready_at docstring.
There was a problem hiding this comment.
Note: a stall wins here even if a candidate's inner queue is waiting only because a background add_requests batch is still running, so an idle crawler can raise PersistentRateLimitError while new requests are arriving. The old get_stall_reason did the same, so this isn't a regression and is fine as a follow-up.
✍️ Drafted by Claude Code
There was a problem hiding this comment.
Good catch. I fixed it here, since this PR is what sets the order between the statuses.
caac18c to
bbe9b52
Compare
bbe9b52 to
09c2557
Compare

Description
RequestLoaderandRequestManagerreplaceis_empty()andis_finished()with a singlecheck_readiness(). It returns one of four statuses, exported fromcrawlee.request_loaders:ReadyStatus,WaitingStatus,StalledStatusorFinishedStatus.WaitingStatus.ready_atsays when a source expects to have work again, andThrottlingRequestManagersets it from its cooldowns.StalledStatus.reasonsays why a source can't make progress. Storage clients keepis_empty()andis_finished(). The upgrade guide covers the migration.ThrottlingRequestManager.get_stall_reason()is removed. The crawler reads a stall from the status, so it now also detects a stalled domain when theThrottlingRequestManageris wrapped in aRequestManagerTandem.RequestManagerTandemmoves a request from its loader only when the loader is ready, so aSitemapRequestLoaderthat's still loading sitemaps no longer holds up the requests already in the manager.RequestLoader.fetch_next_request()no longer has to wait for a request. ANonemeans nothing is pending right now, not that the loader is finished.SitemapRequestLoader.fetch_next_request()returnsNoneonce its sitemaps are loaded, instead of waiting until every fetched request is handled.Issues
is_empty/is_finishedwithcheck_readiness()and add pacing signals toRequestManager#2296✍️ Drafted by Claude Code