-
Notifications
You must be signed in to change notification settings - Fork 133
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 multiprocessing to DatumaroBinaryBase #897
Add multiprocessing to DatumaroBinaryBase #897
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #897 +/- ##
========================================
Coverage 78.86% 78.86%
========================================
Files 204 204
Lines 25002 25020 +18
Branches 5026 5031 +5
========================================
+ Hits 19718 19733 +15
- Misses 4151 4153 +2
- Partials 1133 1134 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
a0fe19c
to
ca504ea
Compare
There was a problem hiding this 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
@@ -23,6 +23,7 @@ class DatumaroBinaryPath: | |||
SECRET_KEY_FILE = "secret_key.txt" | |||
|
|||
MAX_BLOB_SIZE = 2**20 # 1 Mega bytes | |||
MP_TIMEOUT = 30.0 # 30 secs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to consider MP_TIMEOUT
as 30 secs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, I just put in a value that I feel moderately long.
Signed-off-by: Kim, Vinnam <vinnam.kim@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for awesome work!
Summary
--num-workers
option toDatumaroBinaryBase
for better performance.How to test
I updated the tests to cover this change.
Checklist
License
Feel free to contact the maintainers if that's a concern.