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

Fix sequential sync losing track of pip install processes #3537

Merged
merged 6 commits into from Jun 4, 2019

Conversation

gkoz
Copy link
Contributor

@gkoz gkoz commented Feb 14, 2019

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 #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
Copy link
Contributor Author

@gkoz gkoz commented Feb 24, 2019

Added tests. Opened an issue with steps to reproduce.

Copy link
Member

@techalchemy techalchemy left a comment

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

pipenv/core.py Outdated
if procs.full() or procs.qsize() == len(deps_list):
_cleanup_procs(procs, not blocking, failed_deps_queue, retry=retry)
procs.put(c)
if procs.full():
Copy link
Member

@techalchemy techalchemy Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gkoz gkoz Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@techalchemy techalchemy Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -831,15 +827,14 @@ def do_install_dependencies(
failed_dep = failed_deps_queue.get()
retry_list.append(failed_dep)
install_kwargs.update({
"nprocs": 1,
Copy link
Member

@techalchemy techalchemy Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -674,7 +674,7 @@ def _cleanup_procs(procs, concurrent, failed_deps_queue, retry=True):
def batch_install(deps_list, procs, failed_deps_queue,
requirements_dir, no_deps=False, ignore_hashes=False,
allow_global=False, blocking=False, pypi_mirror=None,
nprocs=PIPENV_MAX_SUBPROCESS, retry=True):
Copy link
Member

@techalchemy techalchemy Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@gkoz gkoz Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@techalchemy techalchemy Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

nprocs = PIPENV_MAX_SUBPROCESS
else:
nprocs = 1
procs = queue.Queue(maxsize=nprocs)
Copy link
Member

@techalchemy techalchemy Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@gkoz gkoz Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gkoz
Copy link
Contributor Author

@gkoz gkoz commented Mar 7, 2019

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
Copy link
Member

@techalchemy techalchemy commented Mar 8, 2019

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
Copy link
Contributor

@salty-horse salty-horse commented Mar 11, 2019

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
Copy link

@jstriebel jstriebel commented Apr 17, 2019

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
Copy link
Contributor

@salty-horse salty-horse commented Apr 17, 2019

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

gkoz and others added 5 commits May 27, 2019
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.

Remove the nprocs argument and set `procs.maxsize` to 1 in sequential mode
instead.

Remove redundant `Command.block` call, `delegate.run` takes care of that.

Simplify queue cleanup condition. It's only necessary to cleanup inside the
loop if `procs` is full.
Block on a process in cleanup iff it was run as non-blocking. Blocking
processes are blocked on immediately.
Signed-off-by: Dan Ryan <dan@danryan.co>
@techalchemy techalchemy merged commit 6700d00 into pypa:master Jun 4, 2019
18 checks passed
fwojciak pushed a commit to fwojciak/pipenv that referenced this issue May 29, 2020
2020.5.28 (2020-05-28)
======================

Features & Improvements
-----------------------

-   `pipenv install` and `pipenv sync` will no longer attempt to install satisfied dependencies during installation. pypa#3057, pypa#3506
-   Added support for resolution of direct-url dependencies in `setup.py` files to respect `PEP-508` style URL dependencies. pypa#3148
-   Added full support for resolution of all dependency types including direct URLs, zip archives, tarballs, etc.
    -   Improved error handling and formatting.
    -   Introduced improved cross platform stream wrappers for better `stdout` and `stderr` consistency. pypa#3298
-   For consistency with other commands and the `--dev` option description, `pipenv lock --requirements --dev` now emits both default and development dependencies. The new `--dev-only` option requests the previous behaviour (e.g. to generate a `dev-requirements.txt` file). pypa#3316
-   Pipenv will now successfully recursively lock VCS sub-dependencies. pypa#3328
-   Added support for `--verbose` output to `pipenv run`. pypa#3348
-   Pipenv will now discover and resolve the intrinsic dependencies of **all** VCS dependencies, whether they are editable or not, to prevent resolution conflicts. pypa#3368
-   Added a new environment variable, `PIPENV_RESOLVE_VCS`, to toggle dependency resolution off for non-editable VCS, file, and URL based dependencies. pypa#3577
-   Added the ability for Windows users to enable emojis by setting `PIPENV_HIDE_EMOJIS=0`. pypa#3595
-   Allow overriding `PIPENV_INSTALL_TIMEOUT` environment variable (in seconds). pypa#3652
-   Allow overriding `PIP_EXISTS_ACTION` evironment variable (value is passed to pip install). Possible values here: <https://pip.pypa.io/en/stable/reference/pip/#exists-action-option> Useful when you need to `PIP\_EXISTS\_ACTION=i` (ignore existing packages) - great for CI environments, where you need really fast setup. pypa#3738
-   Pipenv will no longer forcibly override `PIP_NO_DEPS` on all vcs and file dependencies as resolution happens on these in a pre-lock step. pypa#3763
-   Improved verbose logging output during `pipenv lock` will now stream output to the console while maintaining a spinner. pypa#3810
-   Added support for automatic python installs via `asdf` and associated `PIPENV_DONT_USE_ASDF` environment variable. pypa#4018
-   Pyenv/asdf can now be used whether or not they are available on PATH. Setting `PYENV_ROOT`/`ASDF_DIR` in a `.env` file allows Pipenv to install an interpreter without any shell customizations, so long as pyenv/asdf is installed. pypa#4245
-   Added `--key` command line parameter for including personal PyUp.io API tokens when running `pipenv check`. pypa#4257

Behavior Changes
----------------

-   Make conservative checks of known exceptions when subprocess returns output, so user won\'t see the whole traceback - just the error. pypa#2553
-   Do not touch Pipfile early and rely on it so that one can do `pipenv sync` without a Pipfile. pypa#3386
-   Re-enable `--help` option for `pipenv run` command. pypa#3844
-   Make sure `pipenv lock -r --pypi-mirror {MIRROR_URL}` will respect the pypi-mirror in requirements output. pypa#4199

Bug Fixes
---------

-   Raise `PipenvUsageError` when \[\[source\]\] does not contain url field. pypa#2373
-   Fixed a bug which caused editable package resolution to sometimes fail with an unhelpful setuptools-related error message. pypa#2722
-   Fixed an issue which caused errors due to reliance on the system utilities `which` and `where` which may not always exist on some
    systems.
-   Fixed a bug which caused periodic failures in python discovery when executables named `python` were not present on the target `$PATH`. pypa#2783
-   Dependency resolution now writes hashes for local and remote files to the lockfile. pypa#3053
-   Fixed a bug which prevented `pipenv graph` from correctly showing all dependencies when running from within `pipenv shell`. pypa#3071
-   Fixed resolution of direct-url dependencies in `setup.py` files to respect `PEP-508` style URL dependencies. pypa#3148
-   Fixed a bug which caused failures in warning reporting when running pipenv inside a virtualenv under some circumstances.
-   Fixed a bug with package discovery when running `pipenv clean`. pypa#3298
-   Quote command arguments with carets (`^`) on Windows to work around unintended shell escapes. pypa#3307
-   Handle alternate names for UTF-8 encoding. pypa#3313
-   Abort pipenv before adding the non-exist package to Pipfile. pypa#3318
-   Don\'t normalize the package name user passes in. pypa#3324
-   Fix a bug where custom virtualenv can not be activated with pipenv shell pypa#3339
-   Fix a bug that `--site-packages` flag is not recognized. pypa#3351
-   Fix a bug where `pipenv --clear` is not working pypa#3353
-   Fix unhashable type error during `$ pipenv install --selective-upgrade` pypa#3384
-   Dependencies with direct `PEP508` compliant VCS URLs specified in their `install_requires` will now be successfully locked during the resolution process. pypa#3396
-   Fixed a keyerror which could occur when locking VCS dependencies in
    some cases. pypa#3404
-   Fixed a bug that `ValidationError` is thrown when some fields are missing in source section. pypa#3427
-   Updated the index names in lock file when source name in Pipfile is changed. pypa#3449
-   Fixed an issue which caused `pipenv install --help` to show duplicate entries for `--pre`. pypa#3479
-   Fix bug causing `[SSL: CERTIFICATE_VERIFY_FAILED]` when Pipfile `[[source]]` has `verify_ssl=false` and url with custom port. pypa#3502
-   Fix `sync --sequential` ignoring `pip install` errors and logs. pypa#3537
-   Fix the issue that lock file can\'t be created when `PIPENV_PIPFILE` is not under working directory. pypa#3584
-   Pipenv will no longer inadvertently set `editable=True` on all vcs dependencies. pypa#3647
-   The `--keep-outdated` argument to `pipenv install` and `pipenv lock` will now drop specifier constraints when encountering editable dependencies.
    -   In addition, `--keep-outdated` will retain specifiers that would otherwise be dropped from any entries that have not been updated. pypa#3656
-   Fixed a bug which sometimes caused pipenv to fail to respect the `--site-packages` flag when passed with `pipenv install`. pypa#3718
-   Normalize the package names to lowercase when comparing used and in-Pipfile packages. pypa#3745
-   `pipenv update --outdated` will now correctly handle comparisons between pre/post-releases and normal releases. pypa#3766
-   Fixed a `KeyError` which could occur when pinning outdated VCS dependencies via `pipenv lock --keep-outdated`. pypa#3768
-   Resolved an issue which caused resolution to fail when encountering poorly formatted `python_version` markers in `setup.py` and `setup.cfg` files. pypa#3786
-   Fix a bug that installation errors are displayed as a list. pypa#3794
-   Update `pythonfinder` to fix a problem that `python.exe` will be mistakenly chosen for virtualenv creation under WSL. pypa#3807
-   Fixed several bugs which could prevent editable VCS dependencies from being installed into target environments, even when reporting
    successful installation. pypa#3809
-   `pipenv check --system` should find the correct Python interpreter when `python` does not exist on the system. pypa#3819
-   Resolve the symlinks when the path is absolute. pypa#3842
-   Pass `--pre` and `--clear` options to `pipenv update --outdated`. pypa#3879
-   Fixed a bug which prevented resolution of direct URL dependencies which have PEP508 style direct url VCS sub-dependencies with
    subdirectories. pypa#3976
-   Honor `PIPENV_SPINNER` environment variable pypa#4045
-   Fixed an issue with `pipenv check` failing due to an invalid API key from `pyup.io`. pypa#4188
-   Fixed a bug which caused versions from VCS dependencies to be included in `Pipfile.lock` inadvertently. pypa#4217
-   Fixed a bug which caused pipenv to search non-existent virtual environments for `pip` when installing using `--system`. pypa#4220
-   `Requires-Python` values specifying constraint versions of python starting from `1.x` will now be parsed successfully. pypa#4226
-   Fix a bug of `pipenv update --outdated` that can\'t print output correctly. pypa#4229
-   Fixed a bug which caused pipenv to prefer source distributions over wheels from `PyPI` during the dependency resolution phase. Fixed an issue which prevented proper build isolation using `pep517` based builders during dependency resolution. pypa#4231
-   Don\'t fallback to system Python when no matching Python version is found. pypa#4232

Vendored Libraries
------------------

- Updated `pip_shims` to support `--outdated` with new pip versions. pypa#3766
- Update vendored dependencies and invocations
  - Update vendored and patched dependencies
  - Update patches on `piptools`, `pip`, `pip-shims`, `tomlkit`
  - Fix invocations of dependencies
  - Fix custom `InstallCommand` instantiation
  - Update `PackageFinder` usage
  - Fix `Bool` stringify attempts from `tomlkit`
  - Updated vendored dependencies:
    -   **attrs**: `18.2.0 => `19.1.0`
    -   **certifi**: `2018.10.15 => `2019.3.9`
    -   **cached\_property**: `1.4.3 => `1.5.1`
    -   **cerberus**: `1.2.0 => `1.3.1`
    -   **click**: `7.0.0 => `7.1.1`
    -   **click-completion**: `0.5.0 => `0.5.1`
    -   **colorama**: `0.3.9 => `0.4.3`
    -   **contextlib2**: `(new) => `0.6.0.post1`
    -   **distlib**: `0.2.8 => `0.2.9`
    -   **funcsigs**: `(new) => `1.0.2`
    -   **importlib\_metadata** `1.3.0 => `1.5.1`
    -   **importlib-resources**: `(new) => `1.4.0`
    -   **idna**: `2.7 => `2.9`
    -   **jinja2**: `2.10.0 => `2.11.1`
    -   **markupsafe**: `1.0 => `1.1.1`
    -   **more-itertools**: `(new) => `5.0.0`
    -   **orderedmultidict**: `(new) => `1.0`
    -   **packaging**: `18.0 => `19.0`
    -   **parse**: `1.9.0 => `1.15.0`
    -   **pathlib2**: `2.3.2 => `2.3.3`
    -   **pep517**: `(new) => `0.5.0`
    -   **pexpect**: `4.6.0 => `4.8.0`
    -   **pip-shims**: `0.2.0 => `0.5.1`
    -   **pipdeptree**: `0.13.0 => `0.13.2`
    -   **pyparsing**: `2.2.2 => `2.4.6`
    -   **python-dotenv**: `0.9.1 => `0.10.2`
    -   **pythonfinder**: `1.1.10 => `1.2.2`
    -   **pytoml**: `(new) => `0.1.20`
    -   **requests**: `2.20.1 => `2.23.0`
    -   **requirementslib**: `1.3.3 => `1.5.4`
    -   **scandir**: `1.9.0 => `1.10.0`
    -   **shellingham**: `1.2.7 => `1.3.2`
    -   **six**: `1.11.0 => `1.14.0`
    -   **tomlkit**: `0.5.2 => `0.5.11`
    -   **urllib3**: `1.24 => `1.25.8`
    -   **vistir**: `0.3.0 => `0.5.0`
    -   **yaspin**: `0.14.0 => `0.14.3`
    -   **zipp**: `0.6.0`
    - Removed vendored dependency **cursor**. pypa#4169

-   Add and update vendored dependencies to accommodate `safety` vendoring:
    -   **safety** `(none)` => `1.8.7`
    -   **dparse** `(none)` => `0.5.0`
    -   **pyyaml** `(none)` => `5.3.1`
    -   **urllib3** `1.25.8` => `1.25.9`
    -   **certifi** `2019.11.28` => `2020.4.5.1`
    -   **pyparsing** `2.4.6` => `2.4.7`
    -   **resolvelib** `0.2.2` => `0.3.0`
    -   **importlib-metadata** `1.5.1` => `1.6.0`
    -   **pip-shims** `0.5.1` => `0.5.2`
    -   **requirementslib** `1.5.5` => `1.5.6` pypa#4188

-   Updated vendored `pip` => `20.0.2` and `pip-tools` => `5.0.0`. pypa#4215
-   Updated vendored dependencies to latest versions for security and bug fixes:
    -   **requirementslib** `1.5.8` => `1.5.9`
    -   **vistir** `0.5.0` => `0.5.1`
    -   **jinja2** `2.11.1` => `2.11.2`
    -   **click** `7.1.1` => `7.1.2`
    -   **dateutil** `(none)` => `2.8.1`
    -   **backports.functools\_lru\_cache** `1.5.0` => `1.6.1`
    -   **enum34** `1.1.6` => `1.1.10`
    -   **toml** `0.10.0` => `0.10.1`
    -   **importlib\_resources** `1.4.0` => `1.5.0` pypa#4226
-   Changed attrs import path in vendored dependencies to always import from `pipenv.vendor`. pypa#4267

Improved Documentation
----------------------

-   Added documenation about variable expansion in `Pipfile` entries. pypa#2317
-   Consolidate all contributing docs in the rst file pypa#3120
-   Update the out-dated manual page. pypa#3246
-   Move CLI docs to its own page. pypa#3346
-   Replace (non-existant) video on docs index.rst with equivalent gif. pypa#3499
-   Clarify wording in Basic Usage example on using double quotes to escape shell redirection pypa#3522
-   Ensure docs show navigation on small-screen devices pypa#3527
-   Added a link to the TOML Spec under General Recommendations & Version Control to clarify how Pipfiles should be written. pypa#3629
-   Updated the documentation with the new `pytest` entrypoint. pypa#3759
-   Fix link to GIF in README.md demonstrating Pipenv\'s usage, and add descriptive alt text. pypa#3911
-   Added a line describing potential issues in fancy extension. pypa#3912
-   Documental description of how Pipfile works and association with Pipenv. pypa#3913
-   Clarify the proper value of `python_version` and `python_full_version`. pypa#3914
-   Write description for `--deploy` extension and few extensions differences. pypa#3915
-   More documentation for `.env` files pypa#4100
-   Updated documentation to point to working links. pypa#4137
-   Replace docs.pipenv.org with pipenv.pypa.io pypa#4167
-   Added functionality to check spelling in documentation and cleaned up existing typographical issues. pypa#4209
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.

4 participants