Ensure that duplicate links are handled in a cost effective way when using Apify RequestQueue

[Pijukatel]

In JS version fo RQ, there is this and this, which deal with duplicate links that would otherwise dramatically increase RequestQueue writes. Ensure that similar handling is done in Python version of the tooling.

For more context:
https://apify.slack.com/archives/C02JQSN79V4/p1753358299203499

Consider solving this in Crawlee level if possible

[janbuchar]

Consider solving this in Crawlee level if possible

Definitely possible, but I think it can stay in ApifyRequestQueueClient

[B4nan]

Why would we do that? It literally means ignoring the problem on crawlee level (where it is a problem as well, rather a perf issue than a cost issue like on the platform, but still an issue).

[janbuchar]

We can either try to handle deduplication in a general way in storages.RequestQueue, or we can make it a requirement of the storage_clients.RequestQueueClient interface (either a soft requirement explained in docs or some method(s) that would guarantee this).

The current state in crawlee-python is the latter (explained in the docblock of RequestQueueClient.add_batch_of_requests). There are now two implementations of the interface that resolve this on their own: MemoryRequestQueueClient, which has a unique_key => request dictionary and FilesystemRequestQueueClient, which handles this with a file system traversal. The first one does not need any additional optimization (it would needlessly consume memory), the second (file system) does not seem optimal (@vdusek am I reading the code right?).

For a future SQL-backed storage client, I believe that a database-level upsert will be more reliable and efficient than anything we can manage on the Crawlee side.

In the Apify platform client, handling this on the client side reduces usage costs.

It seems like a 2:2 tie to me 😁

[vdusek]

I believe deduplication should be the responsibility of the storage client, and that's how it's currently implemented. Mainly because the optimal deduplication strategy is tightly coupled with the "storage backend" itself.

As Honza pointed out...

• For in-memory storage, we can use a dictionary keyed by unique_key, which should be easy and optimal.
• The filesystem storage client isn't currently optimal (we discussed this about a month ago during a team sync and agreed to optimize it after the v1.0 release).
• For the Apify platform client, we should implement something on the client side (and it can be even shared with the file-system client, for example, if it makes sense).
• SQL-based storage clients can handle deduplication at the database.
• Other future potential storage clients could also implement deduplication using the built-in mechanisms, e.g., Redis and its Sets (I don't know if Redis could be a good candidate, just an example).

So to me, from a design perspective, it makes sense for each storage client to implement deduplication in the way that best fits its backend. Crawlee should only define the expected behavior, but not enforce a specific implementation.

[B4nan]

So instead of dealing with this in one place, we will deal with it multiple places, that's the part I don't like. If we deal with this on RQ level, you will save the bandwidth for every storage client. It will help with all of them, even if we have SQL backed one, it's better not to reach to the database if you don't need to, checking an LRU in memory will always be faster than anything else. Only for the memory storage client it adds a memory overhead, the others I don't really see how it would be better to delegate it to those.

Also thinking about the reason why we wanted to simplify the storage interface - to allow people to create their own easily - now you basically tell them "deal with this issue we have in our library in your extenions".

For the Apify platform client, we should implement something on the client side (and it can be even shared with the file-system client, for example, if it makes sense).

If by this you mean the actual apify client, that's a no-no for me, we should have any caching on this level. But I guess you mean the SDK wrapper?

[Pijukatel]

Can we just have a default implementation of it in RequestQueueClient in Crawlee and any other implementation can override it if needed?

[janbuchar]

So instead of dealing with this in one place, we will deal with it multiple places, that's the part I don't like.

It's not that simple - it's more like a) deal with it once-and-for-all in the storage clients or b) deal with most of it in storages.RequestQueue, with a) on top if you want to be able to handle more requests than the size of the LRU cache or multiple clients at once.

If we deal with this on RQ level, you will save the bandwidth for every storage client. It will help with all of them, even if we have SQL backed one, it's better not to reach to the database if you don't need to, checking an LRU in memory will always be faster than anything else. Only for the memory storage client it adds a memory overhead, the others I don't really see how it would be better to delegate it to those.

I wouldn't be so sure about the performance of a LRU-cache implemented in Python against optimized code in a database server. Sure, there are additional costs for hitting the DB, but I'd make a benchmark before drawing conclusions.

Also thinking about the reason why we wanted to simplify the storage interface - to allow people to create their own easily - now you basically tell them "deal with this issue we have in our library in your extenions".

As I wrote above, unless you're writing something very specific where the deduplication doesn't need to be that reliable, you have to deal with this either way 🤷

[Pijukatel]

I think this gives you the most flexibility and re-use at the same time

class RequestQueueClient(ABC):
    @abstractmethod
    async def add_batch_of_requests_direct(
        self,
        requests: Sequence[Request],
        *,
        forefront: bool = False,
    ) -> AddRequestsResponse:
   "Just abstract..."

    async def add_batch_of_requests(
        self,
        requests: Sequence[Request],
        *,
        forefront: bool = False,
    ) -> AddRequestsResponse:
    "Decent default implementation that will call self.add_batch_of_requests_direct"
...

[vdusek]

If by this you mean the actual apify client, that's a no-no for me, we should have any caching on this level. But I guess you mean the SDK wrapper?

Sorry, of course, you're correct, I meant the Apify storage client in the SDK (wrapper of Apify Python API client).

[Pijukatel]

(Maybe even the JS version still has some RQ handling issues? Similar issue just for reference: https://github.com/apify/store-website-content-crawler/issues/436)

[Pijukatel]

So I did a bunch of draft implementations to see where it makes the most sense.

• Implemented in the base class

  • This requires to add extra method like add_batch_requests_direct and have default implementation of add_batch_requests in base class which wraps the call to add_batch_requests_direct (or do some __gettatribute__ black magic), kind of too complex.
  • Not very good as it requires defining cache in base class already and then children can have their own caches(for other reasons) that would be sort of duplicate.

• Implemented in children class

  • this is in my opinion most convenient. It is simple, straight forward and it can benefit from existing children specific caching mechanisms. Draft with some extra logging

• Implemented in base class with decorator that can be reused in children classes

  • It is similar to previous one, but it can be applied in children like following snippet, where _is_in_cache and _add_to_cache are some callbacks to work with whatever cache the children defines. The typing of this solution would be really terrible and I think it is in general overcomplicated. Example draft for discussion

@with_add_request_cache(_is_in_cache, _add_to_cache)
async def add_batch_of_requests(...)
    ...

Bottom line is: Children client knows best how to implement caching. It is real problem for us only in Apify client (it makes it very expensive), so there is no point in trying to come up with generic solution that solves it for future theoretical clients...