-
Notifications
You must be signed in to change notification settings - Fork 65
feat(rlsapi): add v1 /infer endpoint module #902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a new FastAPI module exposing a stateless POST /infer for RLSAPI v1 with auth enforcement, request_id generation via get_suid, logging of input metadata, OpenAPI response mappings, and unit tests validating minimal/full-context requests and validation behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/app/endpoints/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/{client,app/endpoints/**}.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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). (1)
🔇 Additional comments (5)
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 |
|
Hi @major. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
1-27: Align logger with module-level__name__patternThe module docstring and imports look good, but the logger is initialized with a hard-coded name
"app.endpoints.handlers"instead of__name__. To follow the standard pattern and avoid surprises if this module is moved or renamed, consider:-logger = logging.getLogger("app.endpoints.handlers") +logger = logging.getLogger(__name__)As per coding guidelines, the
logger = logging.getLogger(__name__)pattern is preferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/rlsapi_v1.py(1 hunks)tests/unit/app/endpoints/test_rlsapi_v1.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_rlsapi_v1.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_rlsapi_v1.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/rlsapi_v1.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/rlsapi_v1.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/rlsapi_v1.py
🧬 Code graph analysis (1)
src/app/endpoints/rlsapi_v1.py (6)
src/authentication/__init__.py (1)
get_auth_dependency(21-72)src/authorization/middleware.py (1)
authorize(111-122)src/models/config.py (1)
Action(755-808)src/models/responses.py (3)
ForbiddenResponse(1382-1529)UnauthorizedResponse(1298-1379)UnprocessableEntityResponse(1623-1668)src/models/rlsapi/requests.py (2)
RlsapiV1InferRequest(123-200)get_input_source(178-200)src/models/rlsapi/responses.py (2)
RlsapiV1InferData(9-26)RlsapiV1InferResponse(29-53)
⏰ 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). (5)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (6)
tests/unit/app/endpoints/test_rlsapi_v1.py (4)
1-21: Good use of pytest and clear setupThe module-level setup (docstring, imports,
MOCK_AUTH) is clean and aligns with the pytest + pytest-mock guidelines. The explicitAuthTupletype and static auth fixture make the tests easy to follow and reuse.
23-35: Minimal request test is focused and validates UUID semantics
test_infer_minimal_requestneatly covers the happy path with the minimal required payload and additionally verifies thatrequest_idparses as a real UUID, not just a non-empty string. This is a solid behavioral contract for the endpoint.
37-56: Full-context test effectively exercisesget_input_sourcepath
test_infer_full_context_requestsensibly constructs a richRlsapiV1Context(stdin, attachment, terminal, systeminfo), ensuring the endpoint correctly accepts and processes the full shape of the request. Assertingtextand non-emptyrequest_idkeeps the test robust against future LLM changes while still validating the contract.
58-67: Unique request ID behavior is well-covered
test_infer_generates_unique_request_idsis a concise way to enforce per-call uniqueness forrequest_idgiven identical inputs, which matches how you’re generating UUIDs server-side. This should catch accidental reuse or caching bugs going forward.src/app/endpoints/rlsapi_v1.py (2)
29-36: Reusing shared OpenAPI response schemas is a solid approach
infer_responsescorrectly wires the endpoint into the sharedUnauthorizedResponse,ForbiddenResponse, andUnprocessableEntityResponseOpenAPI helpers, keeping the API surface consistent with the rest of the stack and centralizing error schema definitions.
39-84: Endpoint implementation is clean, stateless, and matches the models/testsThe
/inferroute is well-structured: async, fully typed, guarded byget_auth_dependency()and@authorize(Action.RLSAPI_V1_INFER), generates a per-call UUIDrequest_id, logs with user context, and usesRlsapiV1InferRequest.get_input_source()before returning a typedRlsapiV1InferResponse. The placeholder response text is clearly marked and should be straightforward to replace with real LLM integration later without changing the external contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
45-58: Align docstring with current placeholder behaviorThe docstring says the endpoint “returns an LLM-generated response”, but the implementation currently returns a static placeholder message until LLM integration is added. Consider updating the docstring to match current behavior (and adjust again once the LLM call is wired) to avoid confusion.
For example:
- Accepts a question with optional context (stdin, attachments, terminal output, - system info) and returns an LLM-generated response. + Accepts a question with optional context (stdin, attachments, terminal output, + system info) and returns a placeholder response. LLM integration will be + added in a subsequent update.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/rlsapi_v1.py(1 hunks)tests/unit/app/endpoints/test_rlsapi_v1.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/app/endpoints/test_rlsapi_v1.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/rlsapi_v1.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/rlsapi_v1.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/rlsapi_v1.py
⏰ 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). (5)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
🔇 Additional comments (2)
src/app/endpoints/rlsapi_v1.py (2)
1-36: Good module setup and OpenAPI response wiringDocstring, absolute imports, logger pattern, APIRouter usage, and the typed
infer_responsesmapping all look consistent with the project guidelines and FastAPI conventions. No issues here.
69-84: Placeholder implementation is clean and type-safe; ensureAPIConnectionErrorhandling when adding LLM integrationThe endpoint correctly combines input sources, generates a request ID, and returns a properly typed response. When adding the actual LLM call, handle
APIConnectionErrorfrom Llama Stack by translating it to an appropriateHTTPExceptionstatus code per the endpoint guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/app/endpoints/test_rlsapi_v1.py (2)
36-55: LGTM! Full context test provides good coverage.The test validates the endpoint handles full context data (stdin, attachments, terminal, systeminfo) without errors. Since LLM integration is deferred, the placeholder response approach is appropriate.
Optional enhancement for future: Once LLM integration is added, consider verifying that the context fields influence the response or are logged appropriately.
1-66: Consider adding error case tests for better coverage.The current tests provide solid happy-path coverage. To meet the 60% unit test coverage guideline, consider adding tests for:
- Invalid requests (empty/whitespace-only questions)
- Authorization failures (if testable at this layer)
- Response schema validation
These additions would strengthen the test suite and help ensure edge cases are handled correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/rlsapi_v1.py(1 hunks)tests/unit/app/endpoints/test_rlsapi_v1.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/app/endpoints/test_rlsapi_v1.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/app/endpoints/test_rlsapi_v1.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/app/endpoints/rlsapi_v1.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/rlsapi_v1.py
src/**/{client,app/endpoints/**}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Handle
APIConnectionErrorfrom Llama Stack in integration code
Files:
src/app/endpoints/rlsapi_v1.py
🧬 Code graph analysis (1)
tests/unit/app/endpoints/test_rlsapi_v1.py (5)
src/app/endpoints/rlsapi_v1.py (1)
infer_endpoint(41-82)src/models/rlsapi/requests.py (5)
RlsapiV1Attachment(8-25)RlsapiV1Context(89-120)RlsapiV1InferRequest(123-200)RlsapiV1SystemInfo(42-66)RlsapiV1Terminal(28-39)src/models/rlsapi/responses.py (1)
RlsapiV1InferResponse(29-53)tests/unit/utils/auth_helpers.py (1)
mock_authorization_resolvers(8-26)src/utils/suid.py (1)
check_suid(19-98)
⏰ 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). (5)
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: build-pr
🔇 Additional comments (9)
tests/unit/app/endpoints/test_rlsapi_v1.py (4)
1-17: LGTM! Clean imports and module structure.The module docstring is descriptive, and all imports follow the absolute import pattern required by the coding guidelines.
19-19: LGTM! Well-defined test constant.The MOCK_AUTH constant provides a clean, reusable authentication fixture for the tests.
22-34: LGTM! Solid minimal request test.The test properly validates the basic happy path: response structure, text presence, and request_id validity using
check_suid. Type annotations and docstring follow guidelines.
57-66: LGTM! Important uniqueness validation.This test ensures each request gets a unique identifier, which is critical for request tracing and debugging. Clean and focused test case.
src/app/endpoints/rlsapi_v1.py (5)
1-23: LGTM! Excellent module documentation and imports.The module docstring clearly describes the purpose, and all imports follow the absolute import pattern required by the coding guidelines.
25-26: LGTM! Proper logger and router initialization.Logger follows the required
logging.getLogger(__name__)pattern, and the router is appropriately tagged for API organization.
29-36: LGTM! Comprehensive OpenAPI response documentation.The response schemas properly document all success and error cases (200, 401, 403, 422) with appropriate examples, improving API documentation quality.
39-58: LGTM! Excellent function signature and documentation.The endpoint is properly decorated with routing and authorization, includes complete type annotations, and has a comprehensive Google-style docstring that clearly explains its purpose, parameters, and return value.
59-82: LGTM! Clean implementation with privacy concern addressed.The implementation is well-structured:
- Line 65 logs only the
request_idat INFO level, addressing the privacy concern from the previous review about not logging user identifiers.- Request tracking via
get_suid()is appropriate for tracing.- Placeholder response with clear NOTE comment is acceptable given the PR objectives (LLM integration deferred to follow-up).
async defis appropriate for future LLM integration even though current implementation is synchronous.
|
/ok-to-test |
Create the endpoint structure for stateless inference requests from the RHEL Lightspeed Command Line Assistant (CLA). - POST /infer endpoint with RlsapiV1InferRequest/Response models - Authentication via get_auth_dependency() - Authorization via @authorize(Action.RLSAPI_V1_INFER) - OpenAPI response schemas for 200/401/403/422 - Unit tests for endpoint functionality LLM integration will be wired up in a follow-up PR. Signed-off-by: Major Hayden <major@redhat.com>
|
/ok-to-test |
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks ok, thank you
Description
Create the endpoint structure for stateless inference requests from the RHEL Lightspeed Command Line Assistant (CLA).
LLM integration will be wired up in a follow-up PR.
Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
Existing unit tests should cover this code.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.