-
Notifications
You must be signed in to change notification settings - Fork 63
LCORE-1051: final updates #985
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
WalkthroughThis pull request standardizes type annotations across endpoint and utility modules by replacing union-with-None syntax (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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 (3)
src/app/endpoints/query.py (3)
158-164: Inconsistent with coding guidelines for return types and local variables.Per the coding guidelines,
Optional[Type]should be used specifically for optional parameters, while return types and local variables should use the modern union syntax (Type | None). Line 159's parameter annotation is correct, but lines 161, 163, and 164 should usestr | None.♻️ Suggested change to align with guidelines
def evaluate_model_hints( user_conversation: Optional[UserConversation], query_request: QueryRequest, -) -> tuple[Optional[str], Optional[str]]: +) -> tuple[str | None, str | None]: """Evaluate model hints from user conversation.""" - model_id: Optional[str] = query_request.model - provider_id: Optional[str] = query_request.provider + model_id: str | None = query_request.model + provider_id: str | None = query_request.providerBased on coding guidelines: "Use union types with modern syntax" and "Use
Optional[Type]for optional parameters".
274-274: Local variable should use modern union syntax.Per the coding guidelines,
Optional[Type]is for optional function parameters. Local variables should use the modern union syntax.♻️ Suggested change
- user_conversation: Optional[UserConversation] = None + user_conversation: UserConversation | None = NoneBased on coding guidelines: "Use union types with modern syntax:
str | intinstead ofUnion[str, int]".
860-874: Return type should use modern union syntax.Per the coding guidelines,
Optional[Type]is specifically for optional function parameters. Return types should use the modern union syntaxType | None.♻️ Suggested change
def get_rag_toolgroups( vector_db_ids: list[str], -) -> Optional[list[Toolgroup]]: +) -> list[Toolgroup] | None: """ Return a list of RAG Tool groups if the given vector DB list is not empty. Generate a list containing a RAG knowledge search toolgroup if vector database IDs are provided. Parameters: vector_db_ids (list[str]): List of vector database identifiers to include in the toolgroup. Returns: - Optional[list[Toolgroup]]: A list with a single RAG toolgroup if + list[Toolgroup] | None: A list with a single RAG toolgroup if vector_db_ids is non-empty; otherwise, None. """Based on coding guidelines: "Use union types with modern syntax:
str | intinstead ofUnion[str, int]".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/endpoints/a2a.pysrc/app/endpoints/query.pysrc/app/endpoints/query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/streaming_query_v2.pysrc/utils/types.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/utils/types.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/a2a.pysrc/app/endpoints/query_v2.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use FastAPI
HTTPExceptionwith appropriate status codes for API endpoint error handling
Files:
src/app/endpoints/query.pysrc/app/endpoints/streaming_query_v2.pysrc/app/endpoints/streaming_query.pysrc/app/endpoints/a2a.pysrc/app/endpoints/query_v2.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Use union types with modern syntax: `str | int` instead of `Union[str, int]`
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Use `Optional[Type]` for optional parameters
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Complete type annotations for function parameters and return types, using `typing_extensions.Self` for model validators
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to **/*.py : Use FastAPI dependencies: `from fastapi import APIRouter, HTTPException, Request, status, Depends`
Applied to files:
src/app/endpoints/streaming_query.pysrc/app/endpoints/a2a.py
🧬 Code graph analysis (4)
src/app/endpoints/query.py (1)
src/models/requests.py (1)
QueryRequest(73-276)
src/app/endpoints/streaming_query.py (1)
src/models/database/conversations.py (1)
UserConversation(11-38)
src/app/endpoints/a2a.py (1)
src/a2a_storage/context_store.py (1)
A2AContextStore(7-58)
src/app/endpoints/query_v2.py (1)
src/utils/types.py (2)
ToolCallSummary(111-119)ToolResultSummary(122-133)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (29)
src/utils/types.py (1)
107-108: LGTM!The type annotation changes from
str | NonetoOptional[str]align with the coding guidelines for optional parameters. The Pydantic model fields are correctly annotated.src/app/endpoints/streaming_query_v2.py (4)
4-4: LGTM!Import updated to include
Optionalfor the standardized type annotations used throughout the file.
141-141: LGTM!Type annotation correctly updated to
Optional[Any]for the nullable internal state variable.
375-376: LGTM!Function parameter type annotation correctly updated to
Optional[dict[str, dict[str, str]]]per coding guidelines.
474-475: LGTM!Optional parameter type annotation correctly updated to
Optional[str].src/app/endpoints/a2a.py (6)
8-8: LGTM!Import updated to include
Optionalfor the standardized type annotations.
68-69: LGTM!Module-level state variables correctly typed as
Optional[TaskStore]andOptional[A2AContextStore]for the lazy initialization pattern.
123-123: LGTM!Instance attribute correctly typed as
Optional[Message]for the nullable state.
172-172: LGTM!Property return type correctly updated to
Optional[Message].
188-188: LGTM!Constructor parameter type annotation correctly updated to
Optional[dict[str, dict[str, str]]].
416-417: LGTM!Method parameter type annotation correctly updated to
Optional[str]for the nullable conversation ID.src/app/endpoints/streaming_query.py (6)
10-18: LGTM!Import block reformatted to include
Optionalfor the standardized type annotations used throughout the file.
242-243: LGTM!Function parameter type annotation correctly updated to
Optional[str]for the nullable conversation ID.
395-396: LGTM!Function parameter type annotation correctly updated to
Optional[str].
745-745: LGTM!Internal state variable correctly typed as
Optional[Any].
861-861: LGTM!Local variable correctly typed as
Optional[UserConversation]for the nullable conversation lookup result.
1012-1013: LGTM!Function parameter type annotation correctly updated to
Optional[dict[str, dict[str, str]]].src/app/endpoints/query_v2.py (10)
7-7: LGTM!Import updated to include
Optionalfor the standardized type annotations.
77-77: LGTM!Return type annotation correctly updated to use
Optional[ToolCallSummary]andOptional[ToolResultSummary]for the nullable tuple elements.
113-113: LGTM!Local variable correctly typed as
Optional[Any]for the nullable response payload.
297-298: LGTM!Function parameter type annotation correctly updated to
Optional[dict[str, dict[str, str]]].
508-508: LGTM!Set type annotation correctly uses
Optional[str]for nullable tuple elements in the deduplication set.
538-539: LGTM!Comment and code correctly reference
Optional[AnyUrl]for the nullable URL type.
695-706: LGTM!Function return type and docstring correctly updated to
Optional[list[dict[str, Any]]].
720-721: LGTM!Function parameters correctly typed as
Optional[str]andOptional[dict[str, dict[str, str]]].
765-766: LGTM!Function parameter and return type correctly updated to use
Optionaltyping.
781-782: LGTM!Docstring correctly updated to reflect the
Optional[list[dict[str, Any]]]return type.src/app/endpoints/query.py (2)
485-487: LGTM!Using
Optional[str]for these optional function parameters aligns with the coding guideline: "UseOptional[Type]for optional parameters".
666-666: LGTM!Using
Optional[dict[str, dict[str, str]]]for this optional parameter with defaultNoneis correct per the coding guidelines.
Description
LCORE-1051: final updates
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.