Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage isn't purged for non-crawlee Apify SDKv3 projects #535

Closed
mvolfik opened this issue Mar 25, 2024 · 11 comments
Closed

Storage isn't purged for non-crawlee Apify SDKv3 projects #535

mvolfik opened this issue Mar 25, 2024 · 11 comments
Assignees
Labels
t-tooling Issues with this label are in the ownership of the tooling team.

Comments

@mvolfik
Copy link
Collaborator

mvolfik commented Mar 25, 2024

I have a project using "apify": "^3.1.0" in package.json, but I don't specify any Crawlee packages directly. However, in my code, I use Actor.openKeyValueStore() & friends. However, now the project is not detected as CRAWLEE, nor as PRE_CRAWLEE_APIFY_SDK, so no storage purge happens with run -p

This is the section of the 0.19.1..0.19.2 diff that broke it, this is from PR #475
image

@B4nan
Copy link
Member

B4nan commented Mar 25, 2024

Hmm, but with sdk v3 you should get the purge by default anyway, or not?

@mvolfik
Copy link
Collaborator Author

mvolfik commented Mar 25, 2024

that's what i thought
but looking into the code, just a line before this diff above, the CLI sets CRAWLEE_PURGE_ON_START = '0', and as you cans see, it reverts that to the default 1 only if it detects crawlee, which it doesn't for my project.

@B4nan
Copy link
Member

B4nan commented Mar 25, 2024

I see, @vladfrangu do you remember why it's like that? Feel's wrong, I'd say we should remove that line.

It looks like now when you use the Apify CLI, it won't purge stuff by default even for crawlee projects and will require explicit --purge flag? It shouldn't behave like that, it should be the exact opposite, purge by default for crawlee, and require --no-purge to opt-out (which disables the auto purge via env var).

@B4nan
Copy link
Member

B4nan commented Mar 25, 2024

Maybe better to keep those changes for the upcoming v1 release as they are indeed breaking and could surprise users if it currently works in the opposite way as it should.

edit: on the other hand, we broke it just with the latest version ~month ago...

@B4nan B4nan added the t-tooling Issues with this label are in the ownership of the tooling team. label Mar 25, 2024
@vladfrangu
Copy link
Member

it should be the exact opposite, purge by default for crawlee, and require --no-purge to opt-out (which disables the auto purge via env var).

This is edging undefined behavior terrain very fast. I also don't see why pre-crawlee should be purge opt-in but post-crawlee should be purging opt-out.

I'd say lets clean up all of this logic in v1 (including for older sdks) -> always purge, opt out only. What I tried with that PR (but missed this case, which is easy to fix actually) is to make the crawlee behavior also opt in, like for old sdk.

@B4nan
Copy link
Member

B4nan commented Mar 25, 2024

I also don't see why pre-crawlee should be purge opt-in but post-crawlee should be purging opt-out.

Because that's what we changed on purpose with v3. In v2 you had to use the CLI to purge, with v3 we purge by default, and we wanted to keep that behavior as changing it is breaking.

always purge, opt out only

Yeah, that might be a good approach for v1. Thoughts @mvolfik?

@mvolfik
Copy link
Collaborator Author

mvolfik commented Mar 25, 2024

always purge, opt out only

Yeah, that might be a good approach for v1. Thoughts @mvolfik?

If the --no-purge flag is easily found, and you don't have to dig for some CRAWLEE_NO_PURGE env var (this is a part of the reason I was opposed to this change with v3 when we spoke about this last time), let's go with it. A disadvantage is that the default behavior is destructive (deletes data), and I'm so used to running apify run -p that I don't consider it a hassle anymore, but for common development workflow probably default=purge makes more sense.

@mvolfik
Copy link
Collaborator Author

mvolfik commented Mar 25, 2024

Nonetheless, unless the 1.0 is next week, can you please fix the original issue of this thread for 0.19? thanks

@B4nan
Copy link
Member

B4nan commented Mar 26, 2024

and I'm so used to running apify run -p

Yeah, and that's the root of the problem, people in Apify are used to this while newcomers are confused by it.

@vladfrangu let's fix this in v0.19 too please

If the --no-purge flag is easily found

How often do you actually want this behavior? I would say you want this only when testing resurrection of an actor, otherwise never? Which was one of the reasons we did the behaviour switch in the first place.

@B4nan
Copy link
Member

B4nan commented Mar 26, 2024

@mvolfik 0.19.3 is out, let us know how it works

@B4nan
Copy link
Member

B4nan commented Apr 3, 2024

Closing as the fix is now also in the master branch.

@B4nan B4nan closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

No branches or pull requests

3 participants