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

[wasm] Cache wasm files when running Karma tests #6613

Merged
merged 4 commits into from
Jul 8, 2022

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Jul 7, 2022

Cache wasm files as blobs when running Karma tests. This prevents the browser from re-fetching the wasm files every time the wasm backend is reset (every describe block). This should reduce the number of flaky test failures due to failing to fetch the wasm files. It also removes 200MB of requests when running Karma tests.

Fix error handling when fetch() fails to download the wasm file.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Cache wasm files as blobs when running Karma tests. This prevents the browser from re-fetching the wasm files every time the wasm backend is reset (every `describe` block). This should reduce the number of flaky test failures due to failing to fetch the wasm files. It also removes 200MB of requests when running Karma tests.
@mattsoulanille
Copy link
Member Author

mattsoulanille commented Jul 8, 2022

I just now discovered the true cause of the flakiness. It was not related to network, but instead caused by bad error handling of promise rejections from fetch()ing the wasm files. I'll re-request review from @jinjingforever.

I'll (automatically) run the test 100 times tonight to see if this fixed the flakiness.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thanks, this is great.

Reviewed 3 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @jinjingforever and @mattsoulanille)


tfjs-backend-wasm/src/test_util.ts line 1 at r2 (raw file):

import * as tf from '@tensorflow/tfjs-core';

please add license header for this file.

Copy link
Collaborator

@jinjingforever jinjingforever left a comment

Choose a reason for hiding this comment

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

Nice! Thank you

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

One question why the error handling would cause flakiness? Given fetch has errors already?

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)

@mattsoulanille mattsoulanille requested a review from pyu10055 July 8, 2022 18:44
Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

I'm not sure why the error handling ever worked in the first place. I think fetch would sometimes succeed when we expected it to fail, and when I replacedfetch() with a Promise.reject(), it failed every time. This is probably the cause of the flakines.

The reason the failure caused the test to fail is because we forgot to catch a promise rejection in backend_wasm.ts. This would show up as an unhandled promise rejection and cause the test to fail, but now it's correctly passed up the promise chain and caught (and logged) by tfjs-core when it initializes the backend.

In other news, I ran the test 3000 times and it failed 5 times. The jasmine seeds that reliably reproduce errors are
86706 and 38027. This is much better than 1/3 of tests failing, which is what I saw in a sample of 100 before this change.

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @pyu10055)


tfjs-backend-wasm/src/test_util.ts line 1 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

please add license header for this file.

Thanks! Done.

@mattsoulanille mattsoulanille merged commit 8f52822 into tensorflow:master Jul 8, 2022
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