Fix pickling issue with multiprocessing.Process to (down)load volumes and re-enable (down)load cancelling
#7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The comment explains it all, To avoid `zlib` blowing up the memory usage, we need to reduce the chunk size a lot.
The downloads are now done in chunks, allowing an optional callback to be provided which is checked between every chunk. The callback is expected to return a boolean dictating whether to cancel the download or not.
Contributor
Author
|
Actually, never mind. It's still not working to cancel a load. |
olivierdelree
force-pushed
the
odelree/issue6
branch
from
fa8fc22 to
069af6d
Compare
Contributor
Author
|
I'm giving up for now, pickling should be fixed but allowing cancellation is not and remains disabled. |
Contributor
Author
|
Sorry, not for review, but for testing on macOS... |
This drops the use of `multiprocessing.Process` objects to (down)load volumes, which should fix the pickling troubles on macOS. Additionally, the thread can now react to interruption requests and gracefully cancels downloads or abruptly terminates loads.
This allows calling code to handle cases where the user cancelled the (down)load.
olivierdelree
force-pushed
the
odelree/issue6
branch
from
069af6d to
6d81467
Compare
This makes use of processes to load NumPy arrays of the volumes in a separate environment, allowing it to be cancelled without freezing the main GUI. Additionally, the normalisation can now be done "fast", meaning that now floating, intermediary arrays are used, saving a bit of speed and a lot of memory.
olivierdelree
changed the title
multiprocessing.Process to (down)load volumes and re-enable (down)load cancellingmultiprocessing.Process to (down)load volumes and re-enable (down)load cancelling
Contributor
Author
|
There you go. Now mostly works, although the GUI still freezes a bit towards the end of the loading, when merging with the main thread. |
Member
|
Is this still being drafted atm? Still seeing the same issue on MacOS |
Member
|
Well I'm seeing the following |
Contributor
Author
|
Should have be fixed by ab00 on |
Contributor
Author
|
But the PR is mostly dead. I might revisit it later but will need to have a better look at completely changing the way volumes are loaded to ensure no pickling is necessary. |
Member
|
Ah right yes I was running the released version instead of the one currently on main, works fine now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #6.
This was tested on Linux and Windows, and will be merged once it is tested on macOS.
@celefthe Could you check if you can create projects fine on macOS with this branch?