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

Move api set_element_type() & set_tensor_type() from public to private #22645

Conversation

zhaixuejun1993
Copy link
Contributor

@zhaixuejun1993 zhaixuejun1993 commented Feb 4, 2024

Details:

  • Move set_element_type() from public func to private
  • Move set_tensor_type() from public func to private

Tickets:

  • ticket-id

… public to private

Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993 zhaixuejun1993 requested review from a team as code owners February 4, 2024 04:22
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common category: CPP API OpenVINO CPP API bindings labels Feb 4, 2024
@zhaixuejun1993
Copy link
Contributor Author

@ilya-lavrenov @vurusovs if this solution is fine, I will clean the "deprecated" lable

Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@zhaixuejun1993 zhaixuejun1993 requested a review from a team as a code owner February 5, 2024 02:15
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Feb 5, 2024
@zhaixuejun1993 zhaixuejun1993 changed the title [Deprecated API] Move api set_element_type() & set_tensor_type() from public to private Move api set_element_type() & set_tensor_type() from public to private Feb 5, 2024
Copy link
Contributor

@jane-intel jane-intel left a comment

Choose a reason for hiding this comment

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

PR implements the idea well. Thank you!
Let's see what @ilya-lavrenov and @vurusovs think of this

@p-durandin p-durandin added this pull request to the merge queue Feb 5, 2024
@ilya-lavrenov ilya-lavrenov removed this pull request from the merge queue due to a manual request Feb 5, 2024
This reverts commit bee4417.
…e() from public to private"

This reverts commit 4b3acb2.
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
bk/tensor.cpp Outdated Show resolved Hide resolved
Comment on lines 44 to 47
OPENVINO_API void set_element_type(Tensor& tensor, const element::Type& elemenet_type);

// To change Tensor type please change the Parameter type.
OPENVINO_API void set_tensor_type(Tensor& tensor, const element::Type& element_type, const PartialShape& pshape);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is OPENVINO_API required? It makes functions public for users, but original intention was to hide it

Copy link
Contributor Author

@zhaixuejun1993 zhaixuejun1993 Feb 7, 2024

Choose a reason for hiding this comment

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

@vurusovs Without "OPENVINO_API" (which will not export the func) will cause some linker error, as following:
image

I also tried to move the implement of "remember_input_data_types()" & "restore_input_data_types()" from hpp to cpp, but get the same error. https://github.com/zhaixuejun1993/openvino/blob/1cf81e8400680eb49ce9245ea4ccf85216987b60/src/common/transformations/include/ov_ops/type_relaxed.hpp#L83-L125

I don't know why for "class ov::op::TypeRelaxedBase" & "class ov::op::TemporaryReplaceOutputType" we need to put the implementation in hpp.

Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
Signed-off-by: Zhai, Xuejun <xuejun.zhai@intel.com>
@vurusovs
Copy link
Contributor

vurusovs commented Feb 8, 2024

Will be merged in scope of #22722

@vurusovs vurusovs closed this Feb 8, 2024
@zhaixuejun1993 zhaixuejun1993 deleted the xuejun/set_element_type_private branch March 26, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: IE Tests OpenVINO Test: plugins and common category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants