CVS-175736 - [OVEP] Optimize Stateful Path: use output-to-input strategy to get the pairs of KV name #845
Conversation
| // Licensed under the MIT License | ||
|
|
||
| #include "core/providers/openvino/ov_stateful_patch_utils.h" | ||
| #include "regex" |
There was a problem hiding this comment.
| #include "regex" | |
| #include <regex> |
| auto [key_value_output_names, extracted_patterns] = ExtractKVPatternsFromOutputs(model); | ||
| auto [key_value_input_names, not_kv_inputs] = ExtractInputKVTensors(model, extracted_patterns); | ||
|
|
||
| if (key_value_input_names.empty() || key_value_output_names.empty()) { |
There was a problem hiding this comment.
I know this line not something you added -- but can we add some more strict checking here? If all goes well, then key_value_output_names and key_value_output_names should be non-empty and have the same number of elements correct?
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the stateful path handling in OpenVINO by implementing a dynamic pattern extraction strategy to identify KV (key-value) pairs, replacing the previous hardcoded approach. The strategy extracts patterns from output tensors containing "present" and applies them to identify corresponding input tensors.
Key Changes:
- Introduced regex-based pattern extraction from output tensor names to dynamically identify KV pairs
- Added fallback mechanism using substring matching when pattern extraction yields no results
- Refactored
PatchStatefulDecoderto use the new pattern-based extraction functions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key_value_output_names.push_back(name); | ||
| std::smatch match; | ||
| if (std::regex_match(name, match, present_pattern)) { | ||
| // Extract the middle part (between "present_" and "_number") |
There was a problem hiding this comment.
Corrected spelling of 'tesnor' to 'tensor' in comment on line 2 of function documentation.
| return std::make_pair(key_value_input_names, not_kv_inputs); | ||
| } | ||
|
|
||
| std::set<std::string> found_kv_inputs; |
There was a problem hiding this comment.
The variable found_kv_inputs is declared but never used. Consider removing it or implementing the intended logic.
| std::set<std::string> found_kv_inputs; |
| if (name.starts_with("present")) { | ||
| key_value_output_names.push_back(name); | ||
| std::smatch match; | ||
| if (std::regex_match(name, match, present_pattern)) { |
There was a problem hiding this comment.
Do we need a regular expression here? It seems like it can be simplified to a regular string manipulation: we already know that the string starts with "present" and we can find where the last underscore is, gathering the substring.
This is also a micro-optimization in terms of performance, since C++ regexps are compiled at runtime and are known for their bad performance.
| break; | ||
| } | ||
| } | ||
| if (found) break; |
There was a problem hiding this comment.
This logic contradicts the comment above. If we find a pattern in the name, we won't check other names. Is that an expected behavior?
There was a problem hiding this comment.
yes, it is expected behavior. I rephrased the comment.
| auto [key_value_output_names, extracted_patterns] = ExtractKVPatternsFromOutputs(model); | ||
| auto [key_value_input_names, not_kv_inputs] = ExtractInputKVTensors(model, extracted_patterns); | ||
|
|
||
| std::cout << key_value_input_names.size() << ";" << key_value_output_names.size() << std::endl; |
There was a problem hiding this comment.
looks like a debug statement here.
| } | ||
|
|
||
| if (key_value_input_names.size() != key_value_output_names.size()) { | ||
| std::cout << "found different sizes btween key_value_input_names and key_value_output_names, they couldn't be paired" << std::endl; |
There was a problem hiding this comment.
I think that this one should be a runtime exception of some sort. I don't think we'd ever want to hit this state, return, and have the rest of the stateful flow continue on.
|
|
||
| std::cout << key_value_input_names.size() << ";" << key_value_output_names.size() << std::endl; | ||
| if (key_value_input_names.empty() || key_value_output_names.empty()) { | ||
| std::cout << "no key_value_input_names or key_value_output_names found" << std::endl; |
There was a problem hiding this comment.
Same for here as below -- I think there should be a runtime exception thrown here. I don't think we'd ever intend for the stateful flow to get enabled, and not identify pairs of tensors to perform a make_stateful transformation on.
3 participants
Description
It is hardcoded for the path of identifying KV pairs. This PR is to make it general to fit most of the cases. It is a follow up of #821
Here is the strategy:
key_***_%d; value_***_%dfrom output tesnorJira Ticket :
https://jira.devtools.intel.com/browse/CVS-175736
Go to new ABI?
Yes