Skip to content

Conversation

@jameshfisher
Copy link
Contributor

@jameshfisher jameshfisher commented Sep 24, 2020

My day today: I spent a long time debugging the error Initialization of backend webgl failed on a particular machine. Ultimately, the cause was really obscure: Chrome was returning null from canvas.getContext when passed this failIfMajorPerformanceCaveat flag. Another line in TensorFlow assumes that getContext cannot return null, and so throws an error TypeError: Cannot read property 'isContextLost' of null. This is another bug. But that error was then silently swallowed at the line here.

On restarting the machine, Chrome mysteriously stopped behaving this way. But that underlying issue is a story for another time.

The point is, silently swallowing all thrown errors is a pretty bad practice. To go further, unless we actually expect errors thrown here, I think this try/catch should be removed - if it fails unexpectedly, just let it fail. If we do expect errors thrown here, that should at least be documented in a comment here.

This change is Reviewable

@jameshfisher
Copy link
Contributor Author

(I'll follow up with a separate PR fixing the TypeError: Cannot read property 'isContextLost' of null error that I found was being silently swallowed)

@jameshfisher
Copy link
Contributor Author

@annxingyuan thanks for the +1 - I see the PR failed at this URL, however that page just says "You do not have sufficient permissions to view this page" :-(

@annxingyuan
Copy link
Contributor

Hi @jameshfisher - in order to see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

Looks like you figured out the issue :)

@jameshfisher
Copy link
Contributor Author

Thanks - I found that text in my second PR description; I think I mindlessly deleted it the first time!

@annxingyuan annxingyuan merged commit 6d2fdc3 into tensorflow:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants