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

[RTTI] Make is_type dependent to OV dynamic cast selection #28858

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

[RTTI] Make is_type dependent to OV dynamic cast selection #28858

wants to merge 29 commits into from
+20 −13

Conversation

t-jankowski
Copy link
Contributor

Details:

  • Separate ov::is_type function template by function parameter type: pointer or std::shared_ptr.
  • Add dependency to ov dynamic cast selection into ov::is_type function templates.
  • Rename template and function parameters' names.

Tickets:

@t-jankowski
Make is_type conditional to OV dynamic cast selection
d50ef25 
Signed-off-by: Tomasz Jankowski <tomasz1.jankowski@intel.com>
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Feb 6, 2025
@t-jankowski t-jankowski requested a review from praasz February 6, 2025 18:16
praasz
praasz requested changes Feb 7, 2025
View reviewed changes
@github-actions github-actions bot added the category: extensions OpenVINO Extensibility Mechanism for custom operations label Feb 10, 2025
@t-jankowski
Move use_ov_dynamic_cast into new namespace ov::compile
a0e625d 
Signed-off-by: Tomasz Jankowski <tomasz1.jankowski@intel.com>
@t-jankowski t-jankowski requested a review from praasz February 10, 2025 12:53
@t-jankowski t-jankowski marked this pull request as ready for review February 10, 2025 12:54
@t-jankowski t-jankowski requested a review from a team as a code owner February 10, 2025 12:54
@t-jankowski t-jankowski changed the title POC [RTTI] Make is_type dependent to OV dynamic cast selection [RTTI] Make is_type dependent to OV dynamic cast selection Feb 10, 2025
praasz
praasz approved these changes Feb 11, 2025
View reviewed changes
@t-jankowski t-jankowski requested a review from a team as a code owner February 11, 2025 13:25
@t-jankowski t-jankowski requested review from itikhono and removed request for a team February 11, 2025 13:25
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Feb 11, 2025
@mlukasze
Copy link
Contributor

@itikhono please review

@mlukasze mlukasze requested a review from ilya-lavrenov March 7, 2025 05:44
@ilya-lavrenov
Copy link
Contributor

Is the issue on Jenkins caused by current changes?

openvino._pyopenvino.GeneralFailure: Check 'false' failed at src/frontends/onnx/frontend/src/frontend.cpp:164:
FrontEnd API failed with GeneralFailure:
OpenVINO does not support the following ONNX operations: custom_domain.CustomAdd

@t-jankowski
Copy link
Contributor Author

t-jankowski commented Mar 7, 2025 •
edited
Loading

Is the issue on Jenkins caused by current changes?

openvino._pyopenvino.GeneralFailure: Check 'false' failed at src/frontends/onnx/frontend/src/frontend.cpp:164:
FrontEnd API failed with GeneralFailure:
OpenVINO does not support the following ONNX operations: custom_domain.CustomAdd

I think so .. debugging locally.

@t-jankowski t-jankowski disabled auto-merge March 7, 2025 13:03
@t-jankowski t-jankowski requested review from a team as code owners March 10, 2025 11:51
@github-actions github-actions bot added category: ONNX FE OpenVINO ONNX FrontEnd category: extensions OpenVINO Extensibility Mechanism for custom operations labels Mar 10, 2025
@t-jankowski
Copy link
Contributor Author

Is the issue on Jenkins caused by current changes?

openvino._pyopenvino.GeneralFailure: Check 'false' failed at src/frontends/onnx/frontend/src/frontend.cpp:164:
FrontEnd API failed with GeneralFailure:
OpenVINO does not support the following ONNX operations: custom_domain.CustomAdd

@ilya-lavrenov After my recent changes with ov::frontend::onnx::ConversionExtension dtor wheel_macos_release check is ok.

ilya-lavrenov
ilya-lavrenov reviewed Mar 11, 2025
View reviewed changes
src/frontends/onnx/frontend/include/openvino/frontend/onnx/extension/conversion.hpp Outdated
@@ -11,9 +11,10 @@
namespace ov {
namespace frontend {
namespace onnx {
class ConversionExtension : public ConversionExtensionBase {
class ONNX_FRONTEND_API ConversionExtension : public ConversionExtensionBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have explicitly removed all exports from conversion extension classes here #28834 to avoid dependency on frontend.so in case of external extensions like openvino_tokenizers (before, they depended on TF FE because of TF FE conversion extension)

Why do we need to return it back?

Copy link
Contributor Author

@t-jankowski t-jankowski Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That was the only solution I found to fix failing macos wheel test. I'll check other possibilities not breaking deps.

@github-actions github-actions bot removed category: ONNX FE OpenVINO ONNX FrontEnd category: extensions OpenVINO Extensibility Mechanism for custom operations labels Mar 13, 2025
olpipi
olpipi reviewed Mar 20, 2025
View reviewed changes
src/core/include/openvino/core/type.hpp
@@ -81,20 +81,28 @@ namespace frontend {
class ConversionExtensionBase;
} // namespace frontend

namespace compile {
template <typename T>
constexpr bool use_ov_dynamic_cast() {
return std::is_base_of_v<ov::frontend::ConversionExtensionBase, T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

right now use_ov_dynamic_cast depends only on ConversionExtensionBase which is specific to frontends. Can this check be moved to frontends common part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd imply dependency of almost all the objects to frontend/common, mostly not needed but affecting build time.

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 left review comments

@praasz praasz praasz approved these changes

@olpipi olpipi olpipi approved these changes

@barnasm1 barnasm1 barnasm1 approved these changes

Assignees

@praasz praasz

Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings
Projects
None yet
Milestone
2025.1
Development

Successfully merging this pull request may close these issues.
7 participants
@t-jankowski @mlukasze @ilya-lavrenov @olpipi @praasz @barnasm1 @itikhono