Repository navigation
LCORE-4710: Implement registry initialization - #3124
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 minutes. View limit details
WalkthroughThe application lifespan now initializes configured inference providers before OGX client setup. The registry logs the registered provider count and IDs. Unit tests cover successful registration and model discovery failure. ChangesInference provider startup
Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Lifespan as FastAPI lifespan
participant Registry as ProviderRegistry
participant OpenAI as OpenAI provider
Lifespan->>Registry: initialize(configuration.inference)
opt Model discovery enabled
Registry->>OpenAI: list models
OpenAI-->>Registry: models or ModelDiscoveryError
end
Possibly related PRs
Suggested labels: Suggested reviewers: Merge Risk:
|
|
🤖 Finished Review · Commit: Runtime: pi · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.28 This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
|
🤖 Review · Commit: This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
|
🤖 Finished Review · Commit: Runtime: pi · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.45 This review was superseded by a newer commit. Check the current pull request HEAD's review status before merging. |
| self._rerankers[provider_model_id] = model | ||
| else: | ||
| self._embeddings[provider_model_id] = model | ||
| self._embeddings[provider_model_id] = cast(EmbeddingModel, model) |
There was a problem hiding this comment.
Consequence of ST being added as a main dependency
| # Used for Pydantic AI | ||
| "pydantic-ai==2.38.0", | ||
| "pydantic-ai-skills>=0.11.0,<2.0.0", | ||
| # Used by the sentence-transformers inference provider (embedding / rerank) |
There was a problem hiding this comment.
We need ST to be main dependency
|
🤖 Finished Review · ✅ Success · Started 9:14 AM UTC · Completed 9:32 AM UTC Commit: Runtime: pi · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.55 |
|
<!-- fullsend:risk-assessment --> Details14 files with 264 lines across inference provider modules. Low test ratio is the only elevated signal. Moderate churn on pyproject.toml. Weighted composite 2. |
ReviewFindingsMedium
Low
Labels: New feature PR adding inference provider registry initialization. Next steps:
|
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
src/providers/inference/registry.py(file-level): Line 62 ·
[low] edge-case
ProviderRegistry.initialize() has no cleanup on partial failure. If the Nth provider fails during await provider.initialize(), previously-initialized providers remain in _providers, leaving the singleton in a half-initialized state. In production the service aborts on failure so this is currently benign, but the contract is fragile.
Suggested fix: Clear self._providers at the top of initialize() (e.g., self._providers.clear()) so the method is idempotent and any retry starts from a clean slate.
| # Used for Pydantic AI | ||
| "pydantic-ai==2.38.0", | ||
| "pydantic-ai-skills>=0.11.0,<2.0.0", | ||
| # Used by the sentence-transformers inference provider (embedding / rerank) |
There was a problem hiding this comment.
[medium] scope-bundling
The PR promotes sentence-transformers (which transitively pulls in PyTorch/torch) from the ogxlibdev dev dependency group to a main runtime dependency. This is a significant deployment-impacting change (container image size, startup time, memory footprint) that is bundled with the registry wiring work without separate justification or discussion in the PR description.
Suggested fix: Document in the PR description why sentence-transformers must become a main dependency, or split the dependency promotion into a separate PR with its own rationale and impact analysis.
|
🤖 Finished Retro · ✅ Success · Started 12:03 PM UTC · Completed 12:14 PM UTC Commit: Runtime: pi · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.95 |
Retro: PR #3124 — LCORE-4710: Implement registry initializationTimeline
Total review agent cost: ~$10.28 (3 cancelled runs: ~$4.73 wasted; 1 successful run: $5.55). Evidence for existing issues
Autonomy assessmentThe human reviewer approved with "LGTM" and no inline comments. Per the autonomy-readiness methodology, the delta between agent and human review is inconclusive — silent approval means the human found nothing to flag, but we cannot infer that the agent matched or exceeded human review from this single PR. Proposals filed
|
Description
Wire inference provider registry initialization into FastAPI lifespan so configured providers are built and their catalogs loaded before the service serves traffic.
Integration tests for config will be supplied in another PR.
Type of change
pyproject.toml+uv.lock]requirements.*.txtfor Konflux]Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Unit
test_lifespan_initializes_inference_registry— OpenAI provider withskip_model_discovery=True; after lifespan startup, registry lists the provider and catalog modelchat.test_lifespan_propagates_model_discovery_error— discovery enabled;AsyncModels.listraisesAPIConnectionError; lifespan raisesModelDiscoveryErrorand leaves the registry empty.Summary by CodeRabbit