-
Notifications
You must be signed in to change notification settings - Fork 518
fix(ai): resolve the ONNX wasm directory against the app, not the route #2864
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
Open
wayfarer3130
wants to merge
8
commits into
main
Choose a base branch
from
fix/onnx-wasm-path-app-root
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
e08ad12
fix(ai): resolve the ONNX wasm directory against the app, not the route
wayfarer3130 4f8f44e
fix(ai): anchor the wasm prefix to the same base as the emitted assets
wayfarer3130 ae387f6
Merge branch 'main' into fix/onnx-wasm-path-app-root
wayfarer3130 57c5163
fix(ai): resolve the ONNX wasm prefix against PUBLIC_URL, not the route
wayfarer3130 b60e59e
fix(ai): locate the ONNX wasm from the codec wasm path, then PUBLIC_URL
wayfarer3130 277308d
refactor(core): resolve wasm locations in core, not in the ai package
wayfarer3130 2e357a3
refactor(ai): drop the ort wasm directory override
wayfarer3130 24e958f
Merge remote-tracking branch 'origin/main' into fix/onnx-wasm-path-ap…
wayfarer3130 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| module.exports = require('../../babel.config.js'); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| /* eslint-disable */ | ||
| const base = require('../../jest.config.base.js'); | ||
| const path = require('path'); | ||
|
|
||
| module.exports = { | ||
| ...base, | ||
| displayName: 'ai', | ||
| testMatch: [ | ||
| ...base.testMatch, | ||
| '<rootDir>/src/**/*.test.ts', | ||
| '<rootDir>/src/**/*.spec.ts', | ||
| ], | ||
| moduleNameMapper: { | ||
| ...base.moduleNameMapper, | ||
| '^@cornerstonejs/(\\w+)/(.+)$': path.resolve(__dirname, '../$1/src/$2'), | ||
| '^@cornerstonejs/(.*)$': path.resolve(__dirname, '../$1/src'), | ||
| }, | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { utilities } from '@cornerstonejs/core'; | ||
| import getOrtWasmPaths from './getOrtWasmPaths'; | ||
|
|
||
| /** | ||
| * How a base is resolved is `resolveWasmBasePath` in `@cornerstonejs/core`, | ||
| * covered by `packages/core/test/wasmBasePath.jest.js`. What is left here is the | ||
| * one thing this module decides - the standard directory - and that the two ways | ||
| * an application declares a location both reach the ONNX Runtime. | ||
| */ | ||
| describe('getOrtWasmPaths', () => { | ||
| const publicUrl = process.env.PUBLIC_URL; | ||
|
|
||
| beforeEach(() => { | ||
| delete (globalThis as { PUBLIC_URL?: string }).PUBLIC_URL; | ||
| delete process.env.PUBLIC_URL; | ||
| utilities.setWasmBasePath(undefined); | ||
| window.history.replaceState(null, '', '/'); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| if (publicUrl === undefined) { | ||
| delete process.env.PUBLIC_URL; | ||
| } else { | ||
| process.env.PUBLIC_URL = publicUrl; | ||
| } | ||
| }); | ||
|
|
||
| it('looks in ort/ under the application base', () => { | ||
| process.env.PUBLIC_URL = '/pacs/'; | ||
| // The bug this guards: `'ort/'` used to resolve against the route, so a | ||
| // viewer on /pacs/viewer/dicomweb fetched /pacs/viewer/ort/ and got | ||
| // index.html back. | ||
| window.history.replaceState(null, '', '/pacs/viewer/dicomweb'); | ||
|
|
||
| expect(getOrtWasmPaths()).toBe('http://localhost/pacs/ort/'); | ||
| }); | ||
|
|
||
| it('loads from the configured wasm directory, the way the codecs do', () => { | ||
| utilities.setWasmBasePath('/assets/cs-wasm/'); | ||
|
|
||
| expect(getOrtWasmPaths()).toBe('http://localhost/assets/cs-wasm/'); | ||
| }); | ||
|
|
||
| it('serves the binaries from a CDN the configured directory names', () => { | ||
| utilities.setWasmBasePath('https://cdn.example.com/wasm/'); | ||
|
|
||
| expect(getOrtWasmPaths()).toBe('https://cdn.example.com/wasm/'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /** | ||
| * ONNX Runtime fetches its WebAssembly binaries from the prefix held in | ||
| * `ort.env.wasm.wasmPaths` and feeds whatever comes back straight to | ||
| * `WebAssembly.instantiateStreaming`. | ||
| * | ||
| * Every other wasm binary in this repository is located with | ||
| * `new URL(<specifier>, import.meta.url)` — the bundler resolves the file, | ||
| * emits it, and hands back an absolute URL that does not depend on the page the | ||
| * user is currently on. `onnxruntime-web@1.17` cannot be addressed that way: | ||
| * its `exports` map publishes only the JavaScript entry points, so | ||
| * `new URL('onnxruntime-web/dist/ort-wasm-simd.jsep.wasm', import.meta.url)` | ||
| * fails to resolve. Applications copy `onnxruntime-web/dist` somewhere they | ||
| * serve instead — the example runner copies it to `<example>/ort` | ||
| * (`utils/ExampleRunner/template-config.js`) — and point the runtime there. | ||
| * | ||
| * That makes it the same problem the codec binaries have, and it gets the same | ||
| * answer: `utilities.resolveWasmBasePath` in `@cornerstonejs/core`, which | ||
| * prefers the wasm directory the application declared and otherwise resolves | ||
| * the standard directory against the application's base. All this module adds | ||
| * is the standard directory name. | ||
| * | ||
| * Those two are the whole story, and there is deliberately no third way to name | ||
| * the location here. Since the binaries cannot be reached from this module, | ||
| * there is nothing to fall back to: an application serving them from somewhere | ||
| * else says so with `init({ wasmBasePath })`, or declares where it is mounted | ||
| * with `PUBLIC_URL`, or assigns `ort.env.wasm.wasmPaths` itself — which the | ||
| * controller leaves alone. | ||
| * | ||
| * When `onnxruntime-web` is eventually bumped to >= 1.21 its | ||
| * `*.bundle.min.mjs` builds resolve their own `.wasm` through `import.meta.url` | ||
| * and this module can be deleted. | ||
| */ | ||
| import { utilities } from '@cornerstonejs/core'; | ||
|
|
||
| /** Directory applications copy `onnxruntime-web/dist` into. */ | ||
| export const DEFAULT_ORT_WASM_DIRECTORY = 'ort/'; | ||
|
|
||
| /** | ||
| * Absolute URL prefix for the ONNX Runtime wasm binaries: the wasm directory the | ||
| * application configured, or `ort/` under the application's base. | ||
| * | ||
| * @returns the prefix as an absolute URL, or the directory unchanged when there | ||
| * is nothing to resolve it against (a non-browser context). | ||
| */ | ||
| export default function getOrtWasmPaths(): string { | ||
| return utilities.resolveWasmBasePath(DEFAULT_ORT_WASM_DIRECTORY); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /** | ||
| * Resolves a path against the base the *application* is served from, rather | ||
| * than against the current document. | ||
| * | ||
| * A document-relative path resolves against the current route, so it only finds | ||
| * what it is looking for while the page happens to sit at the right depth. An | ||
| * application served from `/viewer/dicomweb` that asks for `assets/x` requests | ||
| * `/viewer/assets/x` and receives whatever the SPA fallback answers with, which | ||
| * for a binary asset means a corrupt file rather than an error. Anything an | ||
| * application stores beside its bundle and fetches by name — wasm binaries | ||
| * above all, since `WebAssembly.instantiateStreaming` reports the resulting | ||
| * `index.html` as a magic-word failure — has to be resolved against the | ||
| * application instead. | ||
| * | ||
| * `PUBLIC_URL` is where that base comes from: the value an application already | ||
| * injects to say where it is mounted, defaulting to the server root. | ||
| */ | ||
|
|
||
| /** | ||
| * Declared because `process` does not exist in a browser, and the bundlers that | ||
| * substitute `process.env.PUBLIC_URL` do it by matching that exact expression — | ||
| * so it has to be spelled out rather than reached through `globalThis`. | ||
| */ | ||
| declare const process: { env: Record<string, string | undefined> }; | ||
|
|
||
| /** Base assumed when nothing declares one: applications sit at the root. */ | ||
| export const DEFAULT_PUBLIC_URL = '/'; | ||
|
|
||
| /** | ||
| * `PUBLIC_URL` as a build-time substitution (Create React App and friends). | ||
| * | ||
| * A `typeof process !== 'undefined'` guard would be the obvious way to write | ||
| * this and does not work: bundlers rewrite `process.env.PUBLIC_URL` but leave | ||
| * the `typeof` check alone, and it is false in every browser bundle, so the | ||
| * substituted value would never be read. Catching the reference error is what | ||
| * is left. | ||
| */ | ||
| function getBuildTimePublicUrl(): string | undefined { | ||
| try { | ||
| return process.env.PUBLIC_URL || undefined; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The base the application declares for itself, defaulting to the server root. | ||
| * | ||
| * `PUBLIC_URL` on the global is the runtime spelling | ||
| * `utils/demo/helpers/initDemo.ts` sets and `dicom-microscopy-viewer` reads; | ||
| * `config.path` is the same value carried in a viewer's configuration object. | ||
| */ | ||
| export function getPublicUrl(): string { | ||
| const globals = globalThis as { | ||
| PUBLIC_URL?: string; | ||
| config?: { path?: string }; | ||
| }; | ||
|
|
||
| return ( | ||
| globals.PUBLIC_URL || | ||
| globals.config?.path || | ||
| getBuildTimePublicUrl() || | ||
| DEFAULT_PUBLIC_URL | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * The page's origin, and nothing else. `PUBLIC_URL` is usually a path | ||
| * (`/pacs/`) and needs something absolute to resolve against, but the route is | ||
| * exactly what must stay out of the result — so hand over the protocol and host | ||
| * alone rather than `location.href` or `document.baseURI`. | ||
| */ | ||
| function getOrigin(): string | undefined { | ||
| const { location } = globalThis; | ||
|
|
||
| return location ? `${location.protocol}//${location.host}` : undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Resolves a path against the application's base. | ||
| * | ||
| * @param path - a relative path is relative to the application, an absolute | ||
| * path is relative to the server root, and a full URL (a CDN) is used as | ||
| * given. An empty path yields the application's base itself. | ||
| * @returns an absolute URL, or `path` unchanged when there is nothing to | ||
| * resolve it against (a non-browser context). | ||
| */ | ||
| export default function resolveApplicationUrl(path = ''): string { | ||
| const publicUrl = getPublicUrl(); | ||
| // `PUBLIC_URL=/pacs` is a common spelling, and URL resolution would treat | ||
| // that last segment as a file name and discard it. | ||
| const base = publicUrl.endsWith('/') ? publicUrl : `${publicUrl}/`; | ||
| const origin = getOrigin(); | ||
|
|
||
| try { | ||
| return new URL(path, origin ? new URL(base, origin) : new URL(base)).href; | ||
| } catch { | ||
| return path; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * Where Cornerstone loads its WebAssembly binaries from. | ||
| * | ||
| * A wasm binary cannot be located the way a bundled asset is. The usual | ||
| * `new URL(<specifier>, import.meta.url)` only works when the bundler resolves, | ||
| * emits and hashes the file, and several of the binaries Cornerstone needs | ||
| * cannot be reached that way: the decoders name their codecs with bare | ||
| * `@cornerstonejs/codec-...` specifiers, which bundlers do not rewrite inside | ||
| * `new URL(...)`, and `onnxruntime-web@1.17` publishes only JavaScript entry | ||
| * points in its `exports` map. An application therefore copies those binaries | ||
| * somewhere it serves and *declares* where they are. | ||
| * | ||
| * Declaring it once is the point of `wasmBasePath`: one directory holding every | ||
| * binary, set through `init({ wasmBasePath })` on | ||
| * `@cornerstonejs/dicom-image-loader` (see `LoaderOptions.wasmBasePath`) and | ||
| * recorded here rather than privately in the loader, so that every package | ||
| * locating a binary honours the same directory. | ||
| * | ||
| * When nothing declares one, each set of binaries falls back to the standard | ||
| * directory its owner copies them into, resolved against the application rather | ||
| * than against the current document — see `resolveApplicationUrl` for why that | ||
| * distinction is the whole point. | ||
| */ | ||
| import resolveApplicationUrl from './resolveApplicationUrl'; | ||
|
|
||
| /** Directory every wasm binary is loaded from, or undefined for the default. */ | ||
| let wasmBasePath: string | undefined; | ||
|
|
||
| /** | ||
| * Sets the directory every wasm binary is loaded from. Pass undefined (or an | ||
| * empty string) to restore each consumer's own default resolution. | ||
| * | ||
| * `init({ wasmBasePath })` on `@cornerstonejs/dicom-image-loader` calls this, | ||
| * so applications configuring the loader do not need to call it themselves. | ||
| */ | ||
| export function setWasmBasePath(basePath?: string): void { | ||
| wasmBasePath = basePath || undefined; | ||
| } | ||
|
|
||
| /** The configured wasm directory, or undefined when nothing has set one. */ | ||
| export function getWasmBasePath(): string | undefined { | ||
| return wasmBasePath; | ||
| } | ||
|
|
||
| /** | ||
| * Absolute URL of the directory a set of wasm binaries loads from. | ||
| * | ||
| * @param defaultDirectory - directory to use when nothing has declared one: the | ||
| * standard location its owner copies the binaries into, e.g. `ort/` for the | ||
| * ONNX Runtime. Both it and a configured `wasmBasePath` are resolved the same | ||
| * way — relative to the application, or to the server root when absolute, or | ||
| * used as given when a full URL. | ||
| * @returns the directory as an absolute URL with a trailing slash, or the | ||
| * unresolved directory when there is nothing to resolve it against (a | ||
| * non-browser context). | ||
| */ | ||
| export function resolveWasmBasePath(defaultDirectory = ''): string { | ||
| const directory = wasmBasePath || defaultDirectory; | ||
|
|
||
| // A trailing slash, so the value resolves as a directory rather than having | ||
| // its last segment discarded as a file name. An empty directory stays empty: | ||
| // it resolves to the application's base itself, not to the server root. | ||
| return resolveApplicationUrl( | ||
| !directory || directory.endsWith('/') ? directory : `${directory}/` | ||
| ); | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: cornerstonejs/cornerstone3D
Length of output: 5863
🏁 Script executed:
Repository: cornerstonejs/cornerstone3D
Length of output: 1398
Match hyphenated package names in deep imports.
Replace
\\w+with([^/]+). Otherwise, imports such as@cornerstonejs/dicom-image-loader/fooresolve to.../dicom-image-loader/foo/srcinstead of.../dicom-image-loader/src/foo.🤖 Prompt for AI Agents