Adding Lcore 714 health endpoint #744
Conversation
WalkthroughAdded a minimal FastAPI Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant F as Mock Fixture
participant H as AsyncLlamaStackClientHolder
participant P as Health Probe
rect rgb(220,235,255)
Note over T,P: Liveness check
T->>F: prepare holder (healthy/uninitialized)
T->>P: call liveness_probe_get_method()
P->>H: query holder state
H-->>P: holder ref / state
P-->>T: 200 {"alive": true}
end
rect rgb(255,235,230)
Note over T,P: Readiness - uninitialized
T->>F: arrange uninitialized holder
T->>P: call readiness_probe_get_method()
P->>H: inspect/version check (async)
H-->>P: raises / not initialised
P-->>T: 503 + providers list (id "unknown", init failure)
end
rect rgb(230,255,235)
Note over T,P: Readiness - mixed/healthy providers
T->>F: arrange mocked provider statuses
T->>P: call readiness_probe_get_method()
P->>H: inspect returns provider health entries
H-->>P: provider statuses
P-->>T: 200 + readiness payload (ready, providers list)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/endpoints/test_health_integration.py (1)
61-117: Load the configuration fixture for readiness checks.
reset_configuration_stateclearsconfiguration._configurationbefore every test, so both readiness scenarios currently run with an unloaded config. The tests still pass because the code falls back to an empty provider list, but that makes the assertions vacuous and will break onceget_providers_health_statusesenforces a loaded configuration (it raisesLogicErrortoday). Please pull intest_configso the tests exercise the real wiring they document.async def test_health_readiness_config_error( - test_response: Response, + test_config: AppConfig, + test_response: Response, test_auth: AuthTuple, ) -> None: @@ async def test_health_readiness( mock_llama_stack_client_health: AsyncMockType, - test_response: Response, + test_config: AppConfig, + test_response: Response, test_auth: AuthTuple, ) -> None: @@ - _ = mock_llama_stack_client_health + _ = (mock_llama_stack_client_health, test_config)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/conftest.py(2 hunks)tests/integration/endpoints/test_health_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/integration/endpoints/test_health_integration.pytests/integration/conftest.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/integration/endpoints/test_health_integration.pytests/integration/conftest.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/integration/endpoints/test_health_integration.pytests/integration/conftest.py
tests/**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use conftest.py for shared pytest fixtures
Files:
tests/integration/conftest.py
🧠 Learnings (1)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to tests/**/conftest.py : Use conftest.py for shared pytest fixtures
Applied to files:
tests/integration/conftest.py
🧬 Code graph analysis (1)
tests/integration/endpoints/test_health_integration.py (3)
src/configuration.py (1)
configuration(73-77)src/app/endpoints/health.py (2)
liveness_probe_get_method(126-140)readiness_probe_get_method(80-112)src/client.py (1)
get_client(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/endpoints/test_health_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/integration/endpoints/test_health_integration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/integration/endpoints/test_health_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/integration/endpoints/test_health_integration.py
🧬 Code graph analysis (1)
tests/integration/endpoints/test_health_integration.py (2)
src/app/endpoints/health.py (3)
liveness_probe_get_method(126-140)readiness_probe_get_method(80-112)get_providers_health_statuses(29-63)src/client.py (1)
get_client(49-55)
🪛 GitHub Actions: Python linter
tests/integration/endpoints/test_health_integration.py
[warning] 16-16: pylint C0411: third party import 'llama_stack.providers.datatypes.HealthStatus' should be placed before first party imports; wrong-import-order
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
| @pytest.mark.asyncio | ||
| async def test_health_readiness( | ||
| mock_llama_stack_client_health: AsyncMockType, | ||
| test_response: Response, | ||
| test_auth: AuthTuple, | ||
| ) -> None: | ||
| """Test that readiness probe endpoint returns readiness status. | ||
|
|
||
| This integration test verifies: | ||
| - Endpoint handler integrates with configuration system | ||
| - Configuration values are correctly accessed | ||
| - Real noop authentication is used | ||
| - Response structure matches expected format | ||
|
|
||
| Args: | ||
| mock_llama_stack_client_health: Mocked Llama Stack client | ||
| test_response: FastAPI response object | ||
| test_auth: noop authentication tuple | ||
| """ | ||
| _ = mock_llama_stack_client_health | ||
|
|
||
| result = await readiness_probe_get_method(auth=test_auth, response=test_response) | ||
|
|
||
| # Verify that service returns readiness response | ||
| assert result.ready is True | ||
| assert result.reason == "All providers are healthy" | ||
| assert result.providers is not None |
There was a problem hiding this comment.
Stub healthy providers before asserting readiness.
With no return value configured for mock_llama_stack_client_health.providers.list, get_providers_health_statuses() hits the exception path, returns an "unknown" error entry, and readiness_probe_get_method() keeps ready false—so the assertions on Lines 181-183 fail. Set an explicit healthy provider list in this test.
async def test_health_readiness(
mock_llama_stack_client_health: AsyncMockType,
+ mocker: MockerFixture,
test_response: Response,
test_auth: AuthTuple,
) -> None:
@@
- Args:
- mock_llama_stack_client_health: Mocked Llama Stack client
- test_response: FastAPI response object
- test_auth: noop authentication tuple
+ Args:
+ mock_llama_stack_client_health: Mocked Llama Stack client
+ mocker: pytest-mock fixture for creating mock objects
+ test_response: FastAPI response object
+ test_auth: noop authentication tuple
@@
- _ = mock_llama_stack_client_health
+ mock_llama_stack_client_health.providers.list.return_value = [
+ mocker.Mock(
+ provider_id="healthy-provider",
+ health={"status": "ok", "message": ""},
+ )
+ ]
result = await readiness_probe_get_method(auth=test_auth, response=test_response)🤖 Prompt for AI Agents
In tests/integration/endpoints/test_health_integration.py around lines 157 to
183, the mocked Llama Stack client does not have a configured return for
mock_llama_stack_client_health.providers.list causing
get_providers_health_statuses() to return an "unknown" error and readiness to be
false; fix by stubbing the mock to return an explicit healthy providers list
before calling readiness_probe_get_method (e.g., set
mock_llama_stack_client_health.providers.list to return a list/dict representing
one or more healthy providers so the probe sees all providers healthy and the
assertions for ready=True, reason="All providers are healthy", and providers
non-None will pass).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/integration/endpoints/test_health_integration.py (2)
3-16: Fix import grouping to unblock lint.The third-party import
llama_stack.providers.datatypesshould be grouped with other third-party imports before first-party imports to satisfy pylint C0411.
157-178: Stub healthy providers before asserting readiness.Without configuring
mock_llama_stack_client_health.providers.list.return_value,get_providers_health_statuses()hits the exception path and returns an error status, causing the test assertions to fail.
🧹 Nitpick comments (3)
tests/integration/endpoints/test_health_integration.py (3)
19-22: Refine fixture return type annotation.The return type
Generator[Any, None, None]is too generic. Since the fixture yields anAsyncMockType, consider usingGenerator[AsyncMockType, None, None]for better type safety.Apply this diff:
@pytest.fixture(name="mock_llama_stack_client_health") def mock_llama_stack_client_fixture( mocker: MockerFixture, -) -> Generator[Any, None, None]: +) -> Generator[AsyncMockType, None, None]:
42-57: Clarify unusedtest_configparameter.The
test_configparameter is assigned to_on line 57, suggesting it's unused. If this fixture is required only for its side effects (e.g., loading application configuration), consider clarifying this in the docstring. Otherwise, remove the parameter if it's not needed.
180-183: Strengthen assertion for healthy providers scenario.When all providers are healthy,
result.providersshould contain an empty list (since it only includes unhealthy providers). The assertionassert result.providers is not Noneis weak and doesn't verify the expected behavior.Apply this diff:
# Verify that service returns readiness response assert result.ready is True assert result.reason == "All providers are healthy" - assert result.providers is not None + assert result.providers == []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/endpoints/test_health_integration.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/integration/endpoints/test_health_integration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/integration/endpoints/test_health_integration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/integration/endpoints/test_health_integration.py
🧬 Code graph analysis (1)
tests/integration/endpoints/test_health_integration.py (3)
src/configuration.py (2)
configuration(73-77)AppConfig(39-181)src/app/endpoints/health.py (3)
liveness_probe_get_method(126-140)readiness_probe_get_method(80-112)get_providers_health_statuses(29-63)src/client.py (1)
get_client(49-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e_tests (ci)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (2)
tests/integration/endpoints/test_health_integration.py (2)
65-119: LGTM!The test properly configures the mock with mixed health statuses and verifies correct mapping of provider IDs, statuses, and messages. The index-based assertions ensure the function preserves provider order as returned by the client.
121-155: LGTM!This test effectively validates the error handling path by exercising the uninitialized client scenario. The comprehensive assertions verify the 503 status code, error structure, and detailed error message.
3 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit