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

Fixing incorrect (recent?) refactoring: reverting devicePriorities to be vector and respect the order, as opposed to the unordered_map that effectively ignores the priorities #2249

Merged
merged 1 commit into from
Sep 17, 2020
Merged

Fixing incorrect (recent?) refactoring: reverting devicePriorities to be vector and respect the order, as opposed to the unordered_map that effectively ignores the priorities #2249

merged 1 commit into from
Sep 17, 2020

Conversation

myshevts
Copy link
Contributor

@apankratovantonp , when you have replaced the device priorities to be unordered_map and not the vector, the priorities were essentially ignored 🥇 . I strongly suggest you do a code review with me before doing refactoring like this.

@myshevts myshevts changed the title Fixing incorrect refactoring by apankratov: reverting devicePriorities to be vector and respect the order, as opposed to the that introduced the unordered_map that effectively ignores the priorities Fixing incorrect refactoring by apankratov: reverting devicePriorities to be vector and respect the order, as opposed to the unordered_map that effectively ignores the priorities Sep 15, 2020
@ilya-lavrenov
Copy link
Contributor

@myshevts please, submit the same changes to release 2021.1

@openvino-pushbot openvino-pushbot added the category: MULTI OpenVINO MULTI device plugin label Sep 15, 2020
@myshevts myshevts mentioned this pull request Sep 15, 2020
Merged
@myshevts myshevts force-pushed the fix_multi_priorities branch from 2a1e552 to cb998e0 Compare September 15, 2020 16:15
@myshevts myshevts changed the title Fixing incorrect refactoring by apankratov: reverting devicePriorities to be vector and respect the order, as opposed to the unordered_map that effectively ignores the priorities Fixing incorrect (recent?) refactoring: reverting devicePriorities to be vector and respect the order, as opposed to the unordered_map that effectively ignores the priorities Sep 15, 2020
@ilya-lavrenov
Copy link
Contributor

I strongly suggest you do a code review with me before doing refactoring like this.

code review is done. All PRs have +1 from you (see gitlab).
In order to avoid such situations, could you please add a test for this behavior?

@ilya-lavrenov ilya-lavrenov added the pr: needs tests PR needs tests updating label Sep 15, 2020
@myshevts
Reverting devicePriorities to be vector and respect the order, as opp…
aa1d92e 
…osed to the incorrect (re

cent?) refactoring that introduced the unordered_map that effectively ignores the priorities
@myshevts myshevts force-pushed the fix_multi_priorities branch from cb998e0 to aa1d92e Compare September 16, 2020 07:46
@myshevts
Copy link
Contributor Author

I strongly suggest you do a code review with me before doing refactoring like this.

code review is done. All PRs have +1 from you (see gitlab).
In order to avoid such situations, could you please add a test for this behavior?

good point: https://jira.devtools.intel.com/browse/CVS-38404. The only complexity that we will need mocking 2 or more devices to simulate the things

@myshevts
Copy link
Contributor Author

Rebuild

@myshevts
Copy link
Contributor Author

rebuild

@ilya-lavrenov
Copy link
Contributor

CI is red

@myshevts
Copy link
Contributor Author

CI is red

this is Myriad sticks that are down, will need to "rebuild" again

@myshevts
Copy link
Contributor Author

rebuild

@ilya-lavrenov ilya-lavrenov self-assigned this Sep 16, 2020
@myshevts
Copy link
Contributor Author

rebuild

@myshevts myshevts merged commit 6b9376e into openvinotoolkit:master Sep 17, 2020
@myshevts myshevts deleted the fix_multi_priorities branch December 14, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apankratovantonp apankratovantonp apankratovantonp left review comments

@ilya-lavrenov ilya-lavrenov ilya-lavrenov approved these changes

Assignees

@ilya-lavrenov ilya-lavrenov

Labels
category: MULTI OpenVINO MULTI device plugin pr: needs tests PR needs tests updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@myshevts @ilya-lavrenov @apankratovantonp @openvino-pushbot