Skip to content

Refactor: remove config reads from property getters and factory functions #45

Description

@nicoloesch

Problem

OmopEmbConfig.get_config() is called inside property getters, method bodies, and a backend factory function rather than at system entry points. Two of the three call sites already have silent exception catches masking the coupling, which constitutes the original workaround for the same underlying problem.

Specific locations:

src/omop_emb/embeddings/embedding_client.py:145

try:
    cfg_dim = OmopEmbConfig.get_config().embedding_dim
except FileNotFoundError:
    cfg_dim = None

Config is read inside a property getter (embedding_dim) with a FileNotFoundError catch to silently fall back when no config file exists. The catch is a bandaid: it hides the fact that this code path should not be reading config at all.

src/omop_emb/embeddings/embedding_client.py:284

try:
    cfg = OmopEmbConfig.get_config()
    document_embedding_prefix = cfg.document_embedding_prefix
    query_embedding_prefix = cfg.query_embedding_prefix
except Exception:
    document_embedding_prefix = ""
    query_embedding_prefix = ""

A bare except Exception: swallows any config error, including ConfigurationError (missing required resources). The embedding prefixes silently default to "" whenever the config is misconfigured, making misconfiguration undetectable.

src/omop_emb/backends/base_backend.py:977

cfg = OmopEmbConfig.get_config()
if backend_type is None:
    backend_str = cfg.backend

Config is read inside a backend factory function when backend_type is not supplied. The same pattern as the paths.py issue in omop-graph: optional parameters resolve to config at the wrong layer.

Root cause

Config values that should be resolved once at construction time are instead pulled on demand from the file system inside methods and property getters. The except catches convert config errors into silent wrong behaviour rather than surfacing them at the point where the system is misconfigured.

Proposed direction

  1. EmbeddingClient should accept embedding_dim, document_embedding_prefix, and query_embedding_prefix as constructor parameters. The factory that builds it reads config once and passes values in. Property getters become simple attribute lookups.
  2. The backend factory function should require backend_type explicitly, or the caller that omits it resolves it from config before calling. Remove the inline get_config() from the factory body.
  3. Remove all except FileNotFoundError / except Exception catches that were added to work around the missing-config case. Misconfiguration should be an explicit error at startup, not a silent fallback in a property getter.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions