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

Add multi-threading option to ModelTransform and SAMBboxToInstanceMask #1145

Conversation

vinnamkim
Copy link
Contributor

@vinnamkim vinnamkim commented Sep 11, 2023

Summary

  • Add multi-threading option (num_workers > 0) to ModelTransform and SAMBboxToInstanceMask.
  • It is required if the model launcher can take multiple requests at the same time and have high throughput.

How to test

Added some tests for this change.

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added the description of my changes into CHANGELOG.​
  • I have updated the documentation accordingly

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2023 Intel Corporation
#
# SPDX-License-Identifier: MIT

Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@vinnamkim vinnamkim marked this pull request as ready for review September 11, 2023 09:50
@vinnamkim vinnamkim requested review from a team as code owners September 11, 2023 09:50
@vinnamkim vinnamkim requested review from sooahleex and removed request for a team September 11, 2023 09:50
@vinnamkim vinnamkim changed the base branch from develop to releases/1.5.0 September 11, 2023 09:50
@vinnamkim vinnamkim added this to the 1.5.0 milestone Sep 11, 2023
@vinnamkim vinnamkim added the ENHANCE Enhancement of existing features label Sep 11, 2023
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 92.70% and project coverage change: +0.06% 🎉

Comparison is base (8b97641) 79.92% compared to head (865fb47) 79.99%.

Additional details and impacted files
@@                Coverage Diff                 @@
##           releases/1.5.0    #1145      +/-   ##
==================================================
+ Coverage           79.92%   79.99%   +0.06%     
==================================================
  Files                 265      266       +1     
  Lines               29895    29967      +72     
  Branches             5889     5901      +12     
==================================================
+ Hits                23895    23971      +76     
+ Misses               4641     4637       -4     
  Partials             1359     1359              
Flag Coverage Δ
macos-11_Python-3.8 79.10% <92.70%> (+0.05%) ⬆️
ubuntu-20.04_Python-3.8 79.97% <92.70%> (+0.04%) ⬆️
windows-2019_Python-3.8 79.91% <92.70%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/datumaro/components/dataset.py 81.91% <ø> (ø)
src/datumaro/components/hl_ops/__init__.py 56.86% <ø> (ø)
...tumaro/plugins/sam_transforms/bbox_to_inst_mask.py 85.71% <76.47%> (-8.58%) ⬇️
src/datumaro/components/transformer.py 90.52% <92.59%> (-0.50%) ⬇️
src/datumaro/util/multi_procs_util.py 97.87% <97.87%> (ø)
...c/datumaro/plugins/missing_annotation_detection.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vinnamkim vinnamkim changed the base branch from releases/1.5.0 to develop September 12, 2023 05:04
@vinnamkim vinnamkim changed the base branch from develop to releases/1.5.0 September 12, 2023 05:06
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Copy link
Contributor

@sooahleex sooahleex left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@vinnamkim vinnamkim merged commit 0064504 into openvinotoolkit:releases/1.5.0 Sep 12, 2023
6 checks passed
vinnamkim added a commit that referenced this pull request Sep 13, 2023
- One of the tests added in #1145 is flaky:
https://github.com/openvinotoolkit/datumaro/actions/runs/6156803415/job/16706221640
```console
=========================== short test summary info ============================
FAILED tests/unit/test_util.py::MultiProcUtilTest::test_raise_exception_in_main_thread
= 1 failed, 1493 passed, 38 skipped, 2 xfailed, 48148 warnings in 407.34s (0:06:47) =
tests-py38-darwin: exit 1 (462.14 seconds) /Users/runner/work/datumaro/datumaro> python -m pytest -v --csv=/Users/runner/work/datumaro/datumaro/.tox/results-tests-py38-darwin.csv tests/unit --cov --cov-report=xml pid=4536
.pkg: _exit> python /Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pyproject_api/_backend.py True setuptools.build_meta
  tests-py38-darwin: FAIL code 1 (793.18=setup[331.04]+cmd[462.14] seconds)
  evaluation failed :( (803.78 seconds)
```
- This is because `join_timeout` is too short, so that the main thread
tries to assert the error logs before they are created.
- To fix it, set `join_timeout=None` to wait it infinitely until the
producer thread terminates.

Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
@vinnamkim vinnamkim mentioned this pull request Sep 14, 2023
6 tasks
yunchu pushed a commit that referenced this pull request Sep 15, 2023
#1145)

- Add multi-threading option (`num_workers > 0`) to `ModelTransform` and
`SAMBboxToInstanceMask`.
- It is required if the model launcher can take multiple requests at the
same time and have high throughput.

Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
yunchu pushed a commit that referenced this pull request Sep 15, 2023
- One of the tests added in #1145 is flaky:
https://github.com/openvinotoolkit/datumaro/actions/runs/6156803415/job/16706221640
```console
=========================== short test summary info ============================
FAILED tests/unit/test_util.py::MultiProcUtilTest::test_raise_exception_in_main_thread
= 1 failed, 1493 passed, 38 skipped, 2 xfailed, 48148 warnings in 407.34s (0:06:47) =
tests-py38-darwin: exit 1 (462.14 seconds) /Users/runner/work/datumaro/datumaro> python -m pytest -v --csv=/Users/runner/work/datumaro/datumaro/.tox/results-tests-py38-darwin.csv tests/unit --cov --cov-report=xml pid=4536
.pkg: _exit> python /Users/runner/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pyproject_api/_backend.py True setuptools.build_meta
  tests-py38-darwin: FAIL code 1 (793.18=setup[331.04]+cmd[462.14] seconds)
  evaluation failed :( (803.78 seconds)
```
- This is because `join_timeout` is too short, so that the main thread
tries to assert the error logs before they are created.
- To fix it, set `join_timeout=None` to wait it infinitely until the
producer thread terminates.

Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ENHANCE Enhancement of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants