refactor: Introduce new Apify storage client

[vdusek]

Description

• Integration of the Crawlee v1 changes, mostly new storages & storage clients (introduced in refactor!: Introduce new storage client system crawlee-python#1194).

Issues

• Closes: Introduce new Apify storage client #469

Testing

• The current test set covers the changes.

[janbuchar]

Is this BC break worth it?

[vdusek]

let's evaluate all the potential BCs at the end

[janbuchar]

Sure. I thought we are nearing that now 😁

[vdusek]

Since we're just re-exporting the storages from crawlee here, there will be many more cases than this one. I'm not saying we have to rename this particular argument (and I will undo it if you insist—just I don't like those long identifiers, especially when we can use the KVS abbreviation).

[vdusek]

Probably let's wait for @janbuchar, and also I'll add an upgrading guide for v2.

[janbuchar]

I haven't made it through the request queue client, but here's a batch of comments.

[janbuchar]

Does this override mean that Crawlee does not treat INPUT.json differently from other files anymore and this logic is only added in the SDK? I'm a fan, but I wonder if the rest of the team is on board with this.

[vdusek]

Exactly - because Crawlee does not use INPUT.json for anything. We are in the same boat with @Pijukatel here.

@B4nan, would you be OK with this change? TLDR; so you don't have to study the code: As Honza wrote, Crawlee now treats INPUT.json just like any other file. Here in SDK, I've added a minimal override of FileSystemStorageClient that only customizes the purge method, preventing it from deleting INPUT.json. That's it.

[B4nan]

are we talking about the input.json in the project root? since that is implemented on crawlee level (in JS) and i think it should stay this way. i am not talking about the purging logic, in JS, if there is an INPUT.json file in the root, KVS.getInput() will prefer it over anything in the storage folder (the local/memory implementation, with apify client it is ignored completely). not sure if we even do this in the python version (and I am open to discuss if we want to remove this from v4, but it feels like a good thing to me)

if this is just about purging, i am not entirely sure i understand what it means, we dont want to autopurge inputs on crawlee level, crawlee shouldnt behave differently because you init the SDK (not locally).

[B4nan]

this is the part where we handle input file in the project root in JS

https://github.com/apify/crawlee/blob/5c4726df96e358a9bbf44a0cd2760e4e269f0fae/packages/core/src/storages/key_value_store.ts#L697

[janbuchar]

From the "Crawlee is a web scraping framework" point of view, handling input feels a bit redundant here - if you intend to only run the crawler outside of Apify, you'll probably want to accept input some other way - CLI arguments, environment variables, some domain-speciffic config file... Providing a helper for loading a JSON file from two possible locations doesn't add much value IMO.

Asking Apifiers about this doesn't make much sense in this situation, because basically everything we write is intended to run on Apify 🙂

[Pijukatel]

While testing the change on platform I used modified benchmark tests. They are specific, because they scrape locally hosted site and so the speed of scraping is unusually fast - no network related delays. This exposes RequestQueue race condition that can happen when checking if the RQ is empty or not.
When running the test using this branch, I saw that the crawler premature finished even though RQ was still quite full. The hypothesis about premature exit was confirmed when running same test on branch modified with extra log and sleep.

Please see following tiny commit based on this branch and attached log:
5b25728

From this run: https://console.apify.com/actors/UdJ0NQp2j8140G9db/runs/4yfZqLvLo1xhrk2Cb#log