Prevent empty commits if files did not change #809

Pierrci · 2024-07-17T23:58:15Z

Cousin of huggingface/huggingface_hub#2389

Do I need to add tests somewhere? x)

(disclaimer: I obviously haven't tried it yet, but theoretically, it should work!)

Cousin of huggingface/huggingface_hub#2389

Pierrci · 2024-07-17T23:59:20Z

packages/hub/src/utils/sha1.ts

+
+	if (isFrontend) {
+		yield 1;
+		return null; // unsupported if we're here


I wanted to keep things simple, and it's not really a problem IMO, it just means that, in this case, we'll always upload non-LFS files

Pierrci · 2024-07-18T00:00:01Z

packages/hub/src/utils/sha1.ts

+export async function* sha1(buffer: Blob, opts?: { abortSignal?: AbortSignal }): AsyncGenerator<number, string | null> {
+	yield 0;
+
+	if (globalThis.crypto?.subtle) {


no threshold like for sha256 since non-LFS files are always < 10MB (which is the threshold for sha256 IIUC)

Pierrci · 2024-07-18T00:01:21Z

packages/hub/src/lib/commit.ts

+		const allOpsToCommit = [...deleteOps, ...fileOpsToCommit];
+		if (allOpsToCommit.length === 0) {
+			const res: Response = await (params.fetch ?? fetch)(
+				`${params?.hubUrl || HUB_URL}/api/${repoId.type}s/${repoId.name}`,


there doesn't seem to be any method already to call this endpoint, correct?

yes we should create one - in another PR - with expand handling too (getModelInfo, ...)

But here I think we should include the sha directly in the preupload response. Given we give already oid for individual files, and those are the oid in the context a specific commit from the remote, we should add the sha in the preupload response.

That way - no need for all this extra code here - cc @Wauplin as well

I think we should include the sha directly in the preupload response

That would simplifies things indeed!

see 325d917

packages/hub/src/lib/commit.ts

Wauplin

I had a quick look at the PR but haven't tried it myself either 😄

packages/hub/src/lib/commit.ts

packages/hub/src/utils/sha-node.ts

packages/hub/src/lib/commit.ts

coyotte508 · 2024-07-18T09:36:50Z

can we maybe add a preventEmptyCommit in the body of the commit endpoint (or query param, whatever) and let the backend throw an error in that case?

Pierrci · 2024-07-18T13:31:59Z

@coyotte508 that would work too, yes, but it would have been nice to hear from you when we discussed that internally haha; now we already have huggingface/huggingface_hub#2389 and https://github.com/huggingface-internal/moon-landing/pull/10547, but if we do that we won't need those anymore, correct? :)

coyotte508 · 2024-07-18T13:42:37Z

@coyotte508 that would work too, yes, but it would have been nice to hear from you when we discussed that internally haha; now we already have huggingface/huggingface_hub#2389 and huggingface-internal/moon-landing#10547, but if we do that we won't need those anymore, correct? :)

indeed 😬

I guess the backend solution would have been to add a gitaly hook conditionally skipped to prevent empty commits

coyotte508 · 2024-07-18T13:44:47Z

Do I need to add tests somewhere? x)

You can add a test to commit.spec.ts

Wauplin · 2024-07-18T14:38:30Z

can we maybe add a preventEmptyCommit in the body of the commit endpoint (or query param, whatever) and let the backend throw an error in that case?

I actually prefer to enforce the preventEmptyCommit, at least in the Python client. The problem if we add a new parameter prevent_empty_commit: bool when creating a commit is that it'll never be used/propagated to downstream libraries. If we set a default value True, then it's similar to not having a parameter. If we set a default value False, then it's similar to not doing anything at all.

A bit the same for raising errors. That would break some workflows that haven't asked for anything. Silently preventing empty commits (with a log message) is the best compromise I think. TL;DR: no one cares expect us, right?

that may or may not work, in any case it's timing out for sure

Pierrci · 2024-07-19T21:33:59Z

@coyotte508 I added a test, but it seems it's timing out, along with others, any tip regarding this? (does that mean I broke them? :))

coyotte508 · 2024-07-20T06:52:23Z

well yes you probably broke them :)

(they ran fine in https://github.com/huggingface/huggingface.js/actions/runs/9991710454/job/27615066628)

Pierrci · 2024-07-23T23:16:32Z

it's greeeeeeeen

coyotte508 · 2024-07-24T08:35:12Z

packages/hub/src/utils/sha1.ts

Tested nothing is broken by renaming this file - https://huggingface.co/spaces/coyotte508/hub-sha256 - and it seems fine

coyotte508

Looks nice!

packages/hub/src/types/api/api-commit.ts

coyotte508 · 2024-07-24T08:46:20Z

packages/hub/src/lib/commit.ts

+		const allOpsToCommit = [...deleteOps, ...fileOpsToCommit];
+		if (allOpsToCommit.length === 0) {
+			const res: Response = await (params.fetch ?? fetch)(
+				`${params?.hubUrl || HUB_URL}/api/${repoId.type}s/${repoId.name}`,


yes we should create one - in another PR - with expand handling too (getModelInfo, ...)

But here I think we should include the sha directly in the preupload response. Given we give already oid for individual files, and those are the oid in the context a specific commit from the remote, we should add the sha in the preupload response.

That way - no need for all this extra code here - cc @Wauplin as well

Co-authored-by: Eliott C. <[email protected]>

Pierrci · 2024-07-25T21:25:16Z

packages/hub/src/lib/commit.ts

@@ -459,6 +467,38 @@ export async function* commitIter(params: CommitParams): AsyncGenerator<CommitPr

 		yield { event: "phase", phase: "committing" };

+		const allOpsToCommit = [...deleteOps, ...fileOpsToCommit];
+		if (allOpsToCommit.length === 0) {
+			if (!currentCommitOid) {


IIUC, we currently don't return early or throw an error in the case where operations is empty, meaning it's possible to get there without calling the preupload endpoint before, and thus currentCommitOid can still be undefined here.

So I kept this for now; the other alternative would be to return an object with a different different shape or throw an error if there are no operations, but it would be breaking change - I'll let you decide @coyotte508

It's fine to throw on empty commits IMO, we can do a major version change (and maybe do some of https://github.com/huggingface/huggingface.js/milestone/2 before publishing)

actually if we upgrade to 1.0 we can add the null return type and return null on empty commit

(And it's better than throwing an error, at least it will be caught by TS)

Prevent empty commits if files did not change

8664ce9

Cousin of huggingface/huggingface_hub#2389

Pierrci requested a review from coyotte508 as a code owner July 17, 2024 23:58

forgot to rename this

6038ed5

Pierrci commented Jul 18, 2024

View reviewed changes

Wauplin reviewed Jul 18, 2024

View reviewed changes

packages/hub/src/lib/commit.ts Outdated Show resolved Hide resolved

packages/hub/src/utils/sha-node.ts Show resolved Hide resolved

packages/hub/src/lib/commit.ts Outdated Show resolved Hide resolved

Pierrci added 3 commits July 19, 2024 17:09

Correct sha1

01c49b9

add test

1bb1110

that may or may not work, in any case it's timing out for sure

a bit better

57ffa9d

Pierrci added 7 commits July 23, 2024 18:23

I was a bit too radical it seems

cd05975

disappointing

abfb8ee

oopsie

be22d1a

the order changed a bit

234fef2

like that rather

491ff3a

better

a7aafe0

🥳

38030e7

Merge remote-tracking branch 'origin/main' into prevent-empty-commits

a932772

coyotte508 reviewed Jul 24, 2024

View reviewed changes

Pierrci and others added 3 commits July 24, 2024 16:18

Update packages/hub/src/types/api/api-commit.ts

6d07fec

Co-authored-by: Eliott C. <[email protected]>

get commit oid from preupload

325d917

lint/format

9b8f786

Pierrci commented Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent empty commits if files did not change #809

Prevent empty commits if files did not change #809

Pierrci commented Jul 17, 2024

Pierrci Jul 17, 2024

Pierrci Jul 18, 2024

Pierrci Jul 18, 2024

coyotte508 Jul 24, 2024 •

edited

Loading

Wauplin Jul 24, 2024

Pierrci Jul 25, 2024

Wauplin left a comment

coyotte508 commented Jul 18, 2024 •

edited

Loading

Pierrci commented Jul 18, 2024

coyotte508 commented Jul 18, 2024 •

edited

Loading

coyotte508 commented Jul 18, 2024

Wauplin commented Jul 18, 2024 •

edited

Loading

Pierrci commented Jul 19, 2024

coyotte508 commented Jul 20, 2024 •

edited

Loading

Pierrci commented Jul 23, 2024

coyotte508 Jul 24, 2024

coyotte508 left a comment

coyotte508 Jul 24, 2024 •

edited

Loading

Pierrci Jul 25, 2024 •

edited

Loading

coyotte508 Jul 26, 2024 •

edited

Loading

coyotte508 Jul 26, 2024

Prevent empty commits if files did not change #809

Are you sure you want to change the base?

Prevent empty commits if files did not change #809

Conversation

Pierrci commented Jul 17, 2024

Pierrci Jul 17, 2024

Choose a reason for hiding this comment

Pierrci Jul 18, 2024

Choose a reason for hiding this comment

Pierrci Jul 18, 2024

Choose a reason for hiding this comment

coyotte508 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Wauplin Jul 24, 2024

Choose a reason for hiding this comment

Pierrci Jul 25, 2024

Choose a reason for hiding this comment

Wauplin left a comment

Choose a reason for hiding this comment

coyotte508 commented Jul 18, 2024 • edited Loading

Pierrci commented Jul 18, 2024

coyotte508 commented Jul 18, 2024 • edited Loading

coyotte508 commented Jul 18, 2024

Wauplin commented Jul 18, 2024 • edited Loading

Pierrci commented Jul 19, 2024

coyotte508 commented Jul 20, 2024 • edited Loading

Pierrci commented Jul 23, 2024

coyotte508 Jul 24, 2024

Choose a reason for hiding this comment

coyotte508 left a comment

Choose a reason for hiding this comment

coyotte508 Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Pierrci Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

coyotte508 Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

coyotte508 Jul 26, 2024

Choose a reason for hiding this comment

coyotte508 Jul 24, 2024 •

edited

Loading

coyotte508 commented Jul 18, 2024 •

edited

Loading

coyotte508 commented Jul 18, 2024 •

edited

Loading

Wauplin commented Jul 18, 2024 •

edited

Loading

coyotte508 commented Jul 20, 2024 •

edited

Loading

coyotte508 Jul 24, 2024 •

edited

Loading

Pierrci Jul 25, 2024 •

edited

Loading

coyotte508 Jul 26, 2024 •

edited

Loading