Skip to content

fix(packaging): add keyring to base install_requires so gaia connectors works on bare install (#1621)#1624

Merged
itomek merged 2 commits into
mainfrom
fix/issue-1621-keyring-base-dep
Jun 12, 2026
Merged

fix(packaging): add keyring to base install_requires so gaia connectors works on bare install (#1621)#1624
itomek merged 2 commits into
mainfrom
fix/issue-1621-keyring-base-dep

Conversation

@itomek

@itomek itomek commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Closes #1621

Why this matters

gaia connectors is a base CLI command, but keyring — its OS credential-store backend for OAuth tokens (#915) — shipped only in the [ui]/[api]/[dev] extras. So a bare pip install amd-gaia crashed gaia connectors list/status with a raw ModuleNotFoundError. This promotes keyring to base install_requires, matching the command's tier, so connectors works out of the box on every platform.

Decision record (#1621, Option 1). keyring is featherweight (pure-Python + jaraco.*; the cryptography/SecretStorage chain is Linux-only and ships as a prebuilt wheel) and base already pulls transformers/accelerate, so the added footprint is negligible — whereas a [connectors] extra (Option 2) would leave a base command needing an extra to avoid crashing. The guarded import in #1622 ships as defense-in-depth for stripped/minimal installs; the extras keep their own keyring declaration (so #1617's [api] guard stays green).

Test plan

  • pytest tests/unit/test_base_keyring_dep.py — new regression guard: base install_requires declares keyring
  • pytest tests/unit/test_api_extras.py — no regression; extras keep keyring
  • util/lint.py --all
  • Real-world: bare pip install pulls keyring + gaia connectors list/status works on macOS / Windows / Linux; guarded actionable error when keyring absent

…rs works on bare install (#1621)

gaia connectors (list, status, …) is registered as a base CLI command — no
[ui]/[api] gate — but its import chain (cli → gaia.connectors.store) reaches
`import keyring` at module load time and at request time for OAuth token storage
(#915). Because keyring was only declared in the [ui], [api], and [dev] extras,
a plain `pip install amd-gaia` shipped a broken `gaia connectors` command that
crashed with ModuleNotFoundError. Promoting keyring to install_requires is the
correct tier match: the command is base, so its dependency must be too.
keyring is featherweight (pure-Python; only jaraco.* transitive deps; the
cryptography/SecretStorage chain is prebuilt or Linux-only), so the footprint
cost is negligible against the transformers/accelerate base already declares.
The extras ([ui], [api], [dev]) keep their own declarations independently —
this is an addition to base, not a move. Defense-in-depth guarded-import error
for the same issue lands separately in #1622.
@github-actions github-actions Bot added dependencies Dependency updates tests Test changes labels Jun 12, 2026
@itomek itomek marked this pull request as ready for review June 12, 2026 13:41
@itomek itomek requested a review from kovtcharov-amd as a code owner June 12, 2026 13:41
@github-actions

Copy link
Copy Markdown
Contributor

Review: fix(packaging): add keyring to base install_requires (#1624)

Approve. This correctly fixes a real crash: gaia connectors list/status is a base CLI command whose import chain hits an unguarded import keyring (src/gaia/connectors/store.py:38), yet keyring previously shipped only in the [ui]/[api]/[dev] extras — so a bare pip install amd-gaia died with ModuleNotFoundError. The one-line dependency add plus a static regression guard is exactly the right fix at the right tier, and it cleanly mirrors the [api] precedent from #1617.

Issues

None blocking. Two small observations, neither requiring a change:

🟢 Test relies on regex-parsing setup.py (tests/unit/test_base_keyring_dep.py:51). Line-walking the file is inherently more brittle than importing the metadata, but it's a deliberate, documented choice — it runs in the CI unit venv that never installs the package, and it's identical to the established test_api_extras.py pattern. Consistency wins here; no change needed.

🟢 PR description references a "guarded import in #1622" as defense-in-depth. Worth confirming that follow-up actually lands — today store.py:38 imports keyring unconditionally, so this PR's install_requires entry is the only thing standing between a stripped install and a crash. That's fine and intended, just flagging that the safety net is singular until #1622 merges.

Strengths

  • Right fix at the right tier — promotes the dep to match the command's base tier rather than papering over it with a [connectors] extra that would leave a base command needing an opt-in to not crash. The decision record in the PR body justifies this well.
  • Version pin is consistentkeyring>=24.0.0,<26.0.0 exactly matches all four other declarations in setup.py ([api], [ui], [dev]), so there's no resolver-conflict surface.
  • Regression guard with an actionable failure message — the test names the import chain and the issue (feat: Google account connection for AgentUI (OAuth via PKCE) #915) in its assertion, so a future dependency-pruning change that drops keyring fails loudly with a fix-it pointer rather than silently.
  • Verified locally: keyring import at store.py:38 is unguarded at module load, and pytest tests/unit/test_base_keyring_dep.py tests/unit/test_api_extras.py → 3 passed.

Verdict

Approve — ready to merge. Scope-clean (one dep line + one test), no security or breaking-change surface, no docs impact (dependency-only, no new CLI/SDK surface). No injection content observed in the diff.

@itomek itomek enabled auto-merge June 12, 2026 13:45

@itomek-amd itomek-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving the base-dependency fix for #1621. gaia connectors is a base CLI command, so promoting keyring to base install_requires is the correct resolution — it makes connectors work on a bare pip install while the guarded import (#1622) remains as defense-in-depth.

Validated end-to-end before merge:

  • Unit: new test_base_keyring_dep.py (red→green) + test_api_extras.py no regression; util/lint.py --all clean.
  • Bare-install proof: pip install -e . (no extras) pulls keyring 25.7.0 from base.
  • Real-world across platforms: a bare install resolves keyring and gaia connectors list/status works on macOS (Keychain) and Linux+SecretService; on headless Linux/Docker the no-backend case fails with a clean actionable error rather than a traceback.

The extras retain their own keyring declaration, so #1617's [api] guard stays green.

@itomek itomek added this pull request to the merge queue Jun 12, 2026
Merged via the queue into main with commit fb76ccd Jun 12, 2026
62 checks passed
@itomek itomek deleted the fix/issue-1621-keyring-base-dep branch June 12, 2026 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(packaging): gaia connectors (base command) crashes without keyring — not in base deps

2 participants