Skip to content

Conversation

@chemwolf6922
Copy link
Contributor

Why is this change made

GenAI added the ep device selection APIs for selecting ep devices according to the given ep name, device type, device vendor id and device id. However, the legacy OpenVINO specific device filtering logic has a higher priority than the new API. Causing the APIs ineffective when the "device_type" is specified in a OpenVINO genai_config.json.

What changed

This PR adjusts the two method's priority. So the new API can override what's provided in the config file.

// for OpenVINO, fallback to "device_type" in provider_options if no device filtering is specified
std::optional<std::string> config_ov_device_type = std::nullopt;
if (provider_options.name == "OpenVINO") {
if (provider_options.name == "OpenVINO" &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make the change at config parsing time? So, we update the provider_options.device_filtering_options after parsing the config. And we use that same option for setting the session options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this would be a cleaner solution. But we need to deprecate the following Intel specific behavior to do so:
As you can see from the code, Intel compares the "device_type" to the "ov_device" in their ep metadata for device selection. This behaves the same if "device_type" is always CPU, GPU or NPU (or any other standard device type in the future). However, Intel also supports setting the ov_device to something like "GPU1" in some versions of their EPs. And use that to select from multiple devices.
IMHO, Intel's old approach is not a very good one since you cannot generate a model with the correct setting if you don't know the CPUx name ahead. And it can be fully replaced by the device filtering APIs. If we can accept deprecation of this behavior, I'll happily remove the OpenVINO spaghetti here and move it forward to config parsing.

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