WebNN: initial implementation of adding label for MLOperator [chromium/src : main]
Mingming1 Xu (Gerrit)
Mingming1 Xu added 1 comment![]( "Open in Gerrit")
const OperatorNode* resample2d_node = graph_builder.CreateOperatorNode(You can pass label to dml operator here.
Related details
- Lisha Guo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
const OperatorNode* resample2d_node = graph_builder.CreateOperatorNode(You can pass label to dml operator here.
Sounds good. Set name for dml operator in PS4. Thanks!
Related details
- Mingming1 Xu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingming1 Xu (Gerrit)
Mingming1 Xu added 1 comment![]( "Open in Gerrit")
const OperatorNode* resample2d_node = graph_builder.CreateOperatorNode(Lisha GuoYou can pass label to dml operator here.
Sounds good. Set name for dml operator in PS4. Thanks!
- Lisha Guo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
As the discussion in this issue -https://github.com/webmachinelearning/webnn/issues/585, I just implement a prototype which adds labels to MLOperator. The downside with adding to the MLOperator is that it doesn't exist as a concept in the spec, so we will need to add the param to each of the builder method. The advantages compared to adding to MLOperand are listed below:
1. We can get the label in the error message in the validation phase.
2. Since build() is async we need to deal with labels changing during the build process. Right now MLOperand is immutable.
3. If a sequence of operands were returned when invoking some operator, such as split, we need to make a rule to set the label.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 3 comments![]( "Open in Gerrit")
std::optional<std::string> label) {remove the param and get from resample2d->label()
error_code, base::StrCat({kBackendName, label.value(), error_message}));How about base::StrCat({"[", label.value(), "] ", error_message}); and drop the backend name? The backend name feels like an implementation detail we don't need to expose.
const String label = options->hasLabel() ? options->label() : "resample2d";I think we can always have the operator name in the log whether the label is presented or not.
Also you can use ops::kResample2d
Related details
- Dwayne Robinson
- Lisha Guo
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 3 comments![]( "Open in Gerrit")
remove the param and get from resample2d->label()
Done
error_code, base::StrCat({kBackendName, label.value(), error_message}));How about base::StrCat({"[", label.value(), "] ", error_message}); and drop the backend name? The backend name feels like an implementation detail we don't need to expose.
Agree that one day when all implementation is stable, we can remove many error messages including this one. But currently, we have reported some detailed error messages, for example "Failed to create pooling operator.", the backend name let developer know which backend reports these messages, it can help us to locate and debug.
How about
```
if (label.has_value()) {
return mojom::Error::New(error_code,
base::StrCat({kBackendName, "[", label.value(), "] ",
error_message}));
}
```
const String label = options->hasLabel() ? options->label() : "resample2d";I think we can always have the operator name in the log whether the label is presented or not.
Also you can use ops::kResample2d
Remove the default name. If the label is not presented, label will be assigned with empty string.
Related details
- Dwayne Robinson
- Phillis Tang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 3 comments![]( "Open in Gerrit")
error_code, base::StrCat({kBackendName, label.value(), error_message}));Lisha GuoHow about base::StrCat({"[", label.value(), "] ", error_message}); and drop the backend name? The backend name feels like an implementation detail we don't need to expose.
Agree that one day when all implementation is stable, we can remove many error messages including this one. But currently, we have reported some detailed error messages, for example "Failed to create pooling operator.", the backend name let developer know which backend reports these messages, it can help us to locate and debug.
How about
```
if (label.has_value()) {
return mojom::Error::New(error_code,
base::StrCat({kBackendName, "[", label.value(), "] ",
error_message}));
}
```
SGTM for now.
base::StrCat({kBackendName, "[", label.value(), "] ", error_message}));let's create a helper function in webnn_utils like:
```
std::string GetLabelErrorSuffix(const str::optional<std::string& label) {
if (!label.has_value()){
return "";
}
return base::StrCat("[", label.value(), "] ");
}
```and we can use it here as:
```
base:: StrCat({kBackendName, GetLabelErrorSuffix(label), error_message});
```
and also in ml_graph_builder.cc
const String label = options->hasLabel() ? options->label() : "resample2d";Lisha GuoI think we can always have the operator name in the log whether the label is presented or not.
Also you can use ops::kResample2d
Remove the default name. If the label is not presented, label will be assigned with empty string.
oh I was suggesting something like:
```
ml_context_->LogConsoleWarning(base:: StrCat({GetLabelErrorSuffix(label), ops::kResample2d, ": ", error_message});
```
Related details
- Dwayne Robinson
- Lisha Guo
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
if (label.has_value()) {let's add the comment here too.
Mingming1 Xu (Gerrit)
Mingming1 Xu added 1 comment![]( "Open in Gerrit")
dictionary MLResample2dOptions : MLLabelOptions{How about renaming as `MLOperatorOptions`? Each like `MLResample2dOptions` and `MLReduceOptions` should inherit the base operator options `MLOperatorOptions`
Lisha Guo (Gerrit)
Lisha Guo added 3 comments![]( "Open in Gerrit")
let's add the comment here too.
Done
base::StrCat({kBackendName, "[", label.value(), "] ", error_message}));let's create a helper function in webnn_utils like:
```
std::string GetLabelErrorSuffix(const str::optional<std::string& label) {if (!label.has_value()){
return "";
}
return base::StrCat("[", label.value(), "] ");
}
```and we can use it here as:
```
base:: StrCat({kBackendName, GetLabelErrorSuffix(label), error_message});
```
and also in ml_graph_builder.cc
Done
const String label = options->hasLabel() ? options->label() : "resample2d";Lisha GuoI think we can always have the operator name in the log whether the label is presented or not.
Also you can use ops::kResample2d
Phillis TangRemove the default name. If the label is not presented, label will be assigned with empty string.
oh I was suggesting something like:
```
ml_context_->LogConsoleWarning(base:: StrCat({GetLabelErrorSuffix(label), ops::kResample2d, ": ", error_message});
```
- Dwayne Robinson
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
String::Format("%s ops::kResample2d: When sizes and scales are both "I meant using ops::kResample2d in webnn_utils, not the literal string.
Related details
- Dwayne Robinson
- Lisha Guo
- ningxin hu
Dwayne Robinson (Gerrit)
Dwayne Robinson added 4 comments![]( "Open in Gerrit")
const std::optional<std::string>& label = resample2d->label;(repeated comment from other CR) When/if we autogenerate missing node names later, I want to avoid each backend generating its own names which makes it more brittle to inconsistencies, making it harder to compare backend results when one has a failure. So this shouldn't be `optional` after that, as the front-end already generated the labels for each backend. (resolving for now)
error_code, base::StrCat({kBackendName, label.value(), error_message}));Lisha GuoHow about base::StrCat({"[", label.value(), "] ", error_message}); and drop the backend name? The backend name feels like an implementation detail we don't need to expose.
Agree that one day when all implementation is stable, we can remove many error messages including this one. But currently, we have reported some detailed error messages, for example "Failed to create pooling operator.", the backend name let developer know which backend reports these messages, it can help us to locate and debug.
How about
```
if (label.has_value()) {
return mojom::Error::New(error_code,
base::StrCat({kBackendName, "[", label.value(), "] ",
error_message}));
}
```Phillis Tang
Dwayne RobinsonSGTM for now.
Yeah, having the backend would be helpful for now. Once they reach parity of completeness, it will probably be less relevant.
String::Format("%s ops::kResample2d: When sizes and scales are both "I meant using ops::kResample2d in webnn_utils, not the literal string.
- Lisha Guo
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 2 comments![]( "Open in Gerrit")
For PS12, this CL added the implementation of prelu which doesn't have any options before.
@dwa...@microsoft.com Please take another look. Thanks!
String::Format("%s ops::kResample2d: When sizes and scales are both "Dwayne RobinsonI meant using ops::kResample2d in webnn_utils, not the literal string.
++
- Dwayne Robinson
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 4 comments![]( "Open in Gerrit")
webnn::OpTagToString(webnn::mojom::blink::Operation::Tag::kPrelu)no need to use OpTagToString, just use `webnn::ops::kPrelu`, same for resample2d
dictionary MLResample2dOptions : MLLabelOptions{How about renaming as `MLOperatorOptions`? Each like `MLResample2dOptions` and `MLReduceOptions` should inherit the base operator options `MLOperatorOptions`
+1
[RaisesException] MLOperand prelu(MLOperand x, MLOperand slope, optional MLPreluOptions options = {});we can just specify `optional MLOperatorOptions`? No need to define a MLPreluOptions
remove
- Dwayne Robinson
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Dwayne Robinson (Gerrit)
Dwayne Robinson added 3 comments![]( "Open in Gerrit")
👍 LGTM after addressing other's comments.
dictionary MLResample2dOptions : MLLabelOptions{Phillis TangHow about renaming as `MLOperatorOptions`? Each like `MLResample2dOptions` and `MLReduceOptions` should inherit the base operator options `MLOperatorOptions`
+1
++
[RaisesException] MLOperand prelu(MLOperand x, MLOperand slope, optional MLPreluOptions options = {});we can just specify `optional MLOperatorOptions`? No need to define a MLPreluOptions
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 4 comments![]( "Open in Gerrit")
webnn::OpTagToString(webnn::mojom::blink::Operation::Tag::kPrelu)no need to use OpTagToString, just use `webnn::ops::kPrelu`, same for resample2d
Done
dictionary MLResample2dOptions : MLLabelOptions{Phillis TangHow about renaming as `MLOperatorOptions`? Each like `MLResample2dOptions` and `MLReduceOptions` should inherit the base operator options `MLOperatorOptions`
Dwayne Robinson+1
++
Sound good. Rename `MLLabelOptions` to `MLOperatorOptions` in PS13.
[RaisesException] MLOperand prelu(MLOperand x, MLOperand slope, optional MLPreluOptions options = {});Dwayne Robinsonwe can just specify `optional MLOperatorOptions`? No need to define a MLPreluOptions
++
Use `MLOperatorOptions` instead of `MLPreluOptions` in PS13.
remove
- Dwayne Robinson
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dwayne Robinson (Gerrit)
Dwayne Robinson added 1 comment![]( "Open in Gerrit")
webnn::OpTagToString(webnn::mojom::blink::Operation::Tag::kPrelu)Lisha Guono need to use OpTagToString, just use `webnn::ops::kPrelu`, same for resample2d
Done
Phillis more likely meant...
```
exception_state.ThrowTypeError(String::Format(
"%s %s: %s", webnn::GetLabelErrorSuffix(label).c_str(),
webnn::ops::kPrelu, ...
```
...and not literally type `webnn::ops::kPrelu` in the user-visible string.
Related details
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
webnn::OpTagToString(webnn::mojom::blink::Operation::Tag::kPrelu)Lisha Guono need to use OpTagToString, just use `webnn::ops::kPrelu`, same for resample2d
Dwayne RobinsonDone
Phillis more likely meant...
```
exception_state.ThrowTypeError(String::Format(
"%s %s: %s", webnn::GetLabelErrorSuffix(label).c_str(),
webnn::ops::kPrelu, ...
```
...and not literally type `webnn::ops::kPrelu` in the user-visible string.
Sorry for the misunderstanding. Updated the code in PS15. Thanks!
Related details
- Dwayne Robinson
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dwayne Robinson (Gerrit)
Dwayne Robinson added 1 comment![]( "Open in Gerrit")
👍 Thanks for improving the diagnosability for WebNN users, Lisha.
Related details
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 2 comments![]( "Open in Gerrit")
👍 Thanks for improving the diagnosability for WebNN users, Lisha.
It is my pleasure. Look forward to your edit of WebNN spec.
👍 Thanks for improving the diagnosability for WebNN users, Lisha.
- Dwayne Robinson
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang voted Code-Review+1![]( "Open in Gerrit")
- Dwayne Robinson
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
- Chromium IPC Reviews
- Dwayne Robinson
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: kin...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dwayne Robinson
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ningxin hu (Gerrit)
ningxin hu added 8 comments![]( "Open in Gerrit")
This CL attempts to add label for MLOperator to report more detailed
error message during the async build.You may want to mention this CL only implements the label for two operators `prelu` and `resample2d`. Other operators will be supported in a separate CL.
Please also add a reference to spec discussion https://github.com/webmachinelearning/webnn/issues/585
"The data type of the input must be "
"one of the floating point types.");Do we really need this change?
// stored in `GraphBuilder::operator_nodes_` and returns its pointer. When itThe class name is now `GraphBuilderDml`.
// Specifies the name for the operatorAdd period at the end of this sentence.
/*sub_kind=*/absl::monostate{});`options` should be passed to `MLOperator` constructor otherwise it will use the default value nullptr.
```suggestion
/*sub_kind=*/absl::monostate{}, options);
```
"%s %s: When sizes and scales are both "
"specified, scales argument is "
"ignored.",Can this message be in two lines? The lines seem to break too short?
Related details
- Dwayne Robinson
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 9 comments![]( "Open in Gerrit")
Uploaded PS19 to reach your comments. @ningx...@intel.com @phi...@chromium.org Please take another look. Thanks!
This CL attempts to add label for MLOperator to report more detailed
error message during the async build.You may want to mention this CL only implements the label for two operators `prelu` and `resample2d`. Other operators will be supported in a separate CL.
Done
Please also add a reference to spec discussion https://github.com/webmachinelearning/webnn/issues/585
Done
"The data type of the input must be "
"one of the floating point types.");Do we really need this change?
Done
// stored in `GraphBuilder::operator_nodes_` and returns its pointer. When itThe class name is now `GraphBuilderDml`.
Done
Add period at the end of this sentence.
Done
// Specifies the name for the operatorLisha Guoditto.
Done
`options` should be passed to `MLOperator` constructor otherwise it will use the default value nullptr.
```suggestion
/*sub_kind=*/absl::monostate{}, options);
```
Done
"%s %s: When sizes and scales are both "
"specified, scales argument is "
"ignored.",Can this message be in two lines? The lines seem to break too short?
- Dwayne Robinson
- Kinuko Yasuda
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang added 2 comments![]( "Open in Gerrit")
char op_str[] = "webnn::ops::kResample2d";never creates a op str, just use the webnn::ops::kResample2d variable.
ASSIGN_OR_THROW_AND_RETURN_IF_ERROR_WITH_LABEL(instead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to `ValidateResample2dAndInferOutput`?
This allows more flexibilities on where to put the label and op name in the error message. Also the macro is kind of hard to read with implicitly capturing of label and op_str.
Related details
- Dwayne Robinson
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
ASSIGN_OR_THROW_AND_RETURN_IF_ERROR_WITH_LABEL(instead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to `ValidateResample2dAndInferOutput`?
This allows more flexibilities on where to put the label and op name in the error message. Also the macro is kind of hard to read with implicitly capturing of label and op_str.
instead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to ValidateResample2dAndInferOutput?
Agree. Move the logic down to validate functions seems more flexibilities. However, I can not find out how to use the `GetLabelErrorSuffix` and `webnn::ops::kResample2d` defined in services/webnn/webnn_util.h from the `validateResample2dAndInferOut` which is located in `services/webnn/public/cpp/graph_validation_utils.h`. If we add the deps of `services/webnn::webnn_utils` in the build.gn file under the `services/webnn/public/cpp`, the ninja failed as below:
```
Regenerating ninja filesERROR Dependency cycle:
//services/webnn/public/mojom:mojom ->
//services/webnn/public/mojom:webnn_mojom_traits ->
//services/webnn/public/cpp:cpp ->
//services/webnn:webnn_utils ->
//services/webnn/public/mojom:mojom
```
Could you please share some solution?
Related details
- Kinuko Yasuda
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
ASSIGN_OR_THROW_AND_RETURN_IF_ERROR_WITH_LABEL(Lisha Guoinstead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to `ValidateResample2dAndInferOutput`?
This allows more flexibilities on where to put the label and op name in the error message. Also the macro is kind of hard to read with implicitly capturing of label and op_str.
instead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to ValidateResample2dAndInferOutput?
Agree. Move the logic down to validate functions seems more flexibilities. However, I can not find out how to use the `GetLabelErrorSuffix` and `webnn::ops::kResample2d` defined in services/webnn/webnn_util.h from the `validateResample2dAndInferOut` which is located in `services/webnn/public/cpp/graph_validation_utils.h`. If we add the deps of `services/webnn::webnn_utils` in the build.gn file under the `services/webnn/public/cpp`, the ninja failed as below:
```
Regenerating ninja filesERROR Dependency cycle:
//services/webnn/public/mojom:mojom ->
//services/webnn/public/mojom:webnn_mojom_traits ->
//services/webnn/public/cpp:cpp ->
//services/webnn:webnn_utils ->
//services/webnn/public/mojom:mojom
```
Could you please share some solution?
Oh right, now that https://chromium-review.googlesource.com/c/chromium/src/+/5608793 is merged, you can move the function to services/webnn/public/cpp/webnn_errors.h
Related details
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
ASSIGN_OR_THROW_AND_RETURN_IF_ERROR_WITH_LABEL(Lisha Guoinstead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to `ValidateResample2dAndInferOutput`?
This allows more flexibilities on where to put the label and op name in the error message. Also the macro is kind of hard to read with implicitly capturing of label and op_str.
Phillis Tanginstead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to ValidateResample2dAndInferOutput?
Agree. Move the logic down to validate functions seems more flexibilities. However, I can not find out how to use the `GetLabelErrorSuffix` and `webnn::ops::kResample2d` defined in services/webnn/webnn_util.h from the `validateResample2dAndInferOut` which is located in `services/webnn/public/cpp/graph_validation_utils.h`. If we add the deps of `services/webnn::webnn_utils` in the build.gn file under the `services/webnn/public/cpp`, the ninja failed as below:
```
Regenerating ninja filesERROR Dependency cycle:
//services/webnn/public/mojom:mojom ->
//services/webnn/public/mojom:webnn_mojom_traits ->
//services/webnn/public/cpp:cpp ->
//services/webnn:webnn_utils ->
//services/webnn/public/mojom:mojom
```
Could you please share some solution?
Oh right, now that https://chromium-review.googlesource.com/c/chromium/src/+/5608793 is merged, you can move the function to services/webnn/public/cpp/webnn_errors.h
also you don't need to include the op name in the error message, as they should be TypeError like:
```
TypeError: Failed to execute 'resample2d' on 'MLGraphBuilder': ...'
```
Lisha Guo (Gerrit)
Lisha Guo added 3 comments![]( "Open in Gerrit")
Rebased to the latest main branch. @phi...@chromium.org Please take another look. Thanks!
never creates a op str, just use the webnn::ops::kResample2d variable.
Done
Lisha Guoinstead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to `ValidateResample2dAndInferOutput`?
This allows more flexibilities on where to put the label and op name in the error message. Also the macro is kind of hard to read with implicitly capturing of label and op_str.
Phillis Tanginstead of creating this macro that piece together an error message from a downstream message, can we just move the logic down to ValidateResample2dAndInferOutput?
Agree. Move the logic down to validate functions seems more flexibilities. However, I can not find out how to use the `GetLabelErrorSuffix` and `webnn::ops::kResample2d` defined in services/webnn/webnn_util.h from the `validateResample2dAndInferOut` which is located in `services/webnn/public/cpp/graph_validation_utils.h`. If we add the deps of `services/webnn::webnn_utils` in the build.gn file under the `services/webnn/public/cpp`, the ninja failed as below:
```
Regenerating ninja filesERROR Dependency cycle:
//services/webnn/public/mojom:mojom ->
//services/webnn/public/mojom:webnn_mojom_traits ->
//services/webnn/public/cpp:cpp ->
//services/webnn:webnn_utils ->
//services/webnn/public/mojom:mojom
```
Could you please share some solution?
Phillis TangOh right, now that https://chromium-review.googlesource.com/c/chromium/src/+/5608793 is merged, you can move the function to services/webnn/public/cpp/webnn_errors.h
also you don't need to include the op name in the error message, as they should be TypeError like:
```
TypeError: Failed to execute 'resample2d' on 'MLGraphBuilder': ...'
```
- Kinuko Yasuda
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
Adding @aj...@chromium.org as reviewer since this CL also modified the mojo definition. PTAL. Thanks!
Related details
- Alex Gough
- Kinuko Yasuda
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phillis Tang (Gerrit)
Phillis Tang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Phillis Tang
| Code-Review | +1 |
4 comments
LGTM % minor suggestions
const std::optional<std::string>& label = std::nullopt);ditto
const std::optional<std::string>& label = std::nullopt);let's remove the default std::nullopt and always require this param.
base::StrCat({GetLabelErrorSuffix(label), "The scale is too large."}));nit: move this to a helper function:
```
namespace {
std::string ErrorWithLabel(base::string_view error_message, std::optional<std::string> label){
...
};
} // namespace
```
Related details
- Alex Gough
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Code-Coverage
Code-Review
No-Unresolved-Comments
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
why does the label need to go to the gpu side of the mojom interface - we shouldn't be generating graphs that are invalid through that point?
Related details
Alex Gough (Gerrit)
Alex Gough added 2 comments![]( "Open in Gerrit")
string? label;if the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
CHECK(options);don't need to CHECK() you will crash anyway
Phillis Tang (Gerrit)
Phillis Tang added 2 comments![]( "Open in Gerrit")
why does the label need to go to the gpu side of the mojom interface - we shouldn't be generating graphs that are invalid through that point?
the graph could be valid and there could be backend specific errors during graph execution.
string? label;if the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
it's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
Related details
- Alex Gough
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
string? label;Phillis Tangif the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
it's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
are there other cases where we provide this level of detail back to the renderer from a backend process? It feels somewhat wasteful to send strings around only in case they are needed for error reporting
Related details
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
string? label;Phillis Tangif the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
Alex Goughit's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
are there other cases where we provide this level of detail back to the renderer from a backend process? It feels somewhat wasteful to send strings around only in case they are needed for error reporting
the concept is borrowed from webgpu(consulted with their team), and the problem we are trying to solve is that it's very hard for developers to debug async errors coming back from the backend process during graph compilation/execution, so having user defined labels are very helpful to decipher errors from a complex graph.
Related details
- Alex Gough
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
string? label;Phillis Tangif the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
Alex Goughit's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
Phillis Tangare there other cases where we provide this level of detail back to the renderer from a backend process? It feels somewhat wasteful to send strings around only in case they are needed for error reporting
the concept is borrowed from webgpu(consulted with their team), and the problem we are trying to solve is that it's very hard for developers to debug async errors coming back from the backend process during graph compilation/execution, so having user defined labels are very helpful to decipher errors from a complex graph.
thanks - perhaps this can be called 'debug_label' to make its purpose clearer?
Related details
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 2 comments![]( "Open in Gerrit")
// Specifies the name for the operator.Update the comment to "User defined label from `MLOperatorOptions`"
string? label;Phillis Tangif the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
Alex Goughit's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
Phillis Tangare there other cases where we provide this level of detail back to the renderer from a backend process? It feels somewhat wasteful to send strings around only in case they are needed for error reporting
Alex Goughthe concept is borrowed from webgpu(consulted with their team), and the problem we are trying to solve is that it's very hard for developers to debug async errors coming back from the backend process during graph compilation/execution, so having user defined labels are very helpful to decipher errors from a complex graph.
thanks - perhaps this can be called 'debug_label' to make its purpose clearer?
seems fine, although would it be confusing to have the mojo name different from the idl? (the comment should be updated to clarify though)
For the idl I'd prefer `label` as that's a [concept](https://www.w3.org/TR/webgpu/#dom-gpuobjectbase-label) develoeprs already familiar with from WebGPU.
Related details
- Alex Gough
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Alex Gough (Gerrit)
Alex Gough added 2 comments![]( "Open in Gerrit")
return base::StrCat({"[", label.value(), "] "});would it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
string? label;Phillis Tangif the gpu side /needs/ to have an identifier that makes sense in the renderer then this could instead be a uint64 label_id and the labels live in the renderer all the time?
Alex Goughit's less as an identifier(doesn't need to be unique) but a descriptor to be attached to error message.
Phillis Tangare there other cases where we provide this level of detail back to the renderer from a backend process? It feels somewhat wasteful to send strings around only in case they are needed for error reporting
Alex Goughthe concept is borrowed from webgpu(consulted with their team), and the problem we are trying to solve is that it's very hard for developers to debug async errors coming back from the backend process during graph compilation/execution, so having user defined labels are very helpful to decipher errors from a complex graph.
Phillis Tangthanks - perhaps this can be called 'debug_label' to make its purpose clearer?
seems fine, although would it be confusing to have the mojo name different from the idl? (the comment should be updated to clarify though)
For the idl I'd prefer `label` as that's a [concept](https://www.w3.org/TR/webgpu/#dom-gpuobjectbase-label) develoeprs already familiar with from WebGPU.
`label` is ok if it matches the idl - thanks for the gpu example.
Related details
Phillis Tang (Gerrit)
Phillis Tang added 2 comments![]( "Open in Gerrit")
return base::StrCat({"[", label.value(), "] "});would it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
would it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Seems irrelevant to me. If develoeprs label them "[[[" they will just get a less helpful error message.
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Do you have a case in mind that's unsafe?
};thinking more about it. I think passing empty string should be treated the same as not passing label. So how about making it not optional in mojom. So that in third_party/blink/renderer/modules/ml/webnn/ml_graph_type_converter.cc , if it's null we just set it to empty string?
Then we can replace all the chunky `const std::optional<std::string>&` with `std::string_view`.
```
std::string GetLabelErrorSuffix(string_view label) {
if (label.empty()) {
return "";
}
return base::StrCat({"[", label, "] "});
```
Related details
- Alex Gough
Alex Gough (Gerrit)
Alex Gough added 1 comment![]( "Open in Gerrit")
return base::StrCat({"[", label.value(), "] "});Phillis Tangwould it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
would it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Seems irrelevant to me. If develoeprs label them "[[[" they will just get a less helpful error message.Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Do you have a case in mind that's unsafe?
I guess I don't just mean here - will these error strings be interpreted somewhere later - are there any constraints that the labels should meet?
Related details
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Lisha Guo (Gerrit)
Lisha Guo added 1 comment![]( "Open in Gerrit")
const std::optional<std::string>& label = std::nullopt);let's remove the default std::nullopt and always require this param.
Setting the default std::nullopt because this validate method is also invoked by webnn_graph_impl.cc- https://source.chromium.org/chromium/chromium/src/+/main:services/webnn/webnn_graph_impl.cc?q=ValidateResample2dAndInferOutput&ss=chromium%2Fchromium%2Fsrc.
Related details
- Kinuko Yasuda
Phillis Tang (Gerrit)
Phillis Tang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Phillis Tang
| Code-Review | +0 |
1 comment
const std::optional<std::string>& label = std::nullopt);Lisha Guolet's remove the default std::nullopt and always require this param.
Setting the default std::nullopt because this validate method is also invoked by webnn_graph_impl.cc- https://source.chromium.org/chromium/chromium/src/+/main:services/webnn/webnn_graph_impl.cc?q=ValidateResample2dAndInferOutput&ss=chromium%2Fchromium%2Fsrc.
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
return base::StrCat({"[", label.value(), "] "});Phillis Tangwould it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Alex Goughwould it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Seems irrelevant to me. If develoeprs label them "[[[" they will just get a less helpful error message.Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Do you have a case in mind that's unsafe?
I guess I don't just mean here - will these error strings be interpreted somewhere later - are there any constraints that the labels should meet?
It should be mostly used by reporting error using this helper method.
Another place it's used right now is as [directml operator name](https://chromium-review.googlesource.com/c/chromium/src/+/5528797/21/services/webnn/dml/graph_builder_dml.cc#112) that's also used for debugging purpose.
As for constraints, after some reading, it seems we should switch to USVString for the idl, so it's safe to utf8 encode and pass to backends. @lish...@intel.com
Related details
- Alex Gough
Alex Gough (Gerrit)
Alex Gough voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Gough
| Code-Review | +1 |
2 comments
lgtm mojom
return base::StrCat({"[", label.value(), "] "});Phillis Tangwould it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Alex Goughwould it be good/bad/irrelevant if label is something like "[[[[[" or "]]]]]" ?
Seems irrelevant to me. If develoeprs label them "[[[" they will just get a less helpful error message.Is it safe to generate error strings using renderer supplied values, or should the label and error message be rejoined in the renderer?
Do you have a case in mind that's unsafe?
Phillis TangI guess I don't just mean here - will these error strings be interpreted somewhere later - are there any constraints that the labels should meet?
It should be mostly used by reporting error using this helper method.
Another place it's used right now is as [directml operator name](https://chromium-review.googlesource.com/c/chromium/src/+/5528797/21/services/webnn/dml/graph_builder_dml.cc#112) that's also used for debugging purpose.
As for constraints, after some reading, it seems we should switch to USVString for the idl, so it's safe to utf8 encode and pass to backends. @lish...@intel.com
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- Phillis Tang
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
DOMString label;let's switch to USVString instead so it's safe to utf8 encode and pass to backends.
(I will open an issue for operand name too)
Related details
- Kinuko Yasuda
- Lisha Guo
- Mingming1 Xu
- ningxin hu
Phillis Tang (Gerrit)
Phillis Tang added 1 comment![]( "Open in Gerrit")
DOMString label;let's switch to USVString instead so it's safe to utf8 encode and pass to backends.
(I will open an issue for operand name too)
https://github.com/webmachinelearning/webnn/issues/713 for operand input