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

Changed return type of multithread_exec to iterator #1019

Merged
merged 3 commits into from
Aug 25, 2022

Conversation

mtvch
Copy link
Contributor

@mtvch mtvch commented Aug 13, 2022

multithread_exec function returned result of different types depending on number of threads: if it's lesser than 2 - Iterator, otherwise - list.

In every place this function is used, result is expected to be of type list and it fails if result is an Iterator.

With this change function would always return Iterator to let user decide which type to convert it to.

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #1019 (d672d92) into main (26ed132) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
+ Coverage   94.87%   94.93%   +0.06%     
==========================================
  Files         135      135              
  Lines        5559     5590      +31     
==========================================
+ Hits         5274     5307      +33     
+ Misses        285      283       -2     
Flag Coverage Δ
unittests 94.93% <100.00%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
doctr/datasets/loader.py 100.00% <100.00%> (ø)
doctr/models/preprocessor/pytorch.py 96.42% <100.00%> (ø)
doctr/models/preprocessor/tensorflow.py 96.15% <100.00%> (ø)
doctr/utils/multithreading.py 100.00% <100.00%> (ø)
doctr/models/zoo.py 100.00% <0.00%> (ø)
doctr/transforms/modules/base.py 94.59% <0.00%> (ø)
doctr/models/predictor/tensorflow.py 100.00% <0.00%> (ø)
...tr/models/classification/magc_resnet/tensorflow.py 100.00% <0.00%> (ø)
doctr/models/builder.py 99.03% <0.00%> (+0.01%) ⬆️
doctr/models/_utils.py 98.30% <0.00%> (+2.30%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@felixdittrich92 felixdittrich92 self-assigned this Aug 15, 2022
@felixdittrich92 felixdittrich92 added this to the 0.6.0 milestone Aug 15, 2022
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

Hi @mtvch 👋,
thanks for the PR.

LGTM 👍
Only isort and mypy left to fix:
Inside the project you can run make quality to fix import sorting (isort) and get a output which changes you have to apply to make mypy happy :)

@felixdittrich92 felixdittrich92 added type: bug Something isn't working module: models Related to doctr.models module: utils Related to doctr.utils ext: tests Related to tests folder module: datasets Related to doctr.datasets labels Aug 15, 2022
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I only have one comment about the potential latency change, let me know what you think :)

doctr/utils/multithreading.py Outdated Show resolved Hide resolved
@frgfm
Copy link
Collaborator

frgfm commented Aug 16, 2022

LGTM +1 Only isort and mypy left to fix: Inside the project you can run make quality to fix import sorting (isort) and get a output which changes you have to apply to make mypy happy :)

Actually, make quality spots the error, and make style fixes black & isort :)

@felixdittrich92
Copy link
Contributor

Hi @mtvch 👋 , any updates ? :)

@mtvch
Copy link
Contributor Author

mtvch commented Aug 19, 2022

Hi @mtvch wave , any updates ? :)

Hi, @felixdittrich92!
Sorry, I'm busy right know and could not work on this... Also for me it takes more time, because I have little experience with Python.
But I'm planning to complete them until September (most likely next weekend)

@felixdittrich92
Copy link
Contributor

Hi @mtvch wave , any updates ? :)

Hi, @felixdittrich92! Sorry, I'm busy right know and could not work on this... Also for me it takes more time, because I have little experience with Python. But I'm planning to complete them until September (most likely next weekend)

Thanks for the information. yes no stress 🤗

@felixdittrich92 felixdittrich92 marked this pull request as draft August 24, 2022 06:57
@felixdittrich92 felixdittrich92 marked this pull request as ready for review August 25, 2022 07:55
@felixdittrich92
Copy link
Contributor

@mtvch only mypy fix left :)

doctr/models/preprocessor/tensorflow.py:120: error: Unused "type: ignore"
comment
                batches = self.batch_inputs(samples)  # type: ignore[arg-t...
                ^
doctr/models/preprocessor/tensorflow.py:[12](https://github.com/mindee/doctr/runs/8011083781?check_suite_focus=true#step:6:13)5: error: Unused "type: ignore"
comment
            batches = list(multithread_exec(self.normalize, batches))  # t...
            ^
doctr/models/preprocessor/pytorch.py:122: error: Unused "type: ignore" comment
                batches = self.batch_inputs(samples)  # type: ignore[arg-t...
                ^
doctr/models/preprocessor/pytorch.py:127: error: Unused "type: ignore" comment
            batches = list(multithread_exec(self.normalize, batches))  # t...
            ^
Found 4 errors in 2 files (checked [13](https://github.com/mindee/doctr/runs/8011083781?check_suite_focus=true#step:6:14)6 source files)

@mtvch
Copy link
Contributor Author

mtvch commented Aug 25, 2022

@mtvch only mypy fix left :)

doctr/models/preprocessor/tensorflow.py:120: error: Unused "type: ignore"
comment
                batches = self.batch_inputs(samples)  # type: ignore[arg-t...
                ^
doctr/models/preprocessor/tensorflow.py:[12](https://github.com/mindee/doctr/runs/8011083781?check_suite_focus=true#step:6:13)5: error: Unused "type: ignore"
comment
            batches = list(multithread_exec(self.normalize, batches))  # t...
            ^
doctr/models/preprocessor/pytorch.py:122: error: Unused "type: ignore" comment
                batches = self.batch_inputs(samples)  # type: ignore[arg-t...
                ^
doctr/models/preprocessor/pytorch.py:127: error: Unused "type: ignore" comment
            batches = list(multithread_exec(self.normalize, batches))  # t...
            ^
Found 4 errors in 2 files (checked [13](https://github.com/mindee/doctr/runs/8011083781?check_suite_focus=true#step:6:14)6 source files)

Sorry...
Now it's fixed

Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

Thanks a lot LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: tests Related to tests folder module: datasets Related to doctr.datasets module: models Related to doctr.models module: utils Related to doctr.utils type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants