Added `DataChain.diff()` #718

ilongin · 2024-12-17T01:07:01Z

This is wrapper method around more generic DataChain.compare() which is used to calculate diff between file based chains (those which have File objects in it).
For matching same rows file signals source and path are used.
For comparing if row is modified / unchanged, file signals version and etag are used.

codecov · 2024-12-17T01:17:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.43%. Comparing base (983cbd8) to head (d1c5109).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
+ Coverage   87.39%   87.43%   +0.03%     
==========================================
  Files         114      114              
  Lines       10951    10967      +16     
  Branches     1508     1508              
==========================================
+ Hits         9571     9589      +18     
+ Misses        999      998       -1     
+ Partials      381      380       -1

Flag	Coverage Δ
datachain	`87.37% <100.00%> (+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.

dreadatour · 2024-12-17T12:57:36Z

src/datachain/lib/dc.py

+            ```py
+            diff = images.diff(
+                new_images,
+                on=["file"],


Should we update example here or update typings above?

on: str = "file",

Nice catch, fixed!

cloudflare-workers-and-pages · 2024-12-18T01:51:37Z

Deploying datachain-documentation with Cloudflare Pages

Latest commit:	`d1c5109`
Status:	✅ Deploy successful!
Preview URL:	https://97418e03.datachain-documentation.pages.dev
Branch Preview URL:	https://ilongin-716-file-diff.datachain-documentation.pages.dev

View logs

dmpetrov

Amazing ✨
Please see comments inline. I'm approving to not to block it.

dmpetrov · 2024-12-18T18:37:41Z

tests/unit/lib/test_datachain.py

@@ -3350,3 +3350,108 @@ def test_compare_right_compare_wrong_length(test_session):
    assert str(exc_info.value) == (


The test file is massive 😱

Could you please extract compare and diff test to a separate file like test_diff.py. See test_merge.py as an example.

dmpetrov · 2024-12-19T01:50:07Z

src/datachain/lib/dc.py

+        added: bool = True,
+        deleted: bool = True,
+        modified: bool = True,
+        unchanged: bool = False,


I think diff() should have only added=True and modified=True. the rest should be False.

We need to optimize it for delta update use case which process only new and changed files.

dmpetrov · 2024-12-19T01:51:18Z

src/datachain/lib/dc.py

+        unchanged: bool = False,
+        status_col: Optional[str] = None,
+    ) -> "DataChain":
+        """Similar as .compare() but for file based chains, i.e. those that have


Please mention the similarity to compare() in the end of the description. We cannot assum that user knows compare() alread.

Added better docs

dmpetrov · 2024-12-19T01:52:53Z

src/datachain/lib/dc.py

+                deleted=True,
+                modified=True,
+                unchanged=True,
+                status_col="diff"


It's empty by default. The same in the description.

dmpetrov · 2024-12-19T02:10:19Z

tests/unit/lib/test_datachain.py

+@pytest.mark.parametrize("deleted", (True, False))
+@pytest.mark.parametrize("modified", (True, False))
+@pytest.mark.parametrize("unchanged", (True, False))
+@pytest.mark.parametrize("status_col", ("diff", None))


The test looks overcomplicated 🙂

Is it possible to separate this to small tests instead of large parametrized one?

Also, I don't see value in all modified, deleted, unchanged statuses since compare() tests have to cover this, not diff(). The same for status_col. 2-3 simple tests should cover diff() pretty well.

Simplified it

dmpetrov · 2024-12-19T02:11:21Z

tests/unit/lib/test_datachain.py

+@pytest.mark.parametrize("deleted", (True, False))
+@pytest.mark.parametrize("modified", (True, False))
+@pytest.mark.parametrize("unchanged", (True, False))
+@pytest.mark.parametrize("status_col", ("diff", None))


Same here - to complicated to unit test and it test mostly compare(), not diff().
I'd appreciate it if you could simplify it.

dmpetrov · 2024-12-19T02:12:37Z

tests/unit/lib/test_datachain.py

+    if deleted:
+        expected.append(("D", fs3, 3))
+    if unchanged:
+        expected.append(("U", fs4, 4))


🤔 I was sure that we decided to get rid of U and use S (Same) instead, didn't we?

Yes, I will change it in my followup PR. I forgot to change it in that main one

Changed to S .. also changed flag name from unchanged to same

…n into ilongin/716-file-diff

addef file diff method

44f509a

ilongin temporarily deployed to internal December 17, 2024 01:07 — with GitHub Actions Inactive

ilongin linked an issue Dec 17, 2024 that may be closed by this pull request

Create DataChain.diff() method #716

Closed

ilongin requested review from dmpetrov, dreadatour, mattseddon, amritghimire and skshetry December 17, 2024 01:07

dreadatour reviewed Dec 17, 2024

View reviewed changes

fixing comment

3fa375a

ilongin temporarily deployed to internal December 18, 2024 01:51 — with GitHub Actions Inactive

ilongin requested a review from dreadatour December 18, 2024 01:51

dmpetrov approved these changes Dec 19, 2024

View reviewed changes

ilongin added 2 commits December 20, 2024 09:35

Merge branch 'main' into ilongin/716-file-diff

8aef019

Merge branch 'ilongin/716-file-diff' of github.com:iterative/datachai…

05c4e28

…n into ilongin/716-file-diff

ilongin temporarily deployed to internal December 20, 2024 08:35 — with GitHub Actions Inactive

removed compare and diff tests from test_datachain

c884ba3

ilongin temporarily deployed to internal December 20, 2024 08:49 — with GitHub Actions Inactive

changed default values for statuses

2da717a

ilongin temporarily deployed to internal December 20, 2024 08:53 — with GitHub Actions Inactive

added better description of diff method

0d0356e

ilongin temporarily deployed to internal December 20, 2024 10:27 — with GitHub Actions Inactive

update docs

b6eb753

ilongin temporarily deployed to internal December 20, 2024 10:45 — with GitHub Actions Inactive

removed not needed test cases

c2aec82

ilongin temporarily deployed to internal December 20, 2024 11:09 — with GitHub Actions Inactive

changed uncahnged to same

8fa9465

ilongin temporarily deployed to internal December 20, 2024 11:16 — with GitHub Actions Inactive

fixing default values

d1c5109

ilongin temporarily deployed to internal December 20, 2024 14:30 — with GitHub Actions Inactive

ilongin merged commit 20c73b2 into main Dec 20, 2024
34 checks passed

ilongin deleted the ilongin/716-file-diff branch December 20, 2024 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `DataChain.diff()` #718

Added `DataChain.diff()` #718

ilongin commented Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

dreadatour Dec 17, 2024

ilongin Dec 18, 2024

cloudflare-workers-and-pages bot commented Dec 18, 2024 •

edited

Loading

dmpetrov left a comment

dmpetrov Dec 18, 2024

ilongin Dec 20, 2024

dmpetrov Dec 19, 2024

dmpetrov Dec 19, 2024

ilongin Dec 20, 2024

dmpetrov Dec 19, 2024

dmpetrov Dec 19, 2024

ilongin Dec 20, 2024

dmpetrov Dec 19, 2024

dmpetrov Dec 19, 2024

ilongin Dec 19, 2024

ilongin Dec 20, 2024

		@@ -3350,3 +3350,108 @@ def test_compare_right_compare_wrong_length(test_session):
		assert str(exc_info.value) == (

Added DataChain.diff() #718

Added DataChain.diff() #718

Conversation

ilongin commented Dec 17, 2024

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Dec 18, 2024 • edited Loading

Deploying datachain-documentation with Cloudflare Pages

dmpetrov left a 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

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

Added `DataChain.diff()` #718

Added `DataChain.diff()` #718

codecov bot commented Dec 17, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 18, 2024 •

edited

Loading