Skip to content

Remove redundant config re-resolution in doctor diagnostics #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nicoloesch wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove redundant config re-resolution in doctor diagnostics #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
874cf29
Remove redundant config re-resolution in doctor diagnostics
nicoloesch Jul 22, 2026
13a3496
Solve Liksov substitution
nicoloesch Jul 22, 2026
e9ea50a
Revert LSP fix, pin ty version
nicoloesch Jul 23, 2026
42130b9
More specific error capture, make schema_doctor backwards compatible
nicoloesch Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion omop_alchemy/cdm/handlers/timeline/event_timeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ class Condition_Event(Condition_Occurrence, ClinicalEvent):
)


class Measurement_Event(ClinicalEvent, Measurement): # type: ignore[misc]
class Measurement_Event(ClinicalEvent, Measurement):

_mapping = EventMapping(
concept_field="measurement_concept_id",
Expand Down
4 changes: 3 additions & 1 deletion omop_alchemy/maintenance/cli_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ def doctor_command(
) -> None:
"""Run a read-only maintenance health check across connection readiness, schema drift, and FK state."""
with console.status("Running maintenance doctor checks..."):
report = collect_doctor_report(vocabulary_included=vocabulary_included, deep=deep)
report = collect_doctor_report(
engine=engine, vocabulary_included=vocabulary_included, deep=deep
)
console.print(render_info_environment(report.info))
console.print(render_info_database(report.info))
console.print(render_doctor_checks(report.checks))
Expand Down
182 changes: 105 additions & 77 deletions omop_alchemy/maintenance/cli_schema_doctor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

from __future__ import annotations

import warnings
from dataclasses import dataclass

import sqlalchemy as sa

from omop_alchemy.backends.resolve import SupportedDialect

from .cli_foreign_keys import (
Expand Down Expand Up @@ -153,34 +156,58 @@ def _build_recommendations(

def collect_doctor_report(
*,
engine: sa.engine.Engine | None = None,
vocabulary_included: bool = True,
deep: bool = False,
) -> DoctorReport:
"""Run all maintenance health checks and return a prioritised report with recommendations."""
info = collect_maintenance_info(vocabulary_included=vocabulary_included)

checks = [
DoctorCheck(
name="connection",
status="passed" if info.connection_ready else "failed",
detail=(
"Target database connection succeeded."
if info.connection_ready
else info.connection_error or info.engine_error or "Connection could not be established."
),
"""Run all maintenance health checks and return a prioritised report with recommendations.

Parameters
----------
engine : sa.engine.Engine, optional
Already-resolved CDM engine (e.g. from the ``@omop_command`` decorator),
reused for the deep checks below instead of re-resolving config.

.. deprecated::
Omitting this and letting the function resolve its own engine is
deprecated and will be required in 2.0. Pass the already-resolved
engine instead.
"""
owns_engine = engine is None
if owns_engine:
warnings.warn(
"collect_doctor_report() without an explicit engine= is deprecated "
"and will be required in 2.0. Pass the already-resolved engine "
"(e.g. from @omop_command) instead of letting this function "
"re-resolve config a second time.",
DeprecationWarning,
stacklevel=2,
)
]

reconciliation: SchemaReconciliationReport | None = None
foreign_key_status: tuple[ForeignKeyStatusResult, ...] | None = None
foreign_key_validation: ForeignKeyValidationReport | None = None

if info.connection_ready:
from omop_alchemy.config import create_cdm_engine, get_cdm_context
_, resolved = get_cdm_context()
engine = create_cdm_engine(resolved)
db_schema = resolved.cdm_schema
try:

try:
info = collect_maintenance_info(vocabulary_included=vocabulary_included)

checks = [
DoctorCheck(
name="connection",
status="passed" if info.connection_ready else "failed",
detail=(
"Target database connection succeeded."
if info.connection_ready
else info.connection_error or info.engine_error or "Connection could not be established."
),
)
]

reconciliation: SchemaReconciliationReport | None = None
foreign_key_status: tuple[ForeignKeyStatusResult, ...] | None = None
foreign_key_validation: ForeignKeyValidationReport | None = None

if info.connection_ready:
db_schema = info.db_schema
missing_table_count = info.missing_table_count or 0
checks.append(
DoctorCheck(
Expand Down Expand Up @@ -286,68 +313,69 @@ def collect_doctor_report(
detail="Foreign key validation is only available on PostgreSQL.",
)
)
finally:
engine.dispose()
else:
checks.extend(
(
DoctorCheck(
name="managed tables",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="foreign keys",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="schema drift",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="foreign key validation",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
else:
checks.extend(
(
DoctorCheck(
name="managed tables",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="foreign keys",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="schema drift",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
DoctorCheck(
name="foreign key validation",
status="skipped",
detail="Skipped because the database connection is not ready.",
),
)
)
)

if info.backend == SupportedDialect.POSTGRESQL:
backup_tools_ready = info.pg_dump_path is not None and (
info.pg_restore_path is not None or info.psql_path is not None
)
checks.append(
DoctorCheck(
name="backup tooling",
status="passed" if backup_tools_ready else "warning",
detail=(
"PostgreSQL backup and restore client tools are available."
if backup_tools_ready
else "PostgreSQL client tools are incomplete on this machine."
),
if info.backend == SupportedDialect.POSTGRESQL:
backup_tools_ready = info.pg_dump_path is not None and (
info.pg_restore_path is not None or info.psql_path is not None
)
)
else:
checks.append(
DoctorCheck(
name="backup tooling",
status="skipped",
detail="Backup and restore tooling checks are only relevant for PostgreSQL targets.",
checks.append(
DoctorCheck(
name="backup tooling",
status="passed" if backup_tools_ready else "warning",
detail=(
"PostgreSQL backup and restore client tools are available."
if backup_tools_ready
else "PostgreSQL client tools are incomplete on this machine."
),
)
)
else:
checks.append(
DoctorCheck(
name="backup tooling",
status="skipped",
detail="Backup and restore tooling checks are only relevant for PostgreSQL targets.",
)
)
)

return DoctorReport(
info=info,
checks=tuple(checks),
recommendations=_build_recommendations(
return DoctorReport(
info=info,
checks=tuple(checks),
recommendations=_build_recommendations(
info=info,
reconciliation=reconciliation,
foreign_key_status=foreign_key_status,
foreign_key_validation=foreign_key_validation,
),
reconciliation=reconciliation,
foreign_key_status=foreign_key_status,
foreign_key_validation=foreign_key_validation,
),
reconciliation=reconciliation,
foreign_key_status=foreign_key_status,
foreign_key_validation=foreign_key_validation,
)
)
finally:
if owns_engine:
engine.dispose()
16 changes: 9 additions & 7 deletions omop_alchemy/maintenance/cli_schema_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import shutil

import sqlalchemy as sa
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.exc import ArgumentError, SQLAlchemyError

from oa_configurator import Resolver, load_stack_config
from oa_configurator.loader import DEFAULT_CONFIG_PATH
Expand Down Expand Up @@ -347,13 +347,15 @@ def collect_maintenance_info(
raw_url = sa.engine.make_url(resolved.database.url)
engine_url = raw_url.render_as_string(hide_password=True)
backend = raw_url.get_backend_name()
from omop_alchemy.config import create_cdm_engine
engine = create_cdm_engine(resolved)
engine_created = True
except RuntimeError as exc:
engine_error = str(exc)
except Exception as exc:
except (FileNotFoundError, ValueError, ArgumentError, KeyError) as exc:
engine_error = f"Could not resolve engine configuration: {exc}"
else:
from omop_alchemy.config import create_cdm_engine
try:
engine = create_cdm_engine(resolved)
engine_created = True
except RuntimeError as exc:
engine_error = str(exc)

if engine is not None:
try:
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ dev = [
"requests>=2.33.0",
"pytest>=9.0.3",
"pytest-cov>=4.0",
"ty>=0.0.59",
"ty==0.0.61",
"ruff>=0.4",
"mkdocs-material>=9.7.1",
"mkdocstrings-python>=2.0.1",
Expand Down
Loading
Loading