Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Force default locale for cache model write/read #24431

Merged
merged 10 commits into from May 17, 2024
Merged

Force default locale for cache model write/read #24431

merged 10 commits into from May 17, 2024

Conversation

zhaixuejun1993
Copy link
Contributor

@zhaixuejun1993 zhaixuejun1993 commented May 9, 2024 •
edited by peterchen-intel

Details:

  • Check the value of setlocale for export/import, if different with "C" will set to "C" and record the original value, after export/import done will reset to the original.
  • Fix the error caused by pugixml library with The setlocale function installs the specified system locale or its portion as the new C locale. different C may return unexpected results with setlocal()

Tickets:

@zhaixuejun1993
Fix the error caused by pugixml library with different C may return u…
e394dc8 
…nexpected results with setlocal()

Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993 zhaixuejun1993 requested a review from a team as a code owner May 9, 2024 07:08
@github-actions github-actions bot added the category: inference OpenVINO Runtime library - Inference label May 9, 2024
@riverlijunjie
Copy link
Contributor

@ilya-lavrenov could you check whether this fixing is reasonable? Thanks!

@ilya-lavrenov ilya-lavrenov added this to the 2024.2 milestone May 9, 2024
@ilya-lavrenov ilya-lavrenov added the pr: needs tests PR needs tests updating label May 9, 2024
@zhaixuejun1993
Fix review comments & add test
e4b976b 
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993
Fix review comment
d8a72f9 
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993
Fix review comment
adbad84 
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@ilya-lavrenov
Copy link
Contributor

@RyanMetcalfeInt8
Could you please check whether the fix helps in your case?

@ilya-lavrenov ilya-lavrenov removed the pr: needs tests PR needs tests updating label May 11, 2024
riverlijunjie
riverlijunjie reviewed May 12, 2024
View reviewed changes
src/inference/src/cache_manager.hpp Outdated Show resolved Hide resolved
src/inference/tests/functional/local_test.cpp Outdated Show resolved Hide resolved
src/inference/tests/functional/local_test.cpp Outdated Show resolved Hide resolved
@peterchen-intel peterchen-intel changed the title Fix the error caused by pugixml library with The setlocale function installs the specified system locale or its portion as the new C locale. different C may return unexpected results with setlocal() Force C locale for cache model write/read May 12, 2024
@zhaixuejun1993
Fix review comments
f31c0da 
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993
Merge branch 'master' into xuejun/fix_env_C
0924c59
@peterchen-intel peterchen-intel changed the title Force C locale for cache model write/read Force default locale for cache model write/read May 13, 2024
@RyanMetcalfeInt8
Copy link

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

@zhaixuejun1993
Copy link
Contributor Author

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

The CI build is enough, I think.

@RyanMetcalfeInt8
Copy link

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

The CI build is enough, I think.

Can you point me to where I can download the CI build? I was trying to find a link to artifacts from GitHub actions, but I couldn't seem to find it..

@zhaixuejun1993
Copy link
Contributor Author

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

The CI build is enough, I think.

Can you point me to where I can download the CI build? I was trying to find a link to artifacts from GitHub actions, but I couldn't seem to find it..

http://ov-share-03.iotg.sclab.intel.com/volatile/openvino_ci/private_builds/dldt/master/pre_commit/0924c59be2edba83c088890a5a03e5b3712b2e15/private_linux_ubuntu_20_04_release/cpack/

@ilya-lavrenov
Copy link
Contributor

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

The CI build is enough, I think.

Can you point me to where I can download the CI build? I was trying to find a link to artifacts from GitHub actions, but I couldn't seem to find it..

image

@RyanMetcalfeInt8
Copy link

@RyanMetcalfeInt8 Could you please check whether the fix helps in your case?

Do I need to build OpenVINO in order to try this, or can I grab a CI build?

The CI build is enough, I think.

Can you point me to where I can download the CI build? I was trying to find a link to artifacts from GitHub actions, but I couldn't seem to find it..

image

Thanks, I have verified that this PR resolves the locale issue.

@zhaixuejun1993
Fix review comments
c847336 
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@ilya-lavrenov ilya-lavrenov added this pull request to the merge queue May 17, 2024
Merged via the queue into openvinotoolkit:master with commit 7e64e41 May 17, 2024
115 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilya-lavrenov ilya-lavrenov ilya-lavrenov approved these changes

@riverlijunjie riverlijunjie riverlijunjie approved these changes

@peterchen-intel peterchen-intel Awaiting requested review from peterchen-intel

@RyanMetcalfeInt8 RyanMetcalfeInt8 Awaiting requested review from RyanMetcalfeInt8

Assignees

@ilya-lavrenov ilya-lavrenov

@riverlijunjie riverlijunjie

Labels
category: inference OpenVINO Runtime library - Inference Code Freeze
Projects
None yet
Milestone
2024.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zhaixuejun1993 @riverlijunjie @ilya-lavrenov @RyanMetcalfeInt8 @peterchen-intel