Issue633 threaded download #708
Conversation
|
Secondly I want to be cautious on how this potentially interacts with export workspace. |
| def on_job_done(self, job: BatchJob, row): | ||
| """ | ||
| Handles jobs that have finished. Can be overridden to provide custom behaviour. | ||
|
|
||
| Default implementation downloads the results into a folder containing the title. | ||
| Default implementation runs the download in a separate thread. |
There was a problem hiding this comment.
I think threading should be opt-in and classic serial downloading should be the default (at least for now).
It's easy to get in trouble with threading (and other parallelism paradigms like async as used in notebooks).
|
|
||
| :param job: The job that has finished. | ||
| :param row: DataFrame row containing the job's metadata. | ||
| """ | ||
| # TODO: param `row` is never accessed in this method. Remove it? Is this intended for future use? |
There was a problem hiding this comment.
I'd keep this TODO comment, it's not resolved yet as far as I know
| # Start the download in a separate thread | ||
| _log.info(f"Starting download thread for job {job.job_id}") | ||
| downloader = Thread(target=download_task, daemon=True) | ||
| downloader.start() |
There was a problem hiding this comment.
you should keep track of the Thread objects, to properly .join() them when they are done later
| def download_task(): | ||
| try: | ||
| _log.info(f"Starting download for job {job.job_id} to directory {job_dir}") | ||
| job.get_results().download_files(target=job_dir) |
There was a problem hiding this comment.
you use job here as non-local variable from the on_job_done closure. I wonder how thread-safe this is.
A batch job object on it's own is not that complex except for the connection inside it: that's quite a complex object with a lot of state and risk of being thread-unsafe. For robust threaded downloading I would try to minimize the data structures you are sharing from the main thread to the download thread. E.g. just the backend URL (a string), the job id (a string) and a valid access token (a string) might be enough. Or even better: just pass the download URLs if they are signed (and don't require a working access token)
Somewhat of a preemptive PR.
I tried to thread of the downloading in a similar style as the threaded MPJM.
I'd also would hope to get some input on needed unit tests. My current ideas are: