Skip to content

[v2.0] Stop reading OmopEmbConfig deep inside EmbeddingClient and resolve_backend - #50

Closed
nicoloesch wants to merge 2 commits into
mainfrom
45-remove-config-reads
Closed

nicoloesch wants to merge 2 commits into
mainfrom
45-remove-config-reads

Conversation

@nicoloesch

Copy link
Copy Markdown
Collaborator

OmopEmbConfig.get_config() was being called deep inside EmbeddingClient's constructor/embedding_dim property and inside resolve_backend(), instead of once at CLI entry points. Two of the three call sites silently swallowed config errors (except FileNotFoundError / bare except Exception) rather than surfacing misconfiguration. Config is now resolved exactly once per CLI command and threaded down as plain arguments:

  • EmbeddingClient gains embedding_dim, document_embedding_prefix, query_embedding_prefix constructor parameters; property getters no longer read config internally.
  • resolve_backend(backend_type, *, sqlite_path=None) no longer reads config at all; a new resolve_backend_from_config(cfg) is the one place backend selection touches OmopEmbConfig.
  • New load_omop_emb_config() also catches ConfigurationError (missing required resource), previously uncaught anywhere and left to propagate as a raw ValueError.

@nicoloesch nicoloesch added the breaking Incompatible API change. MAJOR: x+1.y.z label Jul 22, 2026

@gkennos gkennos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

configuration.md and interface_guide.md still show resolve_backend with no args

docs should switch to load_omop_emb_config() + resolve_backend_from_config(cfg), or show explicit resolve_backend with new args (both since both public apis?)

@nicoloesch

Copy link
Copy Markdown
Collaborator Author

Closing in favor of absorbing this into #53 , which is already restructuring resolve_backend()/OmopEmbConfig in the same area.

The resolve_backend()/resolve_backend_from_config() split here is being adopted as designed, including the docs fix flagged in review (configuration.md/interface_guide.md). Landing together rather than as two separate breaking releases back to back.

@nicoloesch nicoloesch closed this Aug 7, 2026
@nicoloesch
nicoloesch deleted the 45-remove-config-reads branch September 16, 2026 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Incompatible API change. MAJOR: x+1.y.z

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: remove config reads from property getters and factory functions

2 participants