Upgrade Emscripten and add correctness test to benchmarks UI. #3342
Conversation
annxingyuan
requested review from
dsmilkov,
lina128 and
nsthorat
and removed request for
lina128
…upgrade_emscripten
There was a problem hiding this comment.
Few non-blocking qs. LGTM! Thanks Ann
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, @nsthorat, @pyu10055, and @tafsiri)
tfjs-backend-wasm/cloudbuild.yml, line 58 at r1 (raw file):
waitFor: ['build'] # Check bundle size.
add a TODO(issue_link): Reenable this when ....
tfjs-backend-wasm/scripts/build-ci.sh, line 35 at r1 (raw file):
done ./emsdk activate --no-embedded 1.39.13
for my own understanding, is this flag required for 1.39.13 works? Does it error without that flag?
tfjs-backend-wasm/src/index_test.ts, line 121 at r1 (raw file):
return new Promise((resolve, reject) => { reject(new Error());
curious, what changed here? why using a real fetch to an invalid path doesn't work anymore?
tfjs-core/benchmarks/index.html, line 187 at r1 (raw file):
async function loadModelAndRecordTime() { const start = performance.now();
this start will now measure UI work and 5ms await. Should start go after showMsg()?
tfjs-core/benchmarks/util.js, line 30 at r1 (raw file):
const epsilon = 1e-3; function arraysClose(n1, n2) {
tf.test_util.expectArraysClose ?
There was a problem hiding this comment.
Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
tfjs-backend-wasm/src/index_test.ts, line 113 at r2 (raw file):
// Disabling this test because it intermittently times out on CI. // it('backend init fails when the path is invalid and use platform fetch',
you can use xit to skip test, instead of comment out everything.
There was a problem hiding this comment.
Reviewable status:
tfjs-backend-wasm/cloudbuild.yml, line 58 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
add a TODO(issue_link): Reenable this when ....
Done
tfjs-backend-wasm/scripts/build-ci.sh, line 35 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
for my own understanding, is this flag required for 1.39.13 works? Does it error without that flag?
Strangely, this error started showing up on master (1.39.1). Without this flag emscripten writes configuration to the Emscripten SDK directory instead of user home. embedded seems to be the default (https://github.com/emscripten-core/emsdk/blob/858b176f68ddd040e505f7ce6af637e99cdb9d42/emsdk.py#L2769) so we pass --no-embedded.
tfjs-backend-wasm/src/index_test.ts, line 121 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
curious, what changed here? why using a real fetch to an invalid path doesn't work anymore?
Actually I think the root issue is that real fetch with invalid path is flaky and intermittently times out. I removed my 'fix' and am using xit instead.
tfjs-backend-wasm/src/index_test.ts, line 113 at r2 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
you can use xit to skip test, instead of comment out everything.
Done
tfjs-core/benchmarks/index.html, line 187 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
this start will now measure UI work and 5ms await. Should
startgo aftershowMsg()?
Done
tfjs-core/benchmarks/util.js, line 30 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
tf.test_util.expectArraysClose ?
Done
Changes
no-embeddedto emsdk activate inbuild-ci.shto ensure it writes to home directory (see: emsdk activate doesn't write to home directory emscripten-core/emscripten#9682, https://github.com/emscripten-core/emsdk/blob/858b176f68ddd040e505f7ce6af637e99cdb9d42/emsdk.py#L3041)test-bundle-sizefrom WASM because it usesbuild-ci.shfrom master which does not include theno-embeddedfix.To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is