feat: enable generic serialization for all LiveKit plugins #28
feat: enable generic serialization for all LiveKit plugins #28mahimairaja merged 1 commit intomainfrom
Conversation
… and add CLAUDE.md documentation
|
Important Review skippedAuto 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:
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 90.32% 90.33% +0.01%
==========================================
Files 13 13
Lines 1426 1428 +2
==========================================
+ Hits 1288 1290 +2
Misses 138 138
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Enables generic spawn-safe serialization for LiveKit plugin provider objects by expanding the _opts-based provider reference mechanism beyond the explicitly enumerated OpenAI plugin classes.
Changes:
- Added a generic
_opts-based serialization path for anylivekit.plugins.*provider objects. - Kept
_PROVIDER_REF_KEYSas a fast-path / documentation list for known-tested providers. - Added a unit test intended to ensure non-OpenAI plugins using
_optsare accepted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/openrtc/pool.py |
Extends provider serialization to generically handle livekit.plugins.* objects exposing _opts. |
tests/test_pool.py |
Adds a test case for generic LiveKit plugin acceptance via _opts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Generic path: any livekit plugin with _opts | ||
| if cls.__module__.startswith("livekit.plugins.") and hasattr(value, "_opts"): | ||
| return _ProviderRef( | ||
| module_name=cls.__module__, | ||
| qualname=cls.__qualname__, | ||
| kwargs=_extract_provider_kwargs(value), | ||
| ) |
There was a problem hiding this comment.
The generic provider-ref path triggers purely on cls.__module__.startswith('livekit.plugins.') + hasattr(value, '_opts'), without verifying that the referenced module/class is actually importable/resolvable. This can make a config appear spawn-safe (because _ProviderRef is pickleable) but then fail during unpickling when _deserialize_provider_value calls importlib.import_module(...) / _resolve_qualname(...). Consider guarding the generic path by confirming the module can be imported (or at least importlib.util.find_spec is not None) and that _opts is non-None, otherwise fall back to the existing pickleability check so failures happen at registration time rather than in the worker.
| def test_generic_livekit_plugin_with_opts_is_accepted() -> None: | ||
| """Non-OpenAI livekit plugins with ``_opts`` should serialize via the generic path.""" | ||
|
|
||
| class FakeOpts: | ||
| model = "nova-3" | ||
|
|
||
| # Create a class whose module looks like a livekit plugin | ||
| FakeSTT = type("STT", (), {"__module__": "livekit.plugins.deepgram.stt"}) | ||
| instance = FakeSTT() | ||
| instance._opts = FakeOpts() # type: ignore[attr-defined] | ||
|
|
||
| pool = AgentPool() | ||
| # Should not raise — the generic _opts path handles it | ||
| config = pool.add("test", DemoAgent, stt=instance) | ||
| assert config.stt is not None |
There was a problem hiding this comment.
This test only asserts that pool.add(...) does not raise, but it doesn't validate the intended behavior (pickle/rehydration via the generic _opts path). As written, it also sets model as a class attribute on FakeOpts, so _extract_provider_kwargs (which uses vars(options)) will extract {} and drop model, making the test pass even if option serialization is broken. To properly cover the new behavior, ensure the fake plugin module/class is importable (e.g., via a temporary module in sys.modules) and assert a pickle round-trip restores the provider type and preserves extracted kwargs like model.
No description provided.