feat: Add deduplication to add_batch_of_requests

[Pijukatel]

Description

• Ensure that already known requests are excluded from api_client.batch_add_requests calls to avoid expensive and pointless API calls.
• Add all new requests to the cache when calling batch_add_requests.
• Add test with real API usage measurement.

Issues

• Closes: Ensure that duplicate links are handled in a cost effective way when using Apify RequestQueue #514

Testing

• Added new integration tests to verify reduced API usage.
• Comparing benchmark actor based on master vs this PR. Actor is a simple ParselCrawler that crawls the whole crawlee.dev, which contains many duplicate links, as the documentation is cross-linked thoroughly. Results:
  • Massive reduction of cost from request queue.
  • Significant overall speed up due to reduced API calls.

[janbuchar]

Maybe you could make two distinct Request instances with the same uniqueKey here?

[Pijukatel]

Yes, good point. I also added one more test to make it explicit that deduplication works based on unique_key only and unless we use use_extended_unique_key argument, some attributes of the request might be ignored. Another test makes this behavior clearly intentional to avoid some confusion in the future.

[janbuchar]

I guess you made sure that these do in fact run in parallel? To the naked eye, 100 requests doesn't seem like much, I'd expect that the event loop may run the tasks in sequence.

Maybe you could add the requests in each worker in smaller batches and add some random delays? Or just add a comment saying that you verified parallel execution empirically 😁

[Pijukatel]

I wrote the test for the implementation that did not take parallel execution into account, and it was failing consistently. So from that perspective, I consider the test sufficient.

Anyway, I added some chunking to make the test slightly more challenging. The parallel execution can be verified in the logs. For example, below. From the logs it can be seen that the add_batch_of_requests that was started first did not finish first - as it was "taken over" during it's await by another worker.

DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 10
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 20
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 90
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 80
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 0
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 40
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 50
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 60
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 30
DEBUG Tried to add new requests: 10, succeeded to add new requests: 10, skipped already present requests: 70
INFO  {'readCount': 0, 'writeCount': 100, 'deleteCount': 0, 'headItemReadCount': 0, 'storageBytes': 7400}

[janbuchar]

Any chance we could verify that the request was actually not cached between the two add_requests calls?

[Pijukatel]

This is checked implicitly in the last line where it is asserted that there was exactly 1 writeCount difference. The first call is "hardcoded" to fail, even on all retries, so it never even sends the API request and thus has no chance of increasing the writeCount.

The second call can make the write only if it is not cached, as cached requests do not make the call (tested in other tests). So this means the request was not cached in between.

I could assert the state of the cache in between those calls, but since it is kind of an implementation detail, I would prefer not to.

[janbuchar]

Fair enough, can you explain this in a comment then?

[Pijukatel]

Yes, added to the test description.

[janbuchar]

Did you mean "We are now sure that it was already handled..."? I'm not sure 😁

[Pijukatel]

Yes, that was not very clear. Updated

[janbuchar]

It's probably out of the scope of the PR, but it might be worth it to validate the response with a Pydantic model.

[Pijukatel]

That happens in the original code already few lines down: api_response = AddRequestsResponse.model_validate(response)

[janbuchar]

I'm sorry, I meant validating the whole response object with the two lists, so that you wouldn't need to do response['unprocessedRequests']

[Pijukatel]

I see, added.

[janbuchar]

Judging by apify/crawlee#3120, a day may come when we try to limit the size of _requests_cache somehow. Perhaps we should think ahead and come up with a more space-efficient way of tracking already added requests?

EDIT: hollup a minute, do you use the ID here for deduplication instead of unique key?

[Pijukatel]

Since there is this deterministic transformation function unique_key_to_request_id, which respects Apify platform way of creating IDs, this seems ok. If someone starts creating Requests with a custom id, then deduplication will most likely stop working.

There are two issues I created based on the discussion about this:

• Investigate caching options in ApifyRequestQueueClient #550
• Remove request.id crawlee-python#1358

[Mantisus]

LGTM

[vdusek]

Good job! LGTM (let's wait for Honza's approval as well)