Fix sequential sync losing track of pip install processes

[gkoz]

The issue

pipenv sync --sequential may silently fail to install a package.
pipenv sync --sequential -v doesn't emit verbose logs of any pip install invocations but the first.

The fix

In sequential (blocking) mode only the first pip install process handle was being put in the queue, the subsequent ones never reaching cleanup, where the exit code is checked, hiding installation errors and logs. Put each handle in the queue unconditionally and remove nprocs argument, which appears to serve the same purpose as procs.maxsize but disagrees with it in sequential mode.

While here, remove some unnecessary calls and variables.

The checklist

• Fixes Pipenv sync in sequential mode ignores pip install results #3557.
• A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

[gkoz]

Added tests. Opened an issue with steps to reproduce.

[techalchemy]

Thanks for the PR and for writing tests, can you confirm that the tests were failing before your PR and that your PR is what makes them pass?

There are a number of changes here that are not related to fixing --sequential, some of which will break important functionality. I can't really move ahead with merging this until it's scaled back to focus only on the fix it is intended to address

[techalchemy]

the size check needs to remain here because the queue size will be up to PIPENV_MAX_SUBPROCESS even if we don't have that many dependencies to install, and if we have more than PIPENV_MAX_SUBPROCESS we are very likely to have less during the final installation round.

[gkoz]

Assuming we're talking about if procs.full() or procs.qsize() == len(deps_list):...

This one's a bit awkward. I've failed to find the reason for this check by myself and also can't parse your explanation. I believe:

• This function must force the cleanup when the queue is full because running new processes would violate the nprocs limit.
• Otherwise it doesn't strictly need to clean up ever because the caller will clean up regardless.
• The condition seems to only be true at the last iteration of the loop when all dependencies have fit in the queue at once, but I don't see the significance.

[techalchemy]

it is only true in the last run of a concurrent=True cycle through the loop and I believe there was some race condition where not all processes were cleaned up during one pass if it didn't saturate the queue...

It is possible it isn't needed, however removing an additional safeguard in a bugfix PR where you are mixing in refactoring is my primary issue. There is a good chance it is like this for a reason, even if that reason is not documented

[techalchemy]

This is specified as an argument to the function and re-specified here because we intentionally install with --sequential if there is a failure. This functionality needs to stay as is

[techalchemy]

We support passing nprocs here to allow us to fall back to --sequential on failure. Please revert this aspect of the change.

[gkoz]

I'm sorry I overlooked the fallback mode case. Eliminating the issue requires removing redundancy between nprocs and maxsize though. I'd propose just dropping the maxsize then:

diff --git a/pipenv/core.py b/pipenv/core.py
index 7daff2be..ba5b3dc2 100644
--- a/pipenv/core.py
+++ b/pipenv/core.py
@@ -747,11 +747,10 @@ def batch_install(deps_list, procs, failed_deps_queue,
             )
             if dep.is_vcs or dep.editable:
                 c.block()
-            if procs.qsize() < nprocs:
-                c.dep = dep
-                procs.put(c)
+            c.dep = dep
+            procs.put(c)
 
-            if procs.full() or procs.qsize() == len(deps_list):
+            if procs.qsize() >= nprocs:
                 _cleanup_procs(procs, not blocking, failed_deps_queue, retry=retry)
 
 
@@ -812,7 +811,7 @@ def do_install_dependencies(
         )
         sys.exit(0)
 
-    procs = queue.Queue(maxsize=PIPENV_MAX_SUBPROCESS)
+    procs = queue.Queue()
     failed_deps_queue = queue.Queue()
     if skip_lock:
         ignore_hashes = True

[techalchemy]

hm that makes sense, i'm good with this approach. Thanks for your patience

[techalchemy]

so you have essentially dropped an argument, moved a queue definition, and redefined something that was passed as a kwarg to be a variable? There is a lot of unrelated code movement here when I think this is the only line related to the actual change described

[gkoz]

I don't get it. This line relies on the four lines above it and the queue definition didn't move anywhere.

[gkoz]

I recognize that the two additional commits aren't related to the fix, but they're meant to reduce the number of moving parts and mental overhead based on my experience comprehending the affected code. Can you confirm such changes don't merit consideration and are rejected summarily?

[techalchemy]

I'm good with the most recent proposal -- feel free to resolve the conflicts with master (or else I can do it, they're not complicated)

thanks for your patience, and I do appreciate the cleanup effort

[salty-horse]

I left this following comment in the bug @gkoz opened. The title of this PR is misleading, as the bug is not isolated to running with --sequential.

If you're trying to install more packages than PIPENV_MAX_SUBPROCESS (16 on my machine), pipenv will ignore the results of any above that limit.
(It also makes it impossible to debug any installation errors because --verbose will not print the pip output of the commands it ignores.)

[jstriebel]

Thanks for the fix! I am also affected by this bug, leading to packages silently not being installed. @techalchemy & @gkoz Could you resolve the conflicts? I can also add a PR to this PR, but seems to much overhead for this small change.
I would really appreciate if this could be merged with priority, as not missing packages silently is quite crucial. Thanks for the great work 👍

[salty-horse]

@gkoz's patch at #3537 (comment) is a good fix for this. What is it waiting for?