Use a thread pool when installing dependencies

[blambright]

No description provided.

[buddly27]

Thanks for the PR Brett! I'm not very familiar with the ThreadPool, but reading your code made me want to use it more often :)

I just think the initializer needs to be in a different function for better separation of concern, but otherwise I like the logic. Just a few comment to address, and unit test to update. Integration & system tests are passing so this is a good sign!

[buddly27]

I think it would be better to get this function out of the install() function for better readability. Maybe it should be renamed to something like _install_in_thread() and take the queue as an argument? Also renaming _install() to _install_single() will help making sense of all of this. It would be something like:

def install(requests, output_path, ...):
    """Install packages to *output_path* from *requests*."""
    ...

def _install_in_thread(queue, ...):
    """Install package from requests and update queue."""
    ...

def _install_single(request, output_path, context_mapping, definition_mapping, ...):
    """Install single package to *output_path* from *request*."""
    ...

[blambright]

Hey Jeremy! I was hoping you would see my Teams message before seeing this pull request. This was more just a proof of concept rather than a legit implementation. I'm fine with any changes like this.

[buddly27]

Is that condition necessary? Why keeping one thread alive if other threads are active?

[blambright]

Basically I needed a way to keep all threads alive and waiting even if there was only one install active. Once the initializer function exits then the thread is gone. Since a library's dependencies only get added to the queue after a pip install, this would cause all of the threads to exit because the queue is empty however won't be empty soon. Basically what was happening was sometimes all but one thread would exit leaving only one thread remaining to do the rest of the installs in a linear fashion.

[buddly27]

Just to be sure that I understand properly, the active_count value ensure that the process doesn't finish accidentally after popping up the last item from the queue before processing it, right?

[blambright]

The active_count was a way to communicate between all the threads to stay alive until all installs have finished. The queue grows slowly causing it to go empty at times

[buddly27]

Why is that necessary? Is that because you risk a "Too many open files" error?

[blambright]

For some reason the output parsing that grabs the request from the stdout would fail. Basically the stdout would be empty even the pip install was successful. I've had something like this happen to me before and I knew that close_fds=True would fix it. If you remove it, you'll see what happens.

[buddly27]

Do you think it would be possible to avoid calling the _worker_execute twice? What if you fill up the queue and pass it to the ThreadPool initializer?

queue = six.moves.queue.Queue()

for request in requests:
    queue.put((request, None, editable_mode))

pool = multiprocessing.pool.ThreadPool(
    THREAD_COUNT, _worker_execute,
    (queue, overwrite if overwrite is not None else False)
)

[blambright]

This solved two things:

1. Calling the initial requests on the main thread would allow a user to select from the overwrite prompt if it shows up. Then the "yes to all" would be respected for the dependencies.
2. The queue would be full with dependencies by the time the thread pool started. I think that was only an issue before I added the active_count variable.

[buddly27]

Wouldn't it be safer to have "overwrite" being a multiprocessing.Value instance? So you could just mutate it in a thread without having to return it

[blambright]

Yeah, that sounds fine