[wasm] Make setWasmPaths require an entry for each WASM binary. #3749
Conversation
annxingyuan
changed the title
There was a problem hiding this comment.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan and @tafsiri)
tfjs-backend-wasm/src/backend_wasm.ts, line 426 at r1 (raw file):
`${missingPaths.join(',')}. Please either call setWasmPaths with a ` + `map providing a path for each binary, or with a string indicating ` + `the directory where all the binaries can be found.`);
Does this works for url path for all binaries?
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @tafsiri)
tfjs-backend-wasm/karma.conf.js, line 64 at r1 (raw file):
// Import the generated js library from emscripten. {pattern: 'wasm-out/**/*.js'}, // Import the generated worker file from emscripten.
is this removal intended? how is it related to the rest of the change?
tfjs-backend-wasm/src/backend_wasm.ts, line 421 at r1 (raw file):
const missingPaths = wasmBinaryNames.filter(name => wasmFileMap[name] == null); if (missingPaths.length) {
change this to a more explicit check >= 0
There was a problem hiding this comment.
Hi Ann, what will happen if user provide three paths, the basic one is valid, the other two are invalid, and we try to use the invalid one, if it fails, do we try the basic one or just return setBackend('wasm') false?
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
tfjs-backend-wasm/src/backend_wasm.ts, line 421 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
change this to a more explicit check
>= 0
Isn't it > 0
There was a problem hiding this comment.
Reviewable status:
tfjs-backend-wasm/src/backend_wasm.ts, line 426 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
Does this works for url path for all binaries?
Is this a breaking change for existing code?
There was a problem hiding this comment.
Hi Na, in this case WASM backend initialization will fail. Yannick and I discussed the possibility of falling back to the basic binary, but decided to stick with this behavior for now as it is more explicit. We can definitely revisit this though if you feel strongly about it one way or another.
Reviewable status:
tfjs-backend-wasm/karma.conf.js, line 64 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
is this removal intended? how is it related to the rest of the change?
this is leftover from the previous PR where we removed the worker typescript module in favor of a plain JS one.
tfjs-backend-wasm/src/backend_wasm.ts, line 421 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Isn't it > 0
Done
tfjs-backend-wasm/src/backend_wasm.ts, line 426 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
Is this a breaking change for existing code?
Yes, I added a test for calling setWasmPaths with a string indicating the folder where all binaries can be found. Existing code calls setWasmPath (note there is no 's' at the end), which still exists, although it now logs a deprecation warning (#3627).
There was a problem hiding this comment.
Got it. SG. Thanks for the explanation, Ann!
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is