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

[Transformations] Fix vector subscript out of range in transformations #27180

Conversation

praasz
Copy link
Contributor

@praasz praasz commented Oct 22, 2024

Details:

  • Fix issue reported by MSVC Assertion failed: vector subscript out of range by skip accessing not existing parameters in RemoveMultiSubGraphOpDanglingParamsResults transformation.

Tickets:

RemoveMultiSubGraphOpDanglingParamsResults
@praasz praasz requested a review from a team as a code owner October 22, 2024 11:20
@github-actions github-actions bot added the category: transformations OpenVINO Runtime library - Transformations label Oct 22, 2024
@praasz praasz added this to the 2024.5 milestone Oct 23, 2024
@itikhono
Copy link
Contributor

why did this test start crashing? this is not a new test, what changed?

Build openvino on windows (VS2019, Debug build) and run ov_transformations_tests, the tests are crashed with the following error:
TransformationTestsF.RemoveIfDanglingParametersFromBodiesAndInputs

@praasz
Copy link
Contributor Author

praasz commented Oct 23, 2024

why did this test start crashing? this is not a new test, what changed?

Build openvino on windows (VS2019, Debug build) and run ov_transformations_tests, the tests are crashed with the following error:
TransformationTestsF.RemoveIfDanglingParametersFromBodiesAndInputs

MSVC in debug mode use additional assert which make additional check e.g. if index in range or check if comparators meets some requirements. Is not visible on gcc builds.

@itikhono
Copy link
Contributor

why did this test start crashing? this is not a new test, what changed?

Build openvino on windows (VS2019, Debug build) and run ov_transformations_tests, the tests are crashed with the following error:
TransformationTestsF.RemoveIfDanglingParametersFromBodiesAndInputs

MSVC in debug mode use additional assert which make additional check e.g. if index in range or check if comparators meets some requirements. Is not visible on gcc builds.

I'm trying to clarify why we didn't notice this problem earlier.
Did MSVC in debug mode always failed? MSVC in debug mode is not part of a pre or post commit, but part of some sort of extended validation, so that's why we didn't notice that?

@praasz
Copy link
Contributor Author

praasz commented Oct 23, 2024

why did this test start crashing? this is not a new test, what changed?

Build openvino on windows (VS2019, Debug build) and run ov_transformations_tests, the tests are crashed with the following error:
TransformationTestsF.RemoveIfDanglingParametersFromBodiesAndInputs

MSVC in debug mode use additional assert which make additional check e.g. if index in range or check if comparators meets some requirements. Is not visible on gcc builds.

I'm trying to clarify why we didn't notice this problem earlier. Did MSVC in debug mode always failed? MSVC in debug mode is not part of a pre or post commit, but part of some sort of extended validation, so that's why we didn't notice that?

Looks here is added and was not detected previously

@mryzhov
Copy link
Contributor

mryzhov commented Oct 23, 2024

why did this test start crashing? this is not a new test, what changed?

Build openvino on windows (VS2019, Debug build) and run ov_transformations_tests, the tests are crashed with the following error:
TransformationTestsF.RemoveIfDanglingParametersFromBodiesAndInputs

MSVC in debug mode use additional assert which make additional check e.g. if index in range or check if comparators meets some requirements. Is not visible on gcc builds.

I'm trying to clarify why we didn't notice this problem earlier. Did MSVC in debug mode always failed? MSVC in debug mode is not part of a pre or post commit, but part of some sort of extended validation, so that's why we didn't notice that?

Looks here is added and was not detected previously

We are going to include Win Debug checks in pre-commit scope, previously it was a a part of post-commit checks only and moreover it was built unproperly (as Release build), that is why issue has not been detected earlier

@praasz praasz requested a review from itikhono October 24, 2024 06:06
@itikhono
Copy link
Contributor

We agreed to merge this as a temporary solution.
The issue still exists in RemoveMultiSubGraphOpDanglingParamsResults and will be investigated later

@mryzhov mryzhov added this pull request to the merge queue Oct 24, 2024
Merged via the queue into openvinotoolkit:master with commit d96bd7d Oct 24, 2024
156 of 159 checks passed
@praasz praasz deleted the bug/msvc-debug-vector-subscript-out-of-range-in-test branch October 25, 2024 08:15
@rkazants
Copy link
Contributor

rkazants commented Oct 28, 2024

@praasz, can we have a test for this case in the precommit please? This problem affects several models from TF. See https://jira.devtools.intel.com/browse/CVS-155711

@AKochin, @rnugmanx, fyi

CuriousPanCake pushed a commit to CuriousPanCake/openvino that referenced this pull request Nov 6, 2024
openvinotoolkit#27180)

### Details:
- Fix issue reported by MSVC `Assertion failed: vector subscript out of
range` by skip accessing not existing parameters in
`RemoveMultiSubGraphOpDanglingParamsResults` transformation.

### Tickets:
 - CVS-155258
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: transformations OpenVINO Runtime library - Transformations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants