feat(lfx): add adapter registries#11990
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes introduce a service-scoped adapter registry system enabling dynamic discovery and management of swappable service implementations from decorators, entry points, and TOML configuration files, with thread-safe singleton lifecycle management and comprehensive public APIs for adapter registration and retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Decorator
participant Registry as AdapterRegistry
participant EntryPoints as Entry Points Loader
participant Config as Config Loader
participant Factory
participant Adapter as Adapter Instance
Client->>Registry: get_adapter_registry(adapter_type)
activate Registry
Note over Registry: Check global registry cache
alt Registry exists
Registry->>Client: return cached registry
else Registry not cached
Registry->>Registry: create new AdapterRegistry
Registry->>Client: return new registry
end
deactivate Registry
Client->>Decorator: `@register_adapter`(type, key)
Decorator->>Registry: register_class(key, adapter_class, override=True)
activate Registry
Registry->>Registry: store adapter_class
deactivate Registry
Client->>Registry: discover(config_dir)
activate Registry
rect rgba(100, 150, 200, 0.5)
Note over EntryPoints: Entry points discovery
Registry->>EntryPoints: load from entry_point_group
EntryPoints->>Registry: register_class(key, class, override=False)
end
rect rgba(150, 100, 200, 0.5)
Note over Config: Config file discovery
Registry->>Config: load lfx.toml or pyproject.toml
Config->>Registry: register_class(key, class, override=True)
end
Registry->>Registry: mark discovered=True
deactivate Registry
Client->>Registry: get_instance(key, factory=factory_fn)
activate Registry
alt Instance already cached
Registry->>Client: return cached instance
else Not cached
Registry->>Factory: factory(adapter_class)
activate Factory
Factory->>Adapter: create instance
deactivate Factory
Registry->>Registry: cache instance
Registry->>Client: return instance
end
deactivate Registry
Client->>Registry: teardown_instances()
activate Registry
Registry->>Adapter: await adapter.teardown()
Registry->>Registry: clear instance cache
deactivate Registry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
enhancement
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (42.83%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11990 +/- ##
==========================================
+ Coverage 37.44% 37.61% +0.16%
==========================================
Files 1616 1619 +3
Lines 79060 79289 +229
Branches 11946 11971 +25
==========================================
+ Hits 29607 29827 +220
- Misses 47795 47803 +8
- Partials 1658 1659 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
HzaRashid
changed the title
github-actions
Bot
added
enhancement
HzaRashid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lfx/tests/unit/services/test_subservice_registry.py (1)
136-142: Add a regression test for decoratoroverride=Falseprecedence.Current tests cover
SubServiceRegistry.register_sub_service_class(..., override=False)but notregister_sub_service(..., override=False)when an entry point already registered the same key.🧪 Suggested test addition
+def test_decorator_override_false_preserves_entrypoint(tmp_path, monkeypatch): + registry = _registry() + monkeypatch.setattr( + "importlib.metadata.entry_points", + lambda group: [DummyEntryPoint("local", Path)] if group == "lfx.deployment.adapters" else [], + ) + subservice_mod.register_sub_service("deployment.adapters", "local", override=False)(PurePath) + registry.discover_sub_services(config_dir=tmp_path) + assert registry.get_sub_service_class("local") is PathAs per coding guidelines "Consider including edge cases and error conditions for comprehensive test coverage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/tests/unit/services/test_subservice_registry.py` around lines 136 - 142, Add a regression test that exercises SubServiceRegistry.register_sub_service(..., override=False) when an entry point (or prior register_sub_service_class call) already exists for the same key: create a registry, register a class or entry for key "local" (e.g., via register_sub_service_class("local", Path) or by simulating an entry point), then call register_sub_service("local", some_factory_or_callable, override=False) and assert the original registered class/factory remains (use registry.get_sub_service_class("local") or registry.get_sub_service("local") as appropriate); this mirrors the existing test for register_sub_service_class override behavior but targets register_sub_service to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lfx/src/lfx/services/subservice.py`:
- Around line 23-24: The discovery replay currently forces override=True when
re-registering services, breaking calls that used register_sub_service(...,
override=False); update the replay logic to preserve the original override
semantics by reading and passing the stored override flag from
_decorator_subservice_registry when invoking register_sub_service (and similar
replay calls), rather than hardcoding True; touch all places noted
(_decorator_subservice_registry usage, the register_sub_service calls around the
blocks referencing _subservice_registries and SubServiceRegistry) so replayed
registrations honor the originally recorded override value.
---
Nitpick comments:
In `@src/lfx/tests/unit/services/test_subservice_registry.py`:
- Around line 136-142: Add a regression test that exercises
SubServiceRegistry.register_sub_service(..., override=False) when an entry point
(or prior register_sub_service_class call) already exists for the same key:
create a registry, register a class or entry for key "local" (e.g., via
register_sub_service_class("local", Path) or by simulating an entry point), then
call register_sub_service("local", some_factory_or_callable, override=False) and
assert the original registered class/factory remains (use
registry.get_sub_service_class("local") or registry.get_sub_service("local") as
appropriate); this mirrors the existing test for register_sub_service_class
override behavior but targets register_sub_service to prevent regressions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lfx/PLUGGABLE_SERVICES.mdsrc/lfx/src/lfx/services/subservice.pysrc/lfx/tests/unit/services/test_subservice_registry.py
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
HzaRashid
force-pushed
the
lfx-subservice
branch
from
a4b8f4c to
ba00e1e
Compare
github-actions
Bot
added
enhancement
HzaRashid
changed the title
github-actions
Bot
added
enhancement
ogabrielluiz
reviewed
Mar 5, 2026
| self._adapter_instances[key] = instance | ||
| return instance | ||
|
|
||
| async def teardown_instances(self) -> None: |
Contributor
There was a problem hiding this comment.
Why not keep the lock open for the entire process? that could create a window for instances to be created while teardown is happening.
ogabrielluiz
reviewed
Mar 5, 2026
Contributor
There was a problem hiding this comment.
we should probably add this here since we have very similar classes that could drift:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from lfx.services.interfaces import DeploymentServiceProtocol
# Static assertion: ensure ABC stays in sync with the Protocol.
_: type[DeploymentServiceProtocol] = BaseDeploymentService
Contributor
|
You should probably add a concurrent discovery test. The |
…ing namespace registries
- Use asyncio.iscoroutine for teardown (matches ServiceManager) - Two-tier entry point error handling (warning vs debug) - Add config load logging after adapter discovery - Remove _get_nested_section wrapper, call shared helper directly - Remove deployment/registry.py thin wrapper - Export DeploymentServiceProtocol from services/__init__ - Extract test stubs into shared adapter_test_helpers module - Revert incorrect ValueError->TypeError in schema validators
…ention Align adapter registry with the service manager's @register_service pattern: decorators now write directly to the AdapterRegistry singleton instead of buffering in a module-level staging dict. - register_adapter calls get_adapter_registry() + register_class() directly, removing _decorator_adapter_registry and _decorator_lock - get_adapter_registry derives entry_point_group and config_section_path from AdapterType by convention, making them optional parameters - Remove _discover_from_decorators() since decorators are already registered before discover() runs - Fix discover_plugins docstring that incorrectly claimed decorators had highest priority (config files do) - Simplify _reset_registries, deps.py, test helpers, and docs
…apter registry
- Encapsulate AdapterRegistry internal state behind private attrs and
read-only properties (adapter_type, entry_point_group, config_section_path,
is_discovered, has_cached_instances)
- Re-raise unexpected exceptions in register_adapter decorator instead of
silently swallowing them
- Promote entry-point discovery catch-all from debug to warning with traceback
- Wrap register_class in RLock for thread safety
- Add try/except with context logging around factory calls in get_instance
- Improve teardown_instances: preserve keys for error messages, add exc_info
- Evict stale cached instances when register_class changes the class for a key
- Narrow load_toml_config exception to ValueError (parent of TOMLDecodeError)
- Split load_object_from_import_path error handling: expected import failures
vs unexpected errors with traceback
- Guard redundant discover() calls in get_deployment_adapter via is_discovered
- Add tests for teardown exception isolation, sync teardown, and no-teardown
adapters
ogabrielluiz
reviewed
Mar 5, 2026
ogabrielluiz
approved these changes
Mar 5, 2026
Contributor
ogabrielluiz
left a comment
ogabrielluiz
left a comment
There was a problem hiding this comment.
Looks good. Well-structured adapter registry with solid test coverage and clean design.
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
3 participants
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
AdapterRegistry, a generic plugin registry that manages multiple swappable adapterimplementations per adapter type (e.g. deployment adapters keyed by
"local","remote").Supports the same three discovery sources as
ServiceManager— decorator registration, entrypoints, and TOML config files — with the same effective precedence (config > decorators > entry
points).
lfx/services/deployment/tolfx/services/adapters/deployment/and addAdapterType.DEPLOYMENTas the first adapter type.Expose a typed
get_deployment_adapter()helper inlfx.services.depsfor singleton resolution.config_discovery.py) used by bothServiceManagerandAdapterRegistry, removing duplicated config-loading logic from theservice manager.
Motivation
The existing
ServiceManagerenforces one implementation perServiceType. Some servicecategories (like deployment) need multiple co-existing implementations selected by a runtime key.
Adapter registries fill this gap: they provide per-key class registration, lazy singleton
instantiation, and lifecycle management (teardown) without replacing the top-level service manager.
Key changes
lfx/services/adapters/registry.py—AdapterRegistryclass,@register_adapterdecorator,
get_adapter_registry()singleton factory, andteardown_all_adapter_registries()lfx/services/schema.py—AdapterTypeenum (DEPLOYMENT)lfx/services/config_discovery.py— sharedget_preferred_config_source,load_toml_config,get_nested_sectionlfx/services/manager.py— refactored to use shared config helpers; calls adapterteardown during shutdown
lfx/services/deps.py—get_deployment_adapter()typed accessorPLUGGABLE_SERVICES.md— documentation for adapter registriesSummary by CodeRabbit
New Features
register_adapter,get_deployment_adapter,teardown_all_adapter_registries.Documentation
Tests