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.

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

[PD FE] loading weight from ov::tensor #20044

Merged
merged 7 commits into from
Oct 31, 2023

Conversation

xczhai
Copy link
Contributor

@xczhai xczhai commented Sep 26, 2023

@xczhai xczhai requested a review from a team as a code owner September 26, 2023 07:52
@github-actions github-actions bot added the category: PDPD FE OpenVINO PaddlePaddle FrontEnd label Sep 26, 2023
@xczhai xczhai changed the title Pd fix loading memory [PD FE] loading weight from ov::tensor Sep 26, 2023
@xczhai xczhai force-pushed the pd_fix_loading_memory branch from ee9ff4f to 2db9049 Compare September 26, 2023 07:58
OPENVINO_SUPPRESS_DEPRECATED_START
std::istream* variant_to_stream_ptr(const ov::Any& variant,
std::ifstream& ext_ifstream,
std::stringstream& ext_sstream) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge std::ifstream& ext_ifstream and std::stringstream& ext_sstream into together, because we can see that each branch only uses one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we merge std::ifstream& ext_ifstream and std::stringstream& ext_sstream into together, because we can see that each branch only uses one of them.

@riverlijunjie I simplify the function variant_to_stream_ptr. Please review when you are free.

@ilyachur ilyachur added the pr: needs tests PR needs tests updating label Oct 9, 2023
@xczhai xczhai force-pushed the pd_fix_loading_memory branch 3 times, most recently from b862128 to c7eba6f Compare October 20, 2023 07:50
@xczhai xczhai force-pushed the pd_fix_loading_memory branch from 9dbd1be to cb67ba3 Compare October 24, 2023 02:33
@xczhai xczhai force-pushed the pd_fix_loading_memory branch from cb67ba3 to f5f9072 Compare October 24, 2023 02:39
// PDPD API ParseFromIstream always deconstructs the context in model stream.
// So, make a copy for variants[0] to avoid breaking the context in variants[0].
const auto p_model_stream = variants[0].as<std::istream*>();
std::istream copy_model_stream(p_model_stream->rdbuf());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another issue fixing? Do we have other idea to avoid context desconstruction except for doing copy?

Copy link
Contributor Author

@xczhai xczhai Oct 25, 2023

Choose a reason for hiding this comment

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

This is another issue fixing? Do we have other idea to avoid context desconstruction except for doing copy?

@riverlijunjie No, it is not a new bug. It is a related-bug when loading from memory and no test case to cover.
Actually, it cannot be avoided but don't worry too much. The copy behavior is just about model file instead of weight file so the negative impact is little.

@xczhai xczhai requested a review from riverlijunjie October 25, 2023 08:42
Copy link
Contributor

@riverlijunjie riverlijunjie left a comment

Choose a reason for hiding this comment

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

LGTM

@ceciliapeng2011 ceciliapeng2011 self-assigned this Oct 27, 2023
@@ -135,22 +135,32 @@ bool normalize_framework_node(const std::shared_ptr<FrameworkNode>& node,
return true;
}

std::istream* variant_to_stream_ptr(const ov::Any& variant, std::ifstream& ext_stream) {
OPENVINO_SUPPRESS_DEPRECATED_START
std::istream* variant_to_stream_ptr(const ov::Any& variant, std::fstream& fs, std::stringstream& ss) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use fstream to handle case of AlignedBuffer? Any particular reason to use stringstream?

Copy link
Contributor Author

@xczhai xczhai Oct 31, 2023

Choose a reason for hiding this comment

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

Maybe not. @ceciliapeng2011

  • fstream should be open before use but no file can be open.
  • The return type is istream so we should return a derived type pointer. AFAIK, stringstream as a derived type can be filled with an internal buffer.

@ceciliapeng2011 ceciliapeng2011 removed the pr: needs tests PR needs tests updating label Oct 31, 2023
@ceciliapeng2011 ceciliapeng2011 merged commit 0076f7f into openvinotoolkit:master Oct 31, 2023
36 checks passed
alvoron pushed a commit to alvoron/openvino that referenced this pull request Nov 6, 2023
* fix paddle load model from memory

* fix coding style

* ignore the deprecated api

* fix a istream bug; add test case

* simplify func variant_to_stream_ptr

* restore the previous impl for less memory affect

* fix memory leak
@sdcb
Copy link

sdcb commented Nov 10, 2023

@ceciliapeng2011 @riverlijunjie
We should also include this into 2023.2 release since #19931 is also included.

allnes pushed a commit to allnes/openvino that referenced this pull request Nov 23, 2023
* fix paddle load model from memory

* fix coding style

* ignore the deprecated api

* fix a istream bug; add test case

* simplify func variant_to_stream_ptr

* restore the previous impl for less memory affect

* fix memory leak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: PDPD FE OpenVINO PaddlePaddle FrontEnd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants