Use PythonServerLauncher in LightningStoreServer #303
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors LightningStoreServer to use the PythonServerLauncher utility class instead of directly managing uvicorn server lifecycle. This unifies server launching logic and adds support for multiple launch modes (asyncio, thread, multiprocessing).
Key changes:
- Replaces direct uvicorn server management with
PythonServerLauncher - Adds
launch_modeparameter to control how the server is started (thread, asyncio, mp) - Adds
launcher_argsparameter for advanced configuration - Implements pickling support for both
LightningStoreServerandPythonServerLauncherto enable multiprocessing - Updates error handling and logging to use dedicated loggers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| agentlightning/store/client_server.py | Refactored to use PythonServerLauncher, added launch_mode support, updated pickling logic, changed to dedicated loggers |
| agentlightning/utils/server_launcher.py | Added access_log parameter, improved pickling with initialize() method, fixed error messages, better error handling |
| tests/store/test_client_server.py | Added tests for launch modes, custom launcher args, and mp mode validation; updated existing tests |
| agentlightning/cli/store.py | Added launch_mode="asyncio" to CLI server instantiation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.launcher_args = launcher_args | ||
| else: | ||
| if port is None: | ||
| server_logger.warning("No port provided, using default port 4747.") |
There was a problem hiding this comment.
The warning message claims port 4747 will be used as default, but port is actually None and will be passed to PythonServerLauncherArgs. The actual default behavior in PythonServerLauncher._ensure_port() is to call pick_unused_port(), not use 4747. Either set port=4747 here or update the warning message to say 'using a random available port'.
| server_logger.warning("No port provided, using default port 4747.") | |
| server_logger.warning("No port provided, using a random available port.") |
| def initialize(self): | ||
| # ensure the host/port/access_host are set | ||
| self._ensure_host() | ||
| self._ensure_port() | ||
| self._ensure_access_host() |
There was a problem hiding this comment.
The initialize() method calls _ensure_* methods but doesn't document that these methods have side effects (setting self._host, self._port, self._access_host). Consider adding a docstring explaining this method is used to initialize/reinitialize server state, especially after unpickling.
tests/store/test_client_server.py
Outdated
| import logging | ||
|
|
||
| caplog.set_level(logging.INFO) |
There was a problem hiding this comment.
The import logging statement should be moved to the top of the file with other imports rather than being placed within the test function. This follows Python best practices and is consistent with the rest of the codebase.
| self.app = None | ||
| self.store = None |
There was a problem hiding this comment.
Setting self.app and self.store to None in __setstate__ means unpickled LightningStoreServer instances are in a partially initialized state and will fail if methods requiring these attributes are called. Consider documenting this limitation more explicitly in the docstring or adding a runtime check that raises a clear error if operations requiring the store are attempted on unpickled instances.
| raise RuntimeError( | ||
| f"Server did not start up within {time.time() - start_time:.2f} seconds." | ||
| ) from server_start_exception |
There was a problem hiding this comment.
The variable timeout is referenced in the original code but replaced with time.time() - start_time in the error message. However, the variable timeout is no longer available in scope after this change. While the new calculation correctly shows actual elapsed time, ensure that start_time is defined in the enclosing scope. Review the full function context to verify start_time is accessible here.
|
/ci |
|
🚀 CI Watcher for correlation id-3523351403-mhwc0xfp triggered by comment 3523351403
✅ All runs completed. |
|
/ci |
|
🚀 CI Watcher for correlation id-3524631436-mhwqbykl triggered by comment 3524631436
✅ All runs completed. |
No description provided.