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

[core] Remove custom ov::optional #28949

Merged
merged 2 commits into from
Feb 20, 2025
Merged

[core] Remove custom ov::optional #28949

merged 2 commits into from
Feb 20, 2025

Conversation

oToToT
Copy link
Contributor

@oToToT oToToT commented Feb 12, 2025 •
edited by praasz
Loading

Description:

As CPP17 is the default standard of OpenVINO, there is no need to use the self-implemented optional struct.

Note: discussed in #28942.

Related PRs:

@oToToT oToToT requested review from a team as code owners February 12, 2025 09:03
@oToToT oToToT mentioned this pull request Feb 12, 2025
Draft
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin labels Feb 12, 2025
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Feb 12, 2025
@praasz praasz self-assigned this Feb 12, 2025
@praasz
Copy link
Contributor

praasz commented Feb 12, 2025

build_jenkins

@praasz praasz changed the title Remove ov::optional [core] Remove custom ov::optional Feb 12, 2025
@oToToT oToToT requested a review from a team as a code owner February 12, 2025 09:41
@oToToT oToToT requested review from itikhono and removed request for a team February 12, 2025 09:41
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Feb 12, 2025
@oToToT oToToT force-pushed the remove-optional branch 3 times, most recently from 13b0e03 to 2912661 Compare February 13, 2025 03:25
@mlukasze
Copy link
Contributor

build_jenkins

@oToToT
Copy link
Contributor Author

oToToT commented Feb 13, 2025

Fixed the build on Ubuntu24.04.

Found that the bool operator of ov::optional is not marked as explicit while std::optional is.

@mlukasze
Copy link
Contributor

build_jenkins

@oToToT
Copy link
Contributor Author

oToToT commented Feb 15, 2025 •
edited
Loading

can't see jenkins failures logs. do i need to do anything?

@praasz
Copy link
Contributor

praasz commented Feb 17, 2025

can't see jenkins failures logs. do i need to do anything?

There are just build error which should be visible during standard local build of project.

 In file included from openvino/src/core/src/op/segment_max.cpp:10:

[2025-02-14T05:54:40.407Z] openvino/src/core/shape_inference/include/segment_max_shape_inference.hpp: In function ‘std::vector<UserT> ov::op::v16::shape_infer(const ov::op::v16::SegmentMax*, const std::vector<TShape>&, const ov::ITensorAccessor&)’:

[2025-02-14T05:54:40.408Z] openvino/src/core/shape_inference/include/segment_max_shape_inference.hpp:54:60: error: ‘optional’ is not a member of ‘ov’; did you mean ‘std::optional’?

@oToToT
Copy link
Contributor Author

oToToT commented Feb 17, 2025

Thanks for sharing the error message. It seems like these are some file not in my forked branch but in the latest trunk.

I've rebased and updated those files to reflect the removal of ov::optional.

@praasz
Copy link
Contributor

praasz commented Feb 17, 2025

build_jenkins

@oToToT oToToT force-pushed the remove-optional branch 3 times, most recently from 6610399 to 73c5be9 Compare February 17, 2025 08:53
@mlukasze
Copy link
Contributor

@itikhono please take a look, it's ready to merge, but would prefer to have your approval as well

dmitry-gorokhov
dmitry-gorokhov approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@dmitry-gorokhov dmitry-gorokhov left a comment

Choose a reason for hiding this comment

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

CPU part LGTM

@itikhono itikhono added this pull request to the merge queue Feb 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2025
@oToToT
Copy link
Contributor Author

oToToT commented Feb 19, 2025

rebased and fixed the merge conflict

@mlukasze
Copy link
Contributor

build_jenkins

@mlukasze mlukasze enabled auto-merge February 19, 2025 05:52
@mlukasze
Copy link
Contributor

image

auto-merge was automatically disabled February 19, 2025 06:17

Head branch was pushed to by a user without write access

@oToToT
Copy link
Contributor Author

oToToT commented Feb 19, 2025

image

oops. apologize for that. It should be fixed now.

I've just built the code myself again to check it now.

@mlukasze
Copy link
Contributor

build_jenkins

@mlukasze
Copy link
Contributor

CI reported problem with clang-format, but that one is on us, do not change anything for now, please.
We will rebase and restart CI as soon as problem will be solved by our teams

@oToToT
[core] Remove custom ov::optional
2ffde0b 
As CPP17 is the default standard of OpenVINO, there is no need to use
the custom ov::optional struct anymore.

Note that the `bool` operator of std::optional is marked as `explicit`
while the original implementation is not.
@oToToT
Copy link
Contributor Author

oToToT commented Feb 19, 2025 •
edited
Loading

2ffde0b should be exactly the same as 683cfc2 (except a rebase). I apologize I didn't see the comment earlier before re-commit things.

@mlukasze
Copy link
Contributor

no problem, as rebase is done I will run CI as soon as we will have confirmation that problem has been solved.
stay tuned

@praasz
Copy link
Contributor

praasz commented Feb 19, 2025

build_jenkins

@praasz praasz enabled auto-merge February 19, 2025 08:08
@mlukasze
Copy link
Contributor

build_jenkins

@praasz praasz added this pull request to the merge queue Feb 19, 2025
Merged via the queue into openvinotoolkit:master with commit d3cdfe8 Feb 20, 2025
186 of 187 checks passed
@oToToT oToToT deleted the remove-optional branch February 20, 2025 02:37
@mlukasze mlukasze added this to the 2025.1 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-gorokhov dmitry-gorokhov dmitry-gorokhov approved these changes

@itikhono itikhono itikhono approved these changes

@praasz praasz praasz approved these changes

Assignees

@praasz praasz

Labels
category: Core OpenVINO Core (aka ngraph) category: CPU OpenVINO CPU plugin category: transformations OpenVINO Runtime library - Transformations ExternalPR External contributor
Projects
None yet
Milestone
2025.1
Development

Successfully merging this pull request may close these issues.
6 participants
@oToToT @praasz @mlukasze @dmitry-gorokhov @itikhono @sys-openvino-ci