Skip to content

feat: Add support for NDU storages #1401

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 12, 2025
Merged

feat: Add support for NDU storages #1401

merged 8 commits into from
Sep 12, 2025

Conversation

vdusek
Copy link
Collaborator

@vdusek vdusek commented Sep 9, 2025 •
edited
Loading

Description

Issues

Testing

  • New tests were implemented.

Checklist

  • CI passed

@vdusek vdusek self-assigned this Sep 9, 2025
@vdusek vdusek added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 9, 2025
@github-actions github-actions bot added this to the 123rd sprint - Tooling team milestone Sep 9, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Sep 9, 2025
@vdusek vdusek added the adhoc Ad-hoc unplanned task added during the sprint. label Sep 9, 2025
@vdusek vdusek marked this pull request as ready for review September 9, 2025 13:55
@vdusek
Copy link
Collaborator Author

vdusek commented Sep 9, 2025

cc @Mantisus in context of #1339

@janbuchar
Copy link
Collaborator

Can you add some docs/examples please? 🙂

Mantisus
Mantisus reviewed Sep 10, 2025
View reviewed changes
Copy link
Collaborator

@Mantisus Mantisus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@vdusek vdusek requested a review from Mantisus September 10, 2025 09:28
@vdusek
Copy link
Collaborator Author

vdusek commented Sep 10, 2025

Can you add some docs/examples please? 🙂

I extended the Storages guide.

Mantisus
Mantisus reviewed Sep 10, 2025
View reviewed changes
@Pijukatel Pijukatel mentioned this pull request Sep 10, 2025
Open
@vdusek vdusek requested a review from Mantisus September 11, 2025 07:08
Pijukatel
Pijukatel reviewed Sep 11, 2025
View reviewed changes
@Pijukatel
Copy link
Collaborator

Should StorageMetadata contain some information regarding the alias or scope? How will, for example Apify platform know what kind of storage it is?

@vdusek
Copy link
Collaborator Author

vdusek commented Sep 11, 2025

Should StorageMetadata contain some information regarding the alias or scope? How will, for example Apify platform know what kind of storage it is?

I don't think so. On the Apify platform, the distinction between global scope and run scope storage is based just on naming - unnamed versus named. The alias is just for us (FS and Apify clients use it, or will use it).

@vdusek vdusek requested a review from Pijukatel September 11, 2025 07:28
Mantisus
Mantisus approved these changes Sep 11, 2025
View reviewed changes
Copy link
Collaborator

@Mantisus Mantisus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Pijukatel
Pijukatel approved these changes Sep 12, 2025
View reviewed changes
Copy link
Collaborator

@Pijukatel Pijukatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets merge it and improve if needed based on real usage feedback.

janbuchar
janbuchar reviewed Sep 12, 2025
View reviewed changes
Copy link
Collaborator

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me! I have some nitpicky comments, plus I'd like to ask if you already tried implementing this in ApifyStorageClient. It would be good to see that before merging this part (not mandatory I guess...).

src/crawlee/storage_clients/_memory/_key_value_store_client.py
@@ -51,6 +51,7 @@ async def open(
*,
id: str | None,
name: str | None,
alias: str | None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um, shouldn't this actually be, you know, used somewhere in the method? I imagine you could just do if alias: name = alias, but I don't see that here - am I missing something?

I guess this might work that to the StorageInstanceManager doing the actual work needed to distinguish the instances, but it's kinda hard to see at first, if that's the case. Also we might want to put the alias in the metadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed on Slack, the alias does not have any effect on the memory storage client implementation. I added additional explanatory text to the docstrings.

Also we might want to put the alias in the metadata?

We might, but I'm not 100% sure about it. I would say we can better add it later than remove it later. So I would suggest keeping it as it is and adding it later if we have any reasoning/request for it.

tests/unit/storages/test_dataset.py
Comment on lines +651 to +652
# Clean up
await dataset_1.drop()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the storage_client fixture take care of this? And if it doesn't, it definitely should...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StorageClient class does not have access, and so cannot drop the specific storage clients that were created using it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I guess it's fine then, even though you could theoretically set up some monkey patching to get that access

@vdusek
Copy link
Collaborator Author

vdusek commented Sep 12, 2025

Looks pretty good to me! I have some nitpicky comments, plus I'd like to ask if you already tried implementing this in ApifyStorageClient. It would be good to see that before merging this part (not mandatory I guess...).

I've got it working, it just needs some more polishing and tests. Anyway, there were no issues with the implementation, and it's pretty straightforward. Also, no BCs, as expected.

@vdusek vdusek requested a review from janbuchar September 12, 2025 15:59
@vdusek
Copy link
Collaborator Author

vdusek commented Sep 12, 2025

I'm merging this as all the comments were clarified and/or addressed, so that we can adopt this in #1386 and #1339.

@vdusek vdusek merged commit 5dbd212 into master Sep 12, 2025
19 checks passed
@vdusek vdusek deleted the add-ndu-storages branch September 12, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pijukatel Pijukatel Pijukatel approved these changes

@janbuchar janbuchar Awaiting requested review from janbuchar

+1 more reviewer

@Mantisus Mantisus Mantisus approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@vdusek vdusek

Labels
adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Milestone
123rd sprint - Tooling team
Development

Successfully merging this pull request may close these issues.

Add support for non-default unnamed storages
4 participants
@vdusek @janbuchar @Pijukatel @Mantisus