fix: defer provider binding during cold start#176
Merged
Conversation
Contributor
Reviewer's GuideImplements deferred and retriable provider binding for AstrBot v2 integration to avoid noisy lookup warnings during cold start, adds shared provider registry inspection utilities used by embedding/rerank factories, wires these retries into startup/warmup/runtime paths, and bumps plugin metadata to version 3.1.6. Sequence diagram for deferred provider binding during cold startsequenceDiagram
actor AstrBotCore
participant PluginLifecycle
participant V2Integration
participant EmbeddingFactory
participant ProviderRegistry
participant FrameworkContext
AstrBotCore->>PluginLifecycle: _delayed_provider_reinitialization()
PluginLifecycle->>V2Integration: refresh_provider_bindings(force=True)
V2Integration->>EmbeddingFactory: create(config, context)
EmbeddingFactory->>ProviderRegistry: normalize_provider_id(config.embedding_provider_id)
EmbeddingFactory->>ProviderRegistry: collect_framework_providers(context, EmbeddingProvider)
ProviderRegistry-->>EmbeddingFactory: providers, inspected=True, errors
alt [providers empty]
EmbeddingFactory->>ProviderRegistry: framework_registry_has_any_provider(context)
ProviderRegistry-->>EmbeddingFactory: registry_has_any_provider=False
EmbeddingFactory-->>V2Integration: None
V2Integration-->>PluginLifecycle: refreshed=False
else [provider visible]
EmbeddingFactory->>ProviderRegistry: find_provider_by_id(providers, provider_id)
ProviderRegistry-->>EmbeddingFactory: provider
EmbeddingFactory->>EmbeddingFactory: _wrap_provider(provider_id, provider)
EmbeddingFactory-->>V2Integration: embedding_provider
V2Integration->>V2Integration: refresh_provider_bindings()
V2Integration-->>PluginLifecycle: refreshed=True
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
V2LearningIntegration.refresh_provider_bindingsthetime.monotonic()/interval guard is duplicated both before and inside the lock; consider extracting this into a small helper or performing the check only inside the lock to reduce repetition and potential drift. - The hard-coded
_provider_retry_intervalof 10 seconds inV2LearningIntegrationmay not suit all deployments; consider making this configurable via the existing config object or constructor parameter so operators can tune retry aggressiveness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `V2LearningIntegration.refresh_provider_bindings` the `time.monotonic()`/interval guard is duplicated both before and inside the lock; consider extracting this into a small helper or performing the check only inside the lock to reduce repetition and potential drift.
- The hard-coded `_provider_retry_interval` of 10 seconds in `V2LearningIntegration` may not suit all deployments; consider making this configurable via the existing config object or constructor parameter so operators can tune retry aggressiveness.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
51a5cbf to
b7a506d
Compare
b7a506d to
6f7ac82
Compare
Collaborator
Author
|
本 PR 已完成本地审查和 CI:Python syntax、Ruff、commitlint、Sourcery review 均通过。Sourcery 的两条建议也已处理:Provider 重试间隔现在可配置,重复的 retry interval 判断已抽成 helper。当前仅剩 main 分支规则要求 1 个 approving review,批准后即可 rebase merge 并发布 3.1.6。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
python -m py_compile services\provider_registry.py services\embedding\factory.py services\reranker\factory.py services\core_learning\v2_learning_integration.py core\plugin_lifecycle.py tests\unit\test_provider_registry_rebind.pygit diff --check -- core/plugin_lifecycle.py services/core_learning/v2_learning_integration.py services/embedding/factory.py services/reranker/factory.py services/provider_registry.py tests/unit/test_provider_registry_rebind.py metadata.yaml __init__.py README.md README_EN.mdpython -m pytest -q tests\unit\test_provider_registry_rebind.py tests\integration\test_package_imports.py tests\integration\test_config_blueprint.py(11 passed)Pre-review
Fixes #171
Summary by Sourcery
Defer binding of embedding and rerank providers until AstrBot provider registries are ready, and add retry-based rebinding across the v2 learning lifecycle.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: