Skip to content

fix(lazy-deps): use version-aware check in active_features() to prevent unnecessary downgrades - #44416

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/lazy-deps-active-features-version-check
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/lazy-deps-active-features-version-check

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes active_features() in tools/lazy_deps.py to use version-aware _is_satisfied() instead of presence-only _is_present(). This prevents refresh_active_features() (triggered by hermes update) from unnecessarily downgrading packages that are installed at a working newer version than the pinned spec.

Related Issue

Fixes #44404

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/lazy_deps.py: Changed active_features() to call _is_satisfied() instead of _is_present(). Updated docstring to reflect version-aware behavior.
  • tests/tools/test_lazy_deps.py: Updated existing TestActiveFeatures tests to mock _is_satisfied instead of _is_present. Added two new regression tests verifying that packages at mismatched versions are not counted as active.

How to Test

  1. Run pytest tests/tools/test_lazy_deps.py -v — all 63 tests should pass
  2. To verify the bug manually: install aiohttp at a version newer than the pinned spec, then run hermes update — without this fix, refresh_active_features() would downgrade aiohttp; with this fix, the feature is not reported as active and no downgrade occurs

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

…nt unnecessary downgrades

active_features() used _is_present() (presence-only, ignoring version) to
detect which features the user has activated.  If a dependency like aiohttp
3.14.1 is installed by another package but the lazy-dep spec pins
aiohttp==3.13.4, the feature was reported as active and refresh_active_features()
triggered a pip downgrade to 3.13.4.

Switch to _is_satisfied() so only features whose installed versions actually
match the spec range are considered active.  This prevents hermes update
from downgrading working newer versions to the pinned version.
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for tracing the false-positive path. Current main does reproduce it: tools/lazy_deps.py:870 uses presence-only selection, then refresh_active_features() at tools/lazy_deps.py:889-901 can call ensure() for a version-mismatched spec.

Problems

  • Switching the selector to _is_satisfied() fixes the transitive false positive, but it also skips refresh for a feature the user actually lazy-installed whenever a pin changes. That conflicts with the existing contract in tools/lazy_deps.py:513-516 and the update path's stated CVE/pin-refresh purpose in hermes_cli/main.py:7619-7625.
  • The new tests in d8997e6795b6 mock _is_satisfied() false. They verify the implementation hook, but do not model a real mismatched distribution or distinguish transitive presence from prior feature activation.

Suggested changes

  • Record successful lazy-feature activation and drive update refresh from that record rather than from installed package names.
  • Cover both a mismatched unactivated transitive dependency (no refresh) and a recorded feature after a pin bump (refresh).

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@falkoro

falkoro commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

I ran this branch against a reproduction of the false-activation bug (#58458) alongside the other five open PRs on active_features(), and I think this one needs a rework rather than a merge — it removes the false positives, but by disabling detection of features that really are installed.

Linux / py3.12, main at 760112a, in a venv where Slack and the dashboard were genuinely lazy-installed:

main      active=8   false-positives=4   true-positives-lost=none
this PR   active=2   false-positives=0   true-positives-lost=[platform.slack, tool.dashboard]

The mechanism, printing _is_present vs _is_satisfied for each pin:

platform.slack   pin=slack-bolt==1.27.0        installed=1.29.0   present=True  satisfied=False
platform.slack   pin=slack-sdk==3.40.1         installed=3.43.0   present=True  satisfied=False
platform.slack   pin=aiohttp==3.13.4           installed=3.14.1   present=True  satisfied=False
tool.dashboard   pin=fastapi==0.133.1          installed=0.139.0  present=True  satisfied=False
tool.dashboard   pin=uvicorn[standard]==0.41.0 installed=0.51.0   present=True  satisfied=False
tool.dashboard   pin=starlette==1.0.1          installed=1.3.1    present=True  satisfied=False

Every installed version is newer than its pin, so _is_satisfied returns False and both features fall out of active_features().

That drift is not an odd venv state — it is the normal state, and it's the one that matters. active_features() feeds refresh_active_features(), whose job (per its docstring) is "to figure out which lazy backends need a refresh pass when pins move". A version-aware gate therefore reports a feature as inactive precisely when its pins have moved, i.e. exactly when the refresh should run. hermes update would then silently skip the backends that most need updating, and the more stale an install is, the more features go quiet. The false positives disappear as a side effect of that, not because the shared-dependency problem is addressed: aiohttp still appears in four features' spec lists, so the moment a user's aiohttp happens to match its pin, discord/matrix/teams activate again.

Worth saying that the version-awareness idea is independently useful — "installed but stale" is real information. It just belongs on the refresh decision rather than the activation decision. refresh_active_features() already asks feature_missing(feature) and reports current; making that comparison version-aware would give you "stale pins, reinstall" without ever hiding an installed feature. That's compatible with either of the two designs that do fix the false-activation (specs[0] sentinel in #54178/#65744, computed-distinctive in #67930/#70590), so it could land as a complement to whichever of those merges instead of competing with them.

Happy to be corrected if the intent was narrower than I've read it — and for transparency, I have an open PR on this same file (#60797, the huggingface-hub range); it doesn't modify active_features(), and it merges clean with this branch. Full six-way comparison is on #58458 so it isn't scattered across threads.

@teknium1

Copy link
Copy Markdown
Collaborator

Fixed on main by #72361 (anchor-gated active_features(), salvaged from earliest submitter #27878 by @paralegalia). Your version-aware variant identified the same false-activation mechanism — the anchor approach was chosen because _is_satisfied()-based activation would treat a legitimately-installed-but-newer backend as 'inactive' and skip refreshing it after a pin bump. The downgrade half of your report is covered by the new lockstep invariant (every shared LAZY_DEPS pin must equal uv.lock's version). Thanks @liuhao1024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(lazy-deps): active_features() presence-only check causes false-positive activation and unnecessary downgrades

4 participants