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

How to handle a failing compilation #50

Merged
merged 5 commits into from
Apr 4, 2020

Conversation

wchao1115
Copy link
Collaborator

@wchao1115 wchao1115 commented Mar 19, 2020

Update CompilationPreference to follow WebGL convention for context creation. Update the code sample to show how a certain compilation could fail such as when the model uses data type not supported by the hardware, and how to recover from the failure. #36


Preview | Diff

@wchao1115 wchao1115 requested a review from huningxin March 19, 2020 06:48
Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

Thanks @wchao1115 for the PR. Left my comments. Please take a look.

@gramalingam
Copy link
Contributor

I am fine with the proposal. My main concern was to avoid mixing potentially different concerns/options into one. It is fine to add more options later on as and when appropriate.

@huningxin
Copy link
Contributor

huningxin commented Apr 2, 2020

It is fine to add more options later on as and when appropriate.

To allow adding more options later, I propose to change the argument into a dictionary, it also aligns to the WebGL WebGLContextAttributes that contains WebGLPowerPreference.

For example:

enum PowerPreference {
  "low-power",
  "high-performance"
};

dictionary CompilationOptions {
  PowerPreference powerPreference;
};

interface Model {
  Promise<Compilation> createCompilation(optional CompilationOptions options = {});
};

@huningxin
Copy link
Contributor

Another question is in the latest commit, I failed to find the sample code that handles a failing compilation. Are there any reasons to drop it?

@wchao1115
Copy link
Collaborator Author

Another question is in the latest commit, I failed to find the sample code that handles a failing compilation. Are there any reasons to drop it?

As stated in the commit's comment, with this change the browser is expected to handle failing case due to unsupported data type, most likely by falling back to the default option.

@wchao1115
Copy link
Collaborator Author

wchao1115 commented Apr 2, 2020

dictionary CompilationOptions {
PowerPreference powerPreference;
};

I think the dictionary has merit if we believe we need something similar to WebGLContextAttributes in the future. For instance, a Boolean field similar to WebGL failIfMajorPerformanceCaveat flag could be useful in the future if we need to give the caller some control over how to handle cases where the browser's choice isn't what they are expecting.

I pushed out a new commit that addresses this feedback. Please have a look.

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

It looks good. Thanks @wchao1115 !

@huningxin
Copy link
Contributor

@wchao1115 , please help resolve the conflicts due to merging PR #49 . Thanks.

…reation. Update the code sample to show how a certain compilation could fail such as when the model uses data type not supported by the hardware, and how to recover from the failure.
We now call on the browser to properly fallback to a different compute device if the targeted device e.g. GPU is unable to support compilation of model with unsupported data type e.g. float16. Hence, we eliminate the need to define error code in such event. The browser is required to handle this failure internally and fallback as appropriate.

We also like to scope the current CompilationPreference to just the power options aka PowerPreference with two options available { 'low-power', 'speed' }. These are the same two options available in WebGLPowerPreference enum. The fact that we make the power preference option optional means we do not need to explicit define the 'default' option.
@wchao1115 wchao1115 force-pushed the wchao1115/failed_compilation branch from 21568b0 to af16a69 Compare April 4, 2020 07:33
@wchao1115 wchao1115 merged commit 3e09f7b into master Apr 4, 2020
@wchao1115 wchao1115 deleted the wchao1115/failed_compilation branch April 4, 2020 07:40
@wchao1115
Copy link
Collaborator Author

The change is reviewed and merged. I believe we can now close this issue. In summary, we decided that it's the UA responsibility to handle compilation failure for a given power preference. If an unrecoverable failure is triggered, fatal exception will then be thrown.

@huningxin
Copy link
Contributor

@wchao1115 , thanks for rebasing and merging.

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.

4 participants