Add `DataChain.listings()` method and use it in getting storages #331

ilongin · 2024-08-20T23:40:53Z

This PR adds class method similar to DataChain.datasets() called DataChain.listings() which returns list of ListingInfo objects from chain.
ListingInfo describes one specific cached listing (version) and is simply sub-classing DatasetInfo which is returned from mentioned DataChain.datasets() as listing is just special form of dataset.
The idea is to use this instead of deprecated Storage class which will be, along side with related codebase, removed in upcoming followups.

Additional changes:

Using Dataset.listings() in CLI method ls_local which lists storages / listings
Removed couple of unused Catalog methods: unlist_source, storage_stats, ls_storage_uris, get_storage, ls_storages and underlying metastore methods
Fixing dataset dependencies as now there is no special "storage" table which can be references and storage type dependency is also pointing to dataset (just listing type of dataset)

Blocked by #294

cloudflare-workers-and-pages · 2024-08-20T23:43:13Z

Deploying datachain-documentation with Cloudflare Pages

Latest commit:	`2d7fb35`
Status:	✅ Deploy successful!
Preview URL:	https://d9141fbd.datachain-documentation.pages.dev
Branch Preview URL:	https://ilongin-329-refactor-storage.datachain-documentation.pages.dev

View logs

codecov · 2024-08-21T00:01:26Z

Codecov Report

Attention: Patch coverage is 92.06349% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.81%. Comparing base (2952eb1) to head (2d7fb35).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/datachain/dataset.py	84.21%	1 Missing and 2 partials ⚠️
src/datachain/lib/listing_info.py	91.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   86.78%   86.81%   +0.03%     
==========================================
  Files          92       93       +1     
  Lines       10069    10078       +9     
  Branches     2047     2048       +1     
==========================================
+ Hits         8738     8749      +11     
+ Misses        988      984       -4     
- Partials      343      345       +2

Flag	Coverage Δ
datachain	`86.74% <92.06%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… objects for each cached listing

for more information, see https://pre-commit.ci

…datachain into ilongin/329-refactor-storages

src/datachain/data_storage/metastore.py

shcheklein · 2024-09-04T02:33:11Z

src/datachain/dataset.py

+        dependency_name = dataset_name
+
+        if is_listing_dataset(dataset_name):
+            dependency_type = DatasetDependencyType.STORAGE  # type: ignore[arg-type]


should we use LISTING as a type (since we use listing as term in some other places)

I was thinking about this as well. Not sure, but both is correct IMO

Not sure, but both is correct IMO

yes, but it just make it simpler to maintain, search, understand

not critical, but I would try to go everywhere with the same term

I will consider that in upcoming PRs just to not block this one

shcheklein · 2024-09-04T02:42:06Z

src/datachain/lib/dc.py

+        object_name: str = "listing",
+        **kwargs,
+    ) -> "DataChain":
+        """Generate chain with list of cached listing.


listing -> listings

Q: do we really need to make it a public dc method? (I don't have any strong opinion on this - just thinking if we can for now expose less and see if there are scenarios for this)

does it mean we will need from_listing btw? (just trying to think if we can keep at all about datasets / datachain and avoid exposing "listing" concept to end users (?)

Yea, we do have usage of both listings and storages words around interface which is ok IMO. Listing is just listed storage so I don't think those two words cannot live together.
I don't think we need from_listing as we have from_storage which in background creates new listing if doesn't exist and uses it then.

I think it's ok for us to expose this as we will be using it in Studio for example which can also be seen as one client.

shcheklein · 2024-09-04T03:17:47Z

src/datachain/lib/dc.py

+        object_name: str = "listing",
+        **kwargs,
+    ) -> "DataChain":
+        """Generate chain with list of cached listing.


it would be great to explain a bit what the listing is - just a few words

Added one sentence

src/datachain/lib/dc.py

shcheklein

LGTM, my only question if we really want to expose this as method that returns datachain (what is the use case for this?)

also the name listing vs storage that we use in some places

ilongin · 2024-09-04T12:40:53Z

LGTM, my only question if we really want to expose this as method that returns datachain (what is the use case for this?)

also the name listing vs storage that we use in some places

About exposing it as a method that returns DataChain instance, I wanted to be consistent with similar .datasets() method. If we want to change this, both methods needs to be refactored together

ilongin marked this pull request as draft August 20, 2024 23:40

ilongin changed the base branch from main to ilongin/266-refactor-from-storage August 20, 2024 23:41

ilongin linked an issue Aug 20, 2024 that may be closed by this pull request

Refactor Catalog storages methods to use new listing mechanism #329

Closed

ilongin mentioned this pull request Aug 24, 2024

Refactor DataChain.from_storage() to use new listing generator #294

Merged

Base automatically changed from ilongin/266-refactor-from-storage to main September 2, 2024 09:21

ilongin added 21 commits September 2, 2024 11:52

first version of from_storage without deprecated listing

44552e1

first version of from_storage without deprecated listing

1e64b7f

fixing tests and removing prints, refactoring

1f1b7b3

refactoring listing static methods

5fbcfc7

fixing non recursive queries

a922e0a

using ctc in test session

a09a2c8

fixing json

1d2a8b0

added DataChain.listings classmethod that returns list of ListingInfo…

76e2cce

… objects for each cached listing

another test for listings

ad98378

removed not needed filters

3587f8b

refactoring test

925c7cd

removed not needed catalog storage methods and their related codebase

8714fcd

fixing windows tests

e363c13

returning to all tests

38503d1

removed unlist_source method and related codebase

48931f6

fixing dataset dependencies

1f3f7aa

added session on cloud test catalog and refactoring tests

5a56062

using new listings method in from_storage

40e83f6

fixing test

84ad5da

fixing test

ed4f4a1

added dataset name dependencies test and fixes

8537347

ilongin force-pushed the ilongin/329-refactor-storages branch from 2e125ae to 8537347 Compare September 2, 2024 12:52

[pre-commit.ci] auto fixes from pre-commit.com hooks

18a0a01

for more information, see https://pre-commit.ci

ilongin added 2 commits September 2, 2024 14:59

small refactoring

14aadc0

Merge branch 'ilongin/329-refactor-storages' of github.com:iterative/…

d74f4bf

…datachain into ilongin/329-refactor-storages

ilongin marked this pull request as ready for review September 2, 2024 13:21

ilongin requested review from dtulga, dmpetrov, rlamy and dreadatour September 2, 2024 13:21

Merge branch 'main' into ilongin/329-refactor-storages

bf1e24e

ilongin requested review from mattseddon, skshetry and shcheklein September 3, 2024 16:39

shcheklein reviewed Sep 4, 2024

View reviewed changes

src/datachain/data_storage/metastore.py Show resolved Hide resolved

shcheklein reviewed Sep 4, 2024

View reviewed changes

src/datachain/data_storage/metastore.py Show resolved Hide resolved

shcheklein reviewed Sep 4, 2024

View reviewed changes

src/datachain/lib/dc.py Outdated Show resolved Hide resolved

shcheklein approved these changes Sep 4, 2024

View reviewed changes

ilongin added 2 commits September 4, 2024 14:23

Merge branch 'main' into ilongin/329-refactor-storages

15afd42

refactor comments

97297bf

Merge branch 'main' into ilongin/329-refactor-storages

2d7fb35

ilongin merged commit 576b69a into main Sep 5, 2024
38 checks passed

ilongin deleted the ilongin/329-refactor-storages branch September 5, 2024 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `DataChain.listings()` method and use it in getting storages #331

Add `DataChain.listings()` method and use it in getting storages #331

ilongin commented Aug 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

shcheklein Sep 4, 2024

ilongin Sep 4, 2024

shcheklein Sep 4, 2024

ilongin Sep 5, 2024

shcheklein Sep 4, 2024

shcheklein Sep 4, 2024

ilongin Sep 4, 2024

shcheklein Sep 4, 2024

ilongin Sep 4, 2024

shcheklein left a comment

ilongin commented Sep 4, 2024

Add DataChain.listings() method and use it in getting storages #331

Add DataChain.listings() method and use it in getting storages #331

Conversation

ilongin commented Aug 20, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Aug 20, 2024 • edited Loading

Deploying datachain-documentation with Cloudflare Pages

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein left a comment

Choose a reason for hiding this comment

ilongin commented Sep 4, 2024

Add `DataChain.listings()` method and use it in getting storages #331

Add `DataChain.listings()` method and use it in getting storages #331

ilongin commented Aug 20, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading