Skip to content
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.

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

fix package export #1161

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix package export #1161

wants to merge 2 commits into from

Conversation

fs-eire
Copy link
Contributor

@fs-eire fs-eire commented Jan 20, 2025

Summary

This PR includes a few changes that helps to fix the package export. Now transformers.js has the following exports:

usage file description
CDN
import {...} from "https://cdn.jsdelivr.net/npm/@huggingface/transformers@{version}"
dist/transformers[.min].js This is a JavaScript file that bundled with all its JS dependencies.
Node.js/Deno (ESM/CJS)
import {...} from "@huggingface/transformers"
const {...} = require("@huggingface/transformers")
dist/transformers.node[.min].mjs
dist/transformers.node[.min].cjs
Treat onnxruntime-common and onnxruntime-node as external; Ignore onnxruntime-web
Web (ESM)
import {...} from "@huggingface/transformers"
dist/transformers.web[.min].js Treat onnxruntime-common and onnxruntime-web as external; Ignore onnxruntime-node

Problem

There are complains about different kinds of failures when users try to import transformers.js and/or onnxruntime-web in their web applications.

It took me quite a while to fix most of the problems in onnxruntime-web. The latest dev version of onnxruntime-web should have worked for all known major use cases.

When things come to transformers.js, the situation become more complicated. After some investigation, I found that some import/export problems that resolved in onnxruntime-web occurred in transformers.js again. This is because when building transformers.js, webpack generated some code that will not work for using as input for bundler again.

Solution

There are 2 major changes in this PR:

  • remove the override of env.wasm.wasmPaths
  • make onnxruntime-web as external dependency of transformers.js

Why treat onnxruntime-web as external dependency

ONNX Runtime Web has a few workarounds for webpack to ensure out-of-the-box user experience. Those workarounds requires the code as-is when webpack starts to process it. The workarounds cannot keep in the bundled JS file.

To make sure of backward compatibility, this change still keeps the file dist/transformers[.min].js unchanged. Instead, a new file dist/transformers.web[.min].js is added and replaced in the "exports" property of the package.json.

Tests

The following tests are passed when applying this PR:

** [1] - For Next.js based app, a workaround is needed if any code statically imports @huggingface/transformers. This is because Next.js tries to bundle the code for server even if those code starts with "use client";. This happens to both dev mode and prod mode, and to both default and turbopack bundler.
** [2] - For Vite 5.x based app, a workaround is needed according to vitejs/vite#8427. This is a known issue affecting all Vite app depending on WebAssembly based packages, and may be fixed in Vite 6.x

src/backends/onnx.js Outdated Show resolved Hide resolved
@@ -42,7 +42,7 @@ function buildConfig({
type,
},
assetModuleFilename: "[name][ext]",
chunkFormat: "module",
chunkFormat: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why the final bundle includes new URL("./", import.meta.url).

Setting it to false to disable the chunks feature of webpack (which is not used anyway)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good find! Thanks!

webpack.config.js Outdated Show resolved Hide resolved
src/backends/onnx.js Outdated Show resolved Hide resolved
@fs-eire
Copy link
Contributor Author

fs-eire commented Jan 24, 2025

@xenova what do you think of the proposed changes?

@fs-eire fs-eire changed the title [DO NOT MERGE] fix package export fix package export Jan 29, 2025
@fs-eire fs-eire marked this pull request as ready for review January 29, 2025 21:44
@fs-eire
Copy link
Contributor Author

fs-eire commented Jan 29, 2025

@xenova what do you think of the proposed changes?

PR is ready for review.

@xenova
Copy link
Collaborator

xenova commented Jan 30, 2025

Thanks @fs-eire! I will review this fully soon!

Comment on lines -190 to -194

// https://developer.mozilla.org/en-US/docs/Web/API/crossOriginIsolated
if (typeof crossOriginIsolated === 'undefined' || !crossOriginIsolated) {
ONNX_ENV.wasm.numThreads = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was a bug that in a non-cross origin isolated environment, onnxruntime-web will use multiple threads and then fail the initialization. This code snippet was a workaround for that bug.

Now in latest onnxruntime-web, this check is included so no necessary to do it again in transformerjs

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nice. I’ve run into this error so was curious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to know! Thanks @fs-eire!

Copy link
Collaborator

@xenova xenova left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I'm doing some more testing now.

Comment on lines 179 to 191
// Currently, this behavior is disabled. To enable it, uncomment the following code.

// // Override the wasm search path if:
// // 1. We are not in a service worker.
// // 2. ONNX Runtime Web version is defined.
// // 3. The wasmPaths are not already set.
// //
// // @ts-ignore Cannot find name 'ServiceWorkerGlobalScope'.ts(2304)
// if (!(typeof ServiceWorkerGlobalScope !== 'undefined' && self instanceof ServiceWorkerGlobalScope)
// && typeof ONNX?.versions?.web === 'string'
// && !ONNX_ENV.wasm.wasmPaths) {
// ONNX_ENV.wasm.wasmPaths = `https://cdn.jsdelivr.net/npm/onnxruntime-web@${ONNX.versions.web}/dist/`;
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

We've run into several issues in the past where the wasm files weren't able to be loaded locally, so we default to the CDN. Maybe in future we can switch to this, but I think we can keep it for now.

Another reason is that the raw .wasm file is ~21MB, but the CDN (gzipped) version is ~4MB. It might be a good idea to find a way to compress the raw file too. cc @fs-eire @guschmue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I will make a change to keep them.

We've run into several issues in the past where the wasm files weren't able to be loaded locally

This is indeed the case back to a few versions ago. I made it much better now but I still cannot say it's working for 100% cases.

the CDN (gzipped) version is ~4MB.

I think it is already the default setting for all modern server to enable gzip. A dev server usually does not enable this feature. However I didn't check the file size.

Comment on lines -190 to -194

// https://developer.mozilla.org/en-US/docs/Web/API/crossOriginIsolated
if (typeof crossOriginIsolated === 'undefined' || !crossOriginIsolated) {
ONNX_ENV.wasm.numThreads = 1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to know! Thanks @fs-eire!

@@ -42,7 +42,7 @@ function buildConfig({
type,
},
assetModuleFilename: "[name][ext]",
chunkFormat: "module",
chunkFormat: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good find! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants