[WebNN] Enable model dumping for the OpenVINO EP [chromium/src : main]
Wang, Wei4 (Gerrit)
Wang, Wei4 added 1 comment![]( "Open in Gerrit")
Related details
- Hu, Ningxin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hu, Ningxin (Gerrit)
Hu, Ningxin added 5 comments![]( "Open in Gerrit")
const OrtEpDevice* first_selected_device) {Just pass the primary ep name?
if (first_selected_ep_name == kOpenVINOExecutionProvider) {You may want to consolidate the ep specific config info in the `EpInfo`? e.g. consider adding an optional `model_dump_config_key` in `EpInfo`, for OV EP, it sets to "ep.openvinoexecutionprovider.dump_subgraphs". That would make the code in this file to be ep agnostic.
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
ScopedOrtSessionOptions session_options =
CreateBaseSessionOptions(target_ort_device);
// Disable CPU EP fallback to ensure the session will be created on the
// expected EP device.
CHECK_STATUS(ort_api->AddSessionConfigEntry(
session_options.get(), kOrtSessionOptionsDisableCPUEPFallback, "1"));
const auto ep_it = kKnownEPs.find(target_device.ep_name);
if (ep_it != kKnownEPs.end()) {
for (const auto& config_entry : ep_it->second.config_entries) {
CHECK_STATUS(ort_api->AddSessionConfigEntry(
session_options.get(),
/*config_key=*/config_entry.key.c_str(),
/*config_value=*/config_entry.value.c_str()));
}
}
You may want to document this change in the commit log.
Related details
- Rafael Cintron
- Wang, Wei4
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang, Wei4 (Gerrit)
Wang, Wei4 added 4 comments![]( "Open in Gerrit")
Just pass the primary ep name?
Thanks， done
if (first_selected_ep_name == kOpenVINOExecutionProvider) {You may want to consolidate the ep specific config info in the `EpInfo`? e.g. consider adding an optional `model_dump_config_key` in `EpInfo`, for OV EP, it sets to "ep.openvinoexecutionprovider.dump_subgraphs". That would make the code in this file to be ep agnostic.
Thanks for the suggestion, PTAL again.
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
I have updated the doc of kWebNNOrtDumpModel. I think introducing a separate switch can be be confusing because it becomes complicated to use.
ScopedOrtSessionOptions session_options =
CreateBaseSessionOptions(target_ort_device);
// Disable CPU EP fallback to ensure the session will be created on the
// expected EP device.
CHECK_STATUS(ort_api->AddSessionConfigEntry(
session_options.get(), kOrtSessionOptionsDisableCPUEPFallback, "1"));
const auto ep_it = kKnownEPs.find(target_device.ep_name);
if (ep_it != kKnownEPs.end()) {
for (const auto& config_entry : ep_it->second.config_entries) {
CHECK_STATUS(ort_api->AddSessionConfigEntry(
session_options.get(),
/*config_key=*/config_entry.key.c_str(),
/*config_value=*/config_entry.value.c_str()));
}
}
You may want to document this change in the commit log.
- Hu, Ningxin
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hu, Ningxin (Gerrit)
Hu, Ningxin added 2 comments![]( "Open in Gerrit")
if (first_selected_ep_name == kOpenVINOExecutionProvider) {You may want to consolidate the ep specific config info in the `EpInfo`? e.g. consider adding an optional `model_dump_config_key` in `EpInfo`, for OV EP, it sets to "ep.openvinoexecutionprovider.dump_subgraphs". That would make the code in this file to be ep agnostic.
Thanks for the suggestion, PTAL again.
Looks good, thanks!
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Wang, Wei4Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
I have updated the doc of kWebNNOrtDumpModel. I think introducing a separate switch can be be confusing because it becomes complicated to use.
Dumping ONNX model and EP specific model seem to be orthogonal to me. Not sure whether we should have them seperate. @aditya....@microsoft.com for comments.
Related details
- Rafael Cintron
- Wang, Wei4
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditya Rastogi (Gerrit)
Aditya Rastogi added 1 comment![]( "Open in Gerrit")
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Wang, Wei4Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
Hu, NingxinI have updated the doc of kWebNNOrtDumpModel. I think introducing a separate switch can be be confusing because it becomes complicated to use.
Dumping ONNX model and EP specific model seem to be orthogonal to me. Not sure whether we should have them seperate. @aditya....@microsoft.com for comments.
Agree they are orthogonal concepts, but if we had two switches, I think it becomes a little more tricky to use (e.g., the new switch wouldn't work for CPU / DML, and the ONNX path seems like it would not be valid for OpenVINO based on the comments in the PR). Is it ok to start with this initial direction? We can always add the new switch later on.
Hu, Ningxin (Gerrit)
Hu, Ningxin added 1 comment![]( "Open in Gerrit")
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Wang, Wei4Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
Hu, NingxinI have updated the doc of kWebNNOrtDumpModel. I think introducing a separate switch can be be confusing because it becomes complicated to use.
Aditya RastogiDumping ONNX model and EP specific model seem to be orthogonal to me. Not sure whether we should have them seperate. @aditya....@microsoft.com for comments.
Agree they are orthogonal concepts, but if we had two switches, I think it becomes a little more tricky to use (e.g., the new switch wouldn't work for CPU / DML, and the ONNX path seems like it would not be valid for OpenVINO based on the comments in the PR). Is it ok to start with this initial direction? We can always add the new switch later on.
Is it ok to start with this initial direction? We can always add the new switch later on.
Sounds good. Can we leave a TODO here? An EP may support dumping both ONNX model and IHV model in the future.
Wang, Wei4 (Gerrit)
Wang, Wei4 added 1 comment![]( "Open in Gerrit")
// `SetOptimizedModelFilePath` does not take effect for the OpenVINO EP.
// When OpenVINO is the first selected EP, dump the subgraphs via its own
// session config entry instead. The value is a directory path (UTF-8),Wang, Wei4Should we update doc of kWebNNOrtDumpModel? For OpenVINO EP, it dumps the OV IR models rather than ONNX models. Or consider introducing a separate switch, e.g. `webnn-ort-dump-ihv-ep-model`?
Hu, NingxinI have updated the doc of kWebNNOrtDumpModel. I think introducing a separate switch can be be confusing because it becomes complicated to use.
Aditya RastogiDumping ONNX model and EP specific model seem to be orthogonal to me. Not sure whether we should have them seperate. @aditya....@microsoft.com for comments.
Hu, NingxinAgree they are orthogonal concepts, but if we had two switches, I think it becomes a little more tricky to use (e.g., the new switch wouldn't work for CPU / DML, and the ONNX path seems like it would not be valid for OpenVINO based on the comments in the PR). Is it ok to start with this initial direction? We can always add the new switch later on.
Is it ok to start with this initial direction? We can always add the new switch later on.
Sounds good. Can we leave a TODO here? An EP may support dumping both ONNX model and IHV model in the future.
- Aditya Rastogi
- Hu, Ningxin
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hu, Ningxin (Gerrit)
Hu, Ningxin voted and added 1 comment![]( "Open in Gerrit")
- Aditya Rastogi
- Rafael Cintron
- Wang, Wei4
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 3 comments![]( "Open in Gerrit")
base::StringPrintf("model%d.onnx", dump_count++));The default branch generates unique files via the static counter ( model{N}.onnx ) whereas the OV branch passes only dump_directory. If multiple graphs/sessions dump in one process and the EP uses fixed names, dumps overwrite each other.
Is there a way to pass a specific file in the `model_dump_config_key` case?
base::StringPrintf("model%d.onnx", dump_count++));Nit: For a 64bit unsigned integer, you need to use `%"PRIu64"`. Consider using `base::StrCat` and `base::NumberToString` instead.
std::string_view ep_name = ort_api->EpDevice_EpName(registered_device);Can we pull `ep_name` out of the loop so we can avoid a second `EpDevice_EpName` call when we call `CreatebaseSessionOptions` below?
Related details
- Aditya Rastogi
- Wang, Wei4
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang, Wei4 (Gerrit)
Wang, Wei4 added 3 comments![]( "Open in Gerrit")
Nit: For a 64bit unsigned integer, you need to use `%"PRIu64"`. Consider using `base::StrCat` and `base::NumberToString` instead.
Thanks, done.
base::StringPrintf("model%d.onnx", dump_count++));The default branch generates unique files via the static counter ( model{N}.onnx ) whereas the OV branch passes only dump_directory. If multiple graphs/sessions dump in one process and the EP uses fixed names, dumps overwrite each other.
Is there a way to pass a specific file in the `model_dump_config_key` case?
The dump_subgraphs config value is interpreted strictly as a directory by the OpenVINO EP, so we can't pass a specific filename here: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/7ce14a41887a573ba0972e4ede923bddc49bbd07/plugin/ov_provider.cc#L48
However, OV EP generates unique filenames itself using a process-wide static plus the subgraph index and model stem: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/7ce14a41887a573ba0972e4ede923bddc49bbd07/plugin/ov_provider.cc#L624. This is the same uniqueness guarantee our default branch gets from the static dump_count. So passing just the directory is sufficient and dumps from multiple graphs/sessions in one process won't collide.
std::string_view ep_name = ort_api->EpDevice_EpName(registered_device);Can we pull `ep_name` out of the loop so we can avoid a second `EpDevice_EpName` call when we call `CreatebaseSessionOptions` below?
We can directly use `target_device.ep_name` when calling `CreatebaseSessionOptions `
Related details
- Aditya Rastogi
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 3 comments![]( "Open in Gerrit")
// TODO(crbug.com/530292678): kWebNNOrtDumpModel is used for dumping both the
// optimized ONNX model and the EP-specific IHV model. In the future, an EP
// may support dumping both simultaneously. When that happens, we should
// introduce a separate switch to allow users to control each dump target
// independently.```suggestion
// TODO(crbug.com/530292678): kWebNNOrtDumpModel is used for dumping either the
// optimized ONNX model or the EP-specific IHV model. In the future, an EP
// may support dumping both simultaneously. When that happens, we should
// introduce a separate switch to allow users to control each dump target
// independently.
```
// Some EPs (e.g., OpenVINO) don't support `SetOptimizedModelFilePath`.
// Instead, dump the subgraphs via their own session config entry.This comment makes it seem like `SetOptimizedModelFilePath` will be supported for OV EP in the future. Is this true? Seems like this is a bug with the OV EP that should be fixed in the future. EPs should strive to implement the ORT API first, then provide EP specific enhancements as an addon, not the other way around.
base::StringPrintf("model%d.onnx", dump_count++));The default branch generates unique files via the static counter ( model{N}.onnx ) whereas the OV branch passes only dump_directory. If multiple graphs/sessions dump in one process and the EP uses fixed names, dumps overwrite each other.
Is there a way to pass a specific file in the `model_dump_config_key` case?
The dump_subgraphs config value is interpreted strictly as a directory by the OpenVINO EP, so we can't pass a specific filename here: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/7ce14a41887a573ba0972e4ede923bddc49bbd07/plugin/ov_provider.cc#L48
However, OV EP generates unique filenames itself using a process-wide static plus the subgraph index and model stem: https://github.com/intel-innersource/frameworks.ai.onnxruntime.openvino-plugin-ep/blob/7ce14a41887a573ba0972e4ede923bddc49bbd07/plugin/ov_provider.cc#L624. This is the same uniqueness guarantee our default branch gets from the static dump_count. So passing just the directory is sufficient and dumps from multiple graphs/sessions in one process won't collide.
- Aditya Rastogi
- Wang, Wei4
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |