Add LazyBlob helper class to lazy load file from filesystem #104

Aschen · 2023-03-17T20:46:30Z

Description

This PR add a new helper class LazyBlob which aim to prevent loading huge file into RAM.

This new class can be used instead of a Blob with the commit function.

const lazyBlob = await LazyBlob.create("./package.json");

await commit({
  repo,
  title: "Some commit",
  credentials: {
    accessToken: "<access token>",
  },
  operations: [
    {
      operation: "addOrUpdate",
      content: lazyBlob,
      path: "package.json",
    },
  ],
});

coyotte508

This is going to be really cool when ready!

My main request is that from the outside, it should behave exactly as a Blob. No need for .init() or dispose(), and instanceof Blob should work.

packages/hub/src/lib/LazyBlob.ts

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

Aschen · 2023-03-17T21:16:41Z

@coyotte508 I can get ride of init with the private constructor and static create method as you suggest but for the dispose the only option would be to open the file when reading and slice and then close it.

Here it's DX vs performances so I will let you choose 👍

coyotte508 · 2023-03-17T21:28:47Z

@coyotte508 I can get ride of init with the private constructor and static create method as you suggest but for the dispose the only option would be to open the file when reading and slice and then close it.

Here it's DX vs performances so I will let you choose 👍

The most calls will be three I think, on large files:

a .slice(0, 512) to get a sample to send to /preupload
a .stream() to compute the Sha
Then whatever fetch will call (probably .stream())

On smaller files, it will be two calls: one .slice(0, 512) and one .arrayBuffer().

Given that, I'd rather prioritize DX on this one ! The overhead should be low. Maybe an optimization would be to directly load small files in memory, if we really care about perf.

Aschen · 2023-03-17T21:31:05Z

Given that, I'd rather prioritize DX on this one ! The overhead should be low. Maybe an optimization would be to directly load small files in memory, if we really care about perf.

I agree with that 👍 Also, better have more syscall to open/close than unused file descriptor 🙈

coyotte508

Getting close!

packages/hub/src/utils/sha256.ts

packages/hub/src/lib/commit.ts

packages/hub/src/lib/LazyBlob.ts

packages/hub/test.js

coyotte508 · 2023-03-17T23:55:37Z

For the TS Error: maybe as ReturnType<Blob["stream"]>

coyotte508

I think it's ready after the changes.

I just can't believe this module doesn't already exist on npm (only versions I found loaded everything in memory)

coyotte508 · 2023-03-17T23:51:15Z

packages/hub/src/lib/LazyBlob.ts

+	private async init(): Promise<void> {
+		const { size } = await this.execute((file) => file.stat());
+
+		this.totalSize = size;
+	}


This can be removed

coyotte508 · 2023-03-17T23:51:29Z

packages/hub/src/lib/LazyBlob.ts

+	}
+
+	stream(): ReadableStream {
+		return Readable.toWeb(createReadStream(this.path));


Suggested change

return Readable.toWeb(createReadStream(this.path));

return Readable.toWeb(createReadStream(this.path, {start: this.start, end: this.end}));

I thought about this one on my way back from the beach today 😁

coyotte508 · 2023-03-18T00:05:16Z

packages/hub/src/lib/LazyBlob.ts

+	private path: string;
+	private start: number;
+	private end: number;
+	private totalSize: number;


This can be removed as well

coyotte508 · 2023-03-18T00:05:32Z

packages/hub/src/lib/LazyBlob.ts

+		if (this.start !== null) {
+			if (this.end !== null) {
+				return Math.abs(this.end - this.start);
+			}
+
+			return this.totalSize - this.start;
+		}
+
+		return this.totalSize;


Suggested change

if (this.start !== null) {

if (this.end !== null) {

return Math.abs(this.end - this.start);

}

return this.totalSize - this.start;

}

return this.totalSize;

return this.end - this.start;

coyotte508

Alirght, thanks a lot!

Aschen · 2023-03-20T13:17:04Z

@coyotte508 What about the documentation? If I understood well it's in the same repo, I can give it a try

coyotte508 · 2023-03-20T14:44:23Z

@coyotte508 What about the documentation? If I understood well it's in the same repo, I can give it a try

I intend to keep it internal.

A file with require("node:fs") is directly exposed in the exports, it will cause trouble for the use in browsers, at least for the CJS bundle. See how sha256-node is currently dynamically imported and only outside the browser.

So I think maybe a smart way to expose it is to allow passing a URL as an alternative to a Blob - see https://www.geeksforgeeks.org/node-js-url-pathtofileurl-api/ urls and files paths can be equivalent.

It also opens the door for supporting web urls later on. (using Range header, or HEAD calls appropriately to get a slice / the size)

Aschen · 2023-03-20T15:31:38Z

Yes I saw, I'm familiar with this kind of dual behavior when it's needed to provide a SDK for both node and the browser (See Kuzzle Javascript SDK)

If I understand well, you might want to allows a string representing a path to a local file for ContentSource if it's used in Node.js ?

await commit({
  repo,
  title: "Some commit",
  credentials: {
    accessToken: TEST_ACCESS_TOKEN,
  },
  operations: [
    {
      operation: "addOrUpdate",
      // 1) Only a path to a local file, if it's used from Node.js, a LazyBlob will be used
      path: "test.txt",
    },
    {
      operation: "addOrUpdate",
      // 2) URL to a local file, if it's used from Node.js, a LazyBlob will be used
      path: "file://test.txt",
    },
  ],
});

coyotte508 · 2023-03-20T15:47:03Z

If I understand well, you might want to allows a string representing a path to a local file for ContentSource if it's used in Node.js ?

When I was mentioning URL, it was literally :)

Basically, change:

type ContentSource = Blob;

To

type ContentSource = Blob | URL;

Btw path is the path inside the repo on HF, it's different

Aschen · 2023-03-20T16:05:01Z

Ok understood 👍

So if the URL is a local file (starting with file://) then the LazyBlob will be used, am I right?

coyotte508 · 2023-03-20T16:09:09Z

Ok understood +1

So if the URL is a local file (starting with file://) then the LazyBlob will be used, am I right?

Yea, if url.protocol === 'file:' use LazyBlob (except in browser of course), otherwise throw a TypeError. And later support http: and https:

Aschen · 2023-03-20T16:22:00Z

Do you want me to do this feature, supporting URL as content, in a separate PR?

coyotte508 · 2023-03-20T16:37:49Z

Do you want me to do this feature, supporting URL as content, in a separate PR?

Sure! Then maybe you can remove the export in this one and keep the LazyBlob private ? (I was going to do it later today before merging 😁 )

Aschen · 2023-03-20T17:06:26Z

Do you want me to do this feature, supporting URL as content, in a separate PR?

Sure! Then maybe you can remove the export in this one and keep the LazyBlob private ? (I was going to do it later today before merging grin )

No problem ;-)

… into feat/add-lazy-blob

Aschen added 2 commits March 17, 2023 21:41

Add LazyBlob helper class to lazy load file from filesystem

52278fb

dispose lazyblob in tests

debbb9d

Aschen mentioned this pull request Mar 17, 2023

Add a helper to generate Blob (lazily read) from a file path #68

Closed

fix lint

2d50d1e

coyotte508 reviewed Mar 17, 2023

View reviewed changes

Aschen and others added 2 commits March 17, 2023 22:09

Update packages/hub/src/lib/LazyBlob.ts

7e7945a

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

Update packages/hub/src/lib/LazyBlob.ts

0210f84

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

LazyBlob.create async method

13627ae

Blob compatible API

16c0fed

coyotte508 reviewed Mar 17, 2023

View reviewed changes

Aschen added 4 commits March 18, 2023 00:42

revers files

aa3316f

fix reviews and delete oopsi

2ce190c

Lazyblob.create fix

820f5d7

try to fix ts error

59f98e3

coyotte508 reviewed Mar 17, 2023

View reviewed changes

coyotte508 reviewed Mar 18, 2023

View reviewed changes

Aschen and others added 3 commits March 18, 2023 22:43

add tests, fix stream end bug and export class

48d4fec

nit

245bc9e

Merge branch 'main' into feat/add-lazy-blob

1d19062

coyotte508 approved these changes Mar 19, 2023

View reviewed changes

Aschen added 2 commits March 20, 2023 18:09

move LazyBlob in utils and dont export it

7a15ed2

Merge branch 'feat/add-lazy-blob' of github.com:Aschen/huggingface.js…

3951cd1

… into feat/add-lazy-blob

coyotte508 merged commit 7fac846 into huggingface:main Mar 20, 2023

Aschen deleted the feat/add-lazy-blob branch March 20, 2023 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LazyBlob helper class to lazy load file from filesystem #104

Add LazyBlob helper class to lazy load file from filesystem #104

Aschen commented Mar 17, 2023 •

edited

Loading

coyotte508 left a comment •

edited

Loading

Aschen commented Mar 17, 2023

coyotte508 commented Mar 17, 2023 •

edited

Loading

Aschen commented Mar 17, 2023

coyotte508 left a comment

coyotte508 commented Mar 17, 2023

coyotte508 left a comment

coyotte508 Mar 17, 2023

coyotte508 Mar 17, 2023

Aschen Mar 18, 2023

coyotte508 Mar 18, 2023

coyotte508 Mar 18, 2023

coyotte508 left a comment

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 •

edited

Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 •

edited

Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 •

edited

Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023

Aschen commented Mar 20, 2023

	return Readable.toWeb(createReadStream(this.path));
	return Readable.toWeb(createReadStream(this.path, {start: this.start, end: this.end}));

Add LazyBlob helper class to lazy load file from filesystem #104

Add LazyBlob helper class to lazy load file from filesystem #104

Conversation

Aschen commented Mar 17, 2023 • edited Loading

Description

coyotte508 left a comment • edited Loading

Choose a reason for hiding this comment

Aschen commented Mar 17, 2023

coyotte508 commented Mar 17, 2023 • edited Loading

Aschen commented Mar 17, 2023

coyotte508 left a comment

Choose a reason for hiding this comment

coyotte508 commented Mar 17, 2023

coyotte508 left a comment

Choose a reason for hiding this comment

coyotte508 Mar 17, 2023

Choose a reason for hiding this comment

coyotte508 Mar 17, 2023

Choose a reason for hiding this comment

Aschen Mar 18, 2023

Choose a reason for hiding this comment

coyotte508 Mar 18, 2023

Choose a reason for hiding this comment

coyotte508 Mar 18, 2023

Choose a reason for hiding this comment

coyotte508 left a comment

Choose a reason for hiding this comment

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 • edited Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 • edited Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023 • edited Loading

Aschen commented Mar 20, 2023

coyotte508 commented Mar 20, 2023

Aschen commented Mar 20, 2023

Aschen commented Mar 17, 2023 •

edited

Loading

coyotte508 left a comment •

edited

Loading

coyotte508 commented Mar 17, 2023 •

edited

Loading

coyotte508 commented Mar 20, 2023 •

edited

Loading

coyotte508 commented Mar 20, 2023 •

edited

Loading

coyotte508 commented Mar 20, 2023 •

edited

Loading