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

Added multi threading #177

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ArthurRochette
Copy link

Added multi-threading to the pydicer/convert/data.py/convert method.
Each patient is now processed in a separate thread, significantly improving conversion speed. By default, the method runs in single-threaded mode to maintain backward compatibility.

Added multi threading for convert method. 
Each patient will be converted in a single thread.
@pchlap
Copy link
Contributor

pchlap commented Jan 8, 2025

Thanks for this contribution @ArthurRochette! This is something we have been hoping to do for some time (#92).

I notice the pylint step is failing in our actions. Would you be able to check those issues and fix them up? Seems to be some formatting things, I would suggest using black to help fix many of these automatically.

One of the issues I experience when experimenting with multithreading in the past was the log output would become very messy. Have you had a chance to check if the logs output into the .pydicer directly are properly formatted. If they aren't I'm wondering if we need to think of a solution to that before merging this.

Thanks again for the PR!

@ArthurRochette
Copy link
Author

Hi Phil,
I've fixed the formatting issue.
While logs and parallelism can be challenging, the Python logging library is thread-safe, ensuring proper output formatting. The logs may appear messy due to interleaved outputs, but this shouldn't impact end users. This trade-off seems inevitable with parallel processing, but I'm open to suggestions for improvement.

@pchlap
Copy link
Contributor

pchlap commented Jan 26, 2025

Thanks @ArthurRochette! Sounds good. I will give this a test run over the next few days.

There is just one last thing pylint it complaining about. On line 22 of convert/data.py the get_iterator is still being imported even though it is no longer used. If you could remove that then we can run the tests within this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants