Skip to content

Fix: raise an actionable ImportError when sqlalchemy is missing, instead of a guard that does nothing - #167

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/honest-sqlalchemy-import-guard
Open

Fix: raise an actionable ImportError when sqlalchemy is missing, instead of a guard that does nothing#167
AmaadMartin wants to merge 2 commits into
mainfrom
fix/honest-sqlalchemy-import-guard

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: sessions/database_session_service.py wraps its sqlalchemy imports in except ImportError: pass, but the schema and migration modules it imports below need sqlalchemy too. The guard buys nothing: the module has never been importable without the db extra. It only hides the requirement, so import google.adk.sessions.database_session_service and both CLI database paths report a bare No module named 'sqlalchemy'. sessions/migration/_schema_check_utils.py has the same guard and leaves itself half-loaded, so its functions die later with NameError: name 'create_sync_engine' is not defined.

Solution: Both guards now raise missing_extra("sqlalchemy", "db") from the original error. That is the message sessions/__init__.py already produces for this module, so every entry point converges on one line: pip install google-adk[db]. sqlalchemy stays an optional extra; with it installed the except branch never runs and behaviour is unchanged.

The raise makes two older guards dead, so the second commit removes them. DatabaseSessionService.__init__ re-checked import sqlalchemy, which the module import now settles. sessions/__init__.py special-cased DatabaseSessionService to wrap an ImportError from the module, which both duplicated the exception chain and relabelled any unrelated ImportError as a missing db extra; DatabaseSessionService joins _LAZY_MEMBERS instead.

Collision check: I listed the 100 open PRs on the fork and read the changed-file list of each. Only #151 touches a file I touch (tests/unittests/test_optional_dependencies.py), in a different section, and it does not change either production file. No PR overlaps this change.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

pytest tests/unittests/test_optional_dependencies.py tests/unittests/sessions \
  tests/unittests/test_import_loading.py tests/unittests/cli/test_service_registry.py \
  tests/unittests/cli/utils/test_service_factory.py tests/unittests/cli/utils/test_local_storage.py -q
394 passed, 6 skipped

Two new tests cover the two new except bodies, which is every new line and both directions of both branches. The sqlalchemy-present direction is covered by all 394 tests above. The second commit only deletes code, so it adds no new lines to cover.

Mutation check. I restored both except ImportError: pass guards from main and re-ran the file. Both new tests fail:

  • test_database_session_service_import_reports_missing_extra: Failed: DID NOT RAISE ImportError. Run on its own it fails differently, with Regex pattern did not match. Actual message: 'import of sqlalchemy halted; None in sys.modules'. That split is the bug: once an earlier test has imported the schema modules, the old guard swallows the sqlalchemy failure and the module imports clean.
  • test_schema_check_utils_import_reports_missing_extra: Failed: DID NOT RAISE ImportError

The other five tests in the file still pass on the restored guards. That includes test_database_session_service_fails_on_creation, which asserts only "sqlalchemy" in str(exc). "sqlalchemy" is in the broken message too, so that test cannot see this bug. The new tests match google-adk[db] for that reason.

Manual End-to-End (E2E) Tests:
Build a core-only environment and check each entry point.

uv venv --python 3.11 /tmp/core
uv pip install --python /tmp/core/bin/python .        # no extras

/tmp/core/bin/python -c "import google.adk.sessions.database_session_service"
/tmp/core/bin/python -c "from google.adk.sessions import DatabaseSessionService"
/tmp/core/bin/python -c "
from google.adk.cli.service_registry import get_service_registry
get_service_registry().create_session_service('postgresql://u:p@h/db')"
/tmp/core/bin/python -c "
from google.adk.sessions.migration import _schema_check_utils
_schema_check_utils.get_db_schema_version('sqlite:///x.db')"

All four print ImportError: The 'sqlalchemy' package is required to use this feature. Please install it by running: pip install google-adk[db]. Before this change the first three printed ModuleNotFoundError: No module named 'sqlalchemy' and the fourth printed NameError: name 'create_sync_engine' is not defined.

adk migrate session in the same environment prints Migration failed: The 'sqlalchemy' package is required to use this feature. Please install it by running: pip install google-adk[db].

Core install is unaffected: import google.adk and InMemorySessionService still work, and google.adk.sessions.Nope still raises AttributeError.

The accessor chain is now one wrap deep, not two: ImportError: ... pip install google-adk[db] with __cause__ set to ModuleNotFoundError: No module named 'sqlalchemy'.

Other checks on the pushed commit: isort --check-only, pyink --check and mypy on the changed files. mypy reports the same single pre-existing error before and after, so this adds no new error to the CI baseline.

CI on the first commit: Unit Tests and Mypy Check passed on Python 3.10 through 3.14, and so did the A2A v0.3 tests. Pre-commit Linter fails, for a reason unrelated to this change: the update-constraints hook regenerates constraints-3.*.txt from live PyPI and rewrites them on every run. That hook fails the same way on PRs that touch neither pyproject.toml nor the constraints files, such as #165, #164 and #162, and PRs #163 and #110 already propose fixes for it. This change touches no dependency file, and pre-commit run --files on the four changed files passes every hook.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 7, 2026 17:47
…bsent

`database_session_service` wrapped its sqlalchemy imports in
`except ImportError: pass`, but the schema and migration modules it imports
below need sqlalchemy too. The guard bought nothing: the module has never been
importable without the `db` extra. It only hid the requirement, so
`import google.adk.sessions.database_session_service` and the CLI database and
migration paths reported a bare `No module named 'sqlalchemy'`.

Replace the swallow with `raise missing_extra("sqlalchemy", "db") from e`, the
message `sessions/__init__.py` already produces for this module. Apply the same
change to `migration/_schema_check_utils.py`, which today leaves itself
half-initialised and fails later with a `NameError`.

`sqlalchemy` stays an optional extra. With it installed, behaviour is unchanged.
…ise made dead

`DatabaseSessionService.__init__` re-checked `import sqlalchemy`, and
`sessions/__init__.py` special-cased `DatabaseSessionService` to wrap an
ImportError from the module. The module now raises at import time, so neither
can run with sqlalchemy absent.

The `__init__.py` branch also relabelled any unrelated ImportError from the
module as a missing `db` extra. `DatabaseSessionService` joins `_LAZY_MEMBERS`
instead, so the accessor reports what actually failed. The missing-sqlalchemy
message is unchanged, and its `__cause__` is now the original
ModuleNotFoundError rather than a second copy of itself.
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.

1 participant