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

[v2.0] Stop reading OmopEmbConfig deep inside EmbeddingClient and resolve_backend#50
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
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