Skip to content

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
myshevts merged 1 commit intoopenvinotoolkit:masterfrom
myshevts:fix_multi_priorities
Sep 17, 2020
Merged

Conversation

@myshevts
Copy link
Copy Markdown
Contributor

@myshevts myshevts commented Sep 15, 2020

@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
Copy Markdown
Contributor

ilya-lavrenov commented Sep 15, 2020

@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
Copy Markdown
Contributor

ilya-lavrenov commented Sep 15, 2020

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
Copy Markdown
Contributor Author

myshevts commented Sep 16, 2020

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
Copy Markdown
Contributor Author

myshevts commented Sep 16, 2020

Rebuild

@myshevts
Copy link
Copy Markdown
Contributor Author

myshevts commented Sep 16, 2020

rebuild

@ilya-lavrenov
Copy link
Copy Markdown
Contributor

ilya-lavrenov commented Sep 16, 2020

CI is red

@myshevts
Copy link
Copy Markdown
Contributor Author

myshevts commented Sep 16, 2020

CI is red

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

@myshevts
Copy link
Copy Markdown
Contributor Author

myshevts commented Sep 16, 2020

rebuild

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

myshevts commented Sep 16, 2020

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

2 more reviewers

@apankratovantonp apankratovantonp apankratovantonp left review comments

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

Reviewers whose approvals may not affect merge requirements

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