Added a new test to check whether some transformations is applied during conversion or completion#1651
Added a new test to check whether some transformations is applied during conversion or completion#1651alien-cyber wants to merge 3 commits intohuggingface:mainfrom
Conversation
…as been applied during conversion or compilation of a particular architecture
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
@popovaan @rkazants @alien-cyber
Hi !
I had a few suggestions and potential issues:
- The test currently captures
stdout, but OpenVINO logs are emitted tostderr, so this may miss the relevant transformation logs. - Setting
OV_ENABLE_PROFILE_PASSat the top level leaks into the global test environment it seems sufficient to keep it scoped inside the subprocess. - The subprocess call may be expensive since it can trigger model export for each test case. Adding a
cache_dir(or similar reuse strategy) could help reduce overhead. - The substring matching logic (
key in a) could lead to false positives (e.g.,MatMulmatchingMatMulFusionExtra). Exact matching would be safer.
Overall, the structure and idea of the test look great — addressing these points should make it more robust and CI-friendly.
| import sys | ||
| import unittest | ||
|
|
||
| os.environ["OV_ENABLE_PROFILE_PASS"] = "1" |
There was a problem hiding this comment.
i think setting OV_ENABLE_PROFILE_PASS at the top level might unintentionally leak into the global test environment . This variable is already being set inside the subprocess - line 49 . we can safely remove this line .
| text=True, | ||
| ) | ||
|
|
||
| return result.stdout |
There was a problem hiding this comment.
I think this should use stderr instead of stdout since OpenVINO logs are emitted to stderr
even function name says capture stderr
- return result.stdout
+ return result.stderr
| ARCH_TO_EXPECTED_TRANSFORMATIONS = _load_expected_transformations(_CONFIG_PATH) | ||
|
|
||
|
|
||
| def _capture_stderr_during(model_id, OPENVINO_DEVICE, trust_remote_code): |
There was a problem hiding this comment.
I think the subprocess code here may be quite expensive since it invokes OVModelForCausalLM.from_pretrained(...) inside the string, which can trigger model export on every test run.
| compile=True, | ||
| device="{OPENVINO_DEVICE}", | ||
| trust_remote_code={trust_remote_code}, | ||
| ) |
There was a problem hiding this comment.
can add this param
cache_dir="./ov_cache"
we could pass a cache_dir so that models are reused across runs instead of being re-exported each time.
This should help make the test more scalable.
| remaining = {normalize(w): w for w in words} | ||
|
|
||
| for key in list(remaining.keys()): | ||
| if any(key in a for a in applied_norm): |
There was a problem hiding this comment.
I think substring matching here could cause false positives like MatMul matching MatMulFusionExtra
Using exact matching like if key in applied_norm: would be safer.
There was a problem hiding this comment.
Hi @MissLostCodes
Thanks for the analysis.
- Now the string matching function is Fixed.
- stderr and stdout are captured in the same place.
- unnecessary OV_ENABLE_PROFILE_PASS is removed
Regarding cache_dir="./ov_cache", I believe it is not necessary in this context, as it mainly changes the default directory used for caching model files. The default Hugging Face cache should already handle reuse of downloaded model configuration.
This checks whether a specific set of transformations has been applied during conversion or compilation for particular architectures
expected_transformations.txtget_close_matcheslibrary which does fuzzy search and returns the possible matchesCloses #1645
Please review and let me know if any further improvements are needed.