Feat/robust dependency install#55
Conversation
…dk#54 Migrate PR langbot-app#54 to add pre-check, retry, and verification for plugin dependency installation. - errors.py: add DependencyInstallError, DependencyVerificationError - pkgmgr.py: add verify_dependencies, install_with_retry, precheck_dependencies - mgr.py: wire precheck -> retry -> verify into install flow, track failed deps, raise on failure after all retries exhausted
…e mismatch verify_dependencies and precheck_dependencies used find_spec with the normalized pip package name, which fails for packages whose pip name differs from their Python import name (e.g. yiri-mirai -> mirai, PyYAML -> yaml, Pillow -> PIL). Fix: check distribution metadata via importlib.metadata.distribution() first (authoritative pip install check), then resolve actual import names via packages_distributions() reverse mapping as fallback.
…ady-installed deps in install loop - Replace fragile chained split in _extract_package_name with packaging.requirements.Requirement for proper PEP 508 parsing - Pre-checked already-installed dependencies are skipped in the install loop instead of being re-installed - install_with_retry now captures pip stderr output on failure for meaningful error messages
There was a problem hiding this comment.
Thanks for working on this and for taking the previous dependency-install failure issue seriously. The direction of adding retry / verification is valuable, and I appreciate that this PR also considered the PyPI package name vs Python import name mismatch case, such as yiri-mirai being imported as mirai.
I’m requesting changes for now because there are a few correctness issues that could introduce new dependency-resolution bugs.
-
The original silent-failure issue has already been fixed on
mainThe core problem from #53 was that dependency installation failures were logged but not propagated to the user. This has already been fixed in de9d464: failed pip installs now raise with the pip output, so the error can surface to the UI.
So for this PR, the remaining useful parts are mainly retry / precheck / verification improvements rather than the basic “raise on install failure” behavior.
-
The current precheck may incorrectly skip packages with unsatisfied version constraints
_check_dependency_installed()currently treats a dependency as installed if the distribution exists, but it does not check whether the installed version satisfies the requirement specifier.For example, if
requirements.txtcontains:requests>=2.32
and the environment already has
requests==2.28, the current precheck would mark it as already installed and skip pip install, even though the requirement is not satisfied.This could make plugin installation appear successful while leaving an incompatible dependency version in the environment.
-
packagingneeds to be declared as a runtime dependencyThis PR imports:
from packaging.requirements import Requirement
but
packagingis not added topyproject.toml. In a clean environment this may fail withModuleNotFoundError: No module named 'packaging'. -
Environment markers should be evaluated before verification
Requirements such as:
foo; python_version < "3.9"
may be intentionally skipped by pip when the marker does not match the current environment.
verify_dependencies(deps)should not report those as missing dependencies. -
The package-name/import-name mismatch handling is good, but should be combined with requirement validation
Using
importlib.metadata.distribution()is the right direction for avoiding false negatives with packages likeyiri-mirai -> mirai,PyYAML -> yaml,Pillow -> PIL, etc.However, precheck / verification still needs to account for:
- whether the requirement marker applies to the current environment;
- whether the installed distribution version satisfies the specifier;
- extras / URL requirements;
- not skipping pip install solely because a distribution with the same name exists.
Suggested changes:
- Add
packagingto runtime dependencies if we rely onpackaging.requirements.Requirement. - In precheck, parse each requirement and evaluate its marker first.
- Use
importlib.metadata.version()together withRequirement.specifier.contains(...)to decide whether an installed distribution satisfies the requirement. - Only skip installation when the current environment truly satisfies the full requirement.
- Make verification ignore requirements whose environment markers do not apply.
After these are addressed, the retry / verification improvements should be much safer to merge.
…heck/verify Changes addressing review feedback on langbot-app#55: - _check_dependency_installed() now evaluates PEP 508 environment markers (e.g. `foo; python_version < "3.9"` is correctly skipped when the marker does not apply). - Version constraints are verified via specifier.contains(): a package like `requests>=2.32` is only considered satisfied if the installed version actually meets the requirement. - URL requirements (e.g. `foo @ https://...`) return False in precheck so pip decides whether reinstall is needed, but verify_dependencies() accepts distribution existence after pip has run. - Removed _extract_package_name() dead code — Requirement() is now called directly; malformed specs return False to let pip handle them. - Added packaging>=24.0 to pyproject.toml as an explicit runtime dependency.
Addressed review feedback:
|
What's new
precheck_dependencies()to scan requirements.txt before install and categorize already-installed vs to-install depsinstall_with_retry()to retry pip install up to 3 times for transient failuresverify_dependencies()for post-install import verificationDependencyInstallErrorandDependencyVerificationErrorexception classesto_install)Fixes over the original PR
get_pip_index_args()env-var mirror configuration.find_spec falsenegatives for packages where pip name ≠ Python module name (e.g. yiri-mirai → mirai, PyYAML → yaml). Replaced withimportlib.metadata.distribution()authoritative check +packages_distributions()reverse mapping fallback.split()missed operators (!=, ~=, ===) and couldn't handle URL refs or environment markers. Replaced withpackaging.requirements.Requirementfor PEP 508 parsingalready_installedand only installsto_installNo error was raised on failure — original PR silently logged failures and continued. Restored the existing raise RuntimeError behavior (retries are exhausted, verification fails, then raise)Credits
Thanks to @TyperBody for the original implementation in #54 (#54).