Skip to content

Feat/robust dependency install#55

Open
ghostbird03 wants to merge 4 commits into
langbot-app:mainfrom
ghostbird03:feat/robust-dependency-install
Open

Feat/robust dependency install#55
ghostbird03 wants to merge 4 commits into
langbot-app:mainfrom
ghostbird03:feat/robust-dependency-install

Conversation

@ghostbird03
Copy link
Copy Markdown

@ghostbird03 ghostbird03 commented May 10, 2026

What's new

  • Added precheck_dependencies() to scan requirements.txt before install and categorize already-installed vs to-install deps
  • Added install_with_retry() to retry pip install up to 3 times for transient failures
  • Added verify_dependencies() for post-install import verification
  • Added DependencyInstallError and DependencyVerificationError exception classes
  • Install loop now skips already-installed dependencies (only iterates to_install)
  • Retry error messages now include pip stderr output for debugging

Fixes over the original PR

  • Adapted to current main branch baseline — install_single_async returns 3-tuple here vs 2-tuple in the original, and retained existing get_pip_index_args() env-var mirror configuration.
  • find_spec false negatives for packages where pip name ≠ Python module name (e.g. yiri-mirai → mirai, PyYAML → yaml). Replaced with importlib.metadata.distribution() authoritative check + packages_distributions() reverse mapping fallback
  • Fragile package name extraction — the original manual chained .split() missed operators (!=, ~=, ===) and couldn't handle URL refs or environment markers. Replaced with packaging.requirements.Requirement for PEP 508 parsing
  • Already-installed deps were re-installed — precheck categorized deps but the loop still iterated all of them. Now skips already_installed and only installs to_install
  • Pip stderr was discarded on failure — retry wrapper threw away the actual error output, making diagnosis impossible. Now included in error message
  • No 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).

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

@RockChinQ RockChinQ left a comment

Choose a reason for hiding this comment

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

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.

  1. The original silent-failure issue has already been fixed on main

    The 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.

  2. 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.txt contains:

    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.

  3. packaging needs to be declared as a runtime dependency

    This PR imports:

    from packaging.requirements import Requirement

    but packaging is not added to pyproject.toml. In a clean environment this may fail with ModuleNotFoundError: No module named 'packaging'.

  4. 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.

  5. 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 like yiri-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 packaging to runtime dependencies if we rely on packaging.requirements.Requirement.
  • In precheck, parse each requirement and evaluate its marker first.
  • Use importlib.metadata.version() together with Requirement.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.
@ghostbird03
Copy link
Copy Markdown
Author

Addressed review feedback:

  • Version constraints are now checked via specifier.contains() — an installed requests==2.28 no longer satisfies requests>=2.32.
  • Environment markers (e.g. ; python_version < "3.9") are evaluated before precheck and verification, so they are no longer reported as missing.
  • URL requirements defer to pip for install decisions, with distribution existence accepted as sufficient post-install in verification.
  • Added packaging>=24.0 as an explicit runtime dependency.
  • Removed _extract_package_name()Requirement() is now the single parsing path; malformed specs return false to let pip decide.

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.

2 participants