LCORE-2492: Use optional data types consistently#1877
Conversation
WalkthroughThis PR standardizes type annotations across two modules by replacing PEP-604 union syntax ( ChangesType annotation standardization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pydantic_ai_lightspeed/llamastack/_provider.py (1)
5-57:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis change contradicts the coding guidelines for modern type syntax.
The coding guidelines for
src/**/*.pyexplicitly state: "Use modern syntax (str | int)". This directive applies to union types, where the modern PEP-604 syntax should be preferred over the olderOptionalform.This PR changes multiple type annotations from modern syntax to legacy syntax:
- Line 47:
ModelProfile | None→Optional[ModelProfile]- Line 54:
str | None→Optional[str]- Line 55:
str | None→Optional[str]- Line 56:
AsyncLlamaStackAsLibraryClient | None→Optional[AsyncLlamaStackAsLibraryClient]- Line 57:
httpx.AsyncClient | None→Optional[httpx.AsyncClient]All of these changes move away from the modern PEP-604 union syntax to the legacy
typing.Optionalapproach, contradicting the established guideline.♻️ Revert to modern PEP-604 union syntax
from __future__ import annotations as _annotations -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING import httpx from openai import AsyncOpenAI from pydantic_ai import ModelProfile from pydantic_ai.models import create_async_http_client from pydantic_ai.profiles.openai import openai_model_profile from pydantic_ai.providers import Provider from pydantic_ai_lightspeed.llamastack._transport import LlamaStackLibraryTransport if TYPE_CHECKING: from llama_stack.core.library_client import AsyncLlamaStackAsLibraryClient DEFAULT_BASE_URL = "http://localhost:8321/v1" class LlamaStackProvider(Provider[AsyncOpenAI]): """Provider for Llama Stack — connects to a Llama Stack server's OpenAI-compatible API. Supports two modes: 1. **Server mode** — connect to a running Llama Stack server via HTTP 2. **Library mode** — run Llama Stack in-process via ``AsyncLlamaStackAsLibraryClient`` """ `@property` def name(self) -> str: """The provider name.""" return "llama-stack" `@property` def base_url(self) -> str: """The base URL for the provider API.""" return str(self._client.base_url) `@property` def client(self) -> AsyncOpenAI: """The OpenAI-compatible client for the provider.""" return self._client `@staticmethod` - def model_profile(model_name: str) -> Optional[ModelProfile]: + def model_profile(model_name: str) -> ModelProfile | None: """Return the model profile for the named model, if available.""" return openai_model_profile(model_name) def __init__( self, *, - base_url: Optional[str] = None, - api_key: Optional[str] = None, - library_client: Optional[AsyncLlamaStackAsLibraryClient] = None, - http_client: Optional[httpx.AsyncClient] = None, + base_url: str | None = None, + api_key: str | None = None, + library_client: AsyncLlamaStackAsLibraryClient | None = None, + http_client: httpx.AsyncClient | None = None, ) -> None: """Create a new Llama Stack provider. Args: base_url: The base URL for the Llama Stack server (OpenAI-compatible endpoint). Defaults to ``http://localhost:8321/v1``. Must be ``None`` when ``library_client`` is provided. api_key: The API key for authentication. Defaults to ``'not-needed'`` since local Llama Stack servers typically don't require one. Must be ``None`` when ``library_client`` is provided. library_client: An initialized ``AsyncLlamaStackAsLibraryClient`` for library mode. When provided, requests are dispatched in-process (no server needed). Mutually exclusive with ``base_url``, ``api_key``, and ``http_client``. http_client: An existing ``httpx.AsyncClient`` to use for making HTTP requests. Must be ``None`` when ``library_client`` is provided. """
As per coding guidelines: "Use modern syntax (
str | int)" for type annotations in files undersrc/**/*.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pydantic_ai_lightspeed/llamastack/_provider.py` around lines 5 - 57, The PR reverted PEP-604 union syntax to typing.Optional; update the type hints back to modern syntax: change the return annotation of LlamaStackProvider.model_profile from Optional[ModelProfile] to ModelProfile | None, and in LlamaStackProvider.__init__ change the parameter annotations for base_url, api_key, library_client, and http_client from Optional[...] to str | None, str | None, AsyncLlamaStackAsLibraryClient | None, and httpx.AsyncClient | None respectively so the file follows the "Use modern syntax" guideline.Source: Coding guidelines
src/models/common/agents/turn_accumulator.py (1)
4-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis change contradicts the coding guidelines for modern type syntax.
The coding guidelines for
src/**/*.pyexplicitly state: "Use modern syntax (str | int)". This directive applies to union types, where the modern PEP-604 syntax (AgentRunResult[str] | None) should be preferred over the olderOptional[AgentRunResult[str]]form.The current PR moves in the opposite direction—from modern PEP-604 syntax to the legacy
typing.Optionalapproach—which contradicts the established guideline.♻️ Revert to modern PEP-604 union syntax
from dataclasses import dataclass, field -from typing import Final, Optional +from typing import Final from pydantic_ai import AgentRunResult from models.common.turn_summary import TurnSummary `@dataclass`(slots=True) class AgentTurnAccumulator: # pylint: disable=too-many-instance-attributes """Information accumulator for a single interaction turn. Attributes: vector_store_ids: Vector store IDs used to resolve RAG source labels. rag_id_mapping: Maps vector store IDs to user-facing source names. turn_summary: Aggregated turn output (text, tools, RAG, token usage). run_result: Agent run result (streaming only). chunk_id: Monotonic SSE chunk index (streaming only). text_parts: Buffered text deltas before turn_complete (streaming only). tool_round: Current tool-call round for summary labeling. round_increment_pending: Whether to bump tool_round on the next step. emitted_tool_call_ids: Tool call IDs already sent or recorded. emitted_tool_result_ids: Tool result IDs already sent or recorded. seen_docs: Referenced-document keys already added (deduplication). """ vector_store_ids: Final[list[str]] rag_id_mapping: Final[dict[str, str]] turn_summary: TurnSummary - run_result: Optional[AgentRunResult[str]] = None + run_result: AgentRunResult[str] | None = None chunk_id: int = 0 text_parts: list[str] = field(default_factory=list) tool_round: int = 1 round_increment_pending: bool = False emitted_tool_call_ids: set[str] = field(default_factory=set) emitted_tool_result_ids: set[str] = field(default_factory=set) seen_docs: set[tuple[str, str]] = field(default_factory=set)
As per coding guidelines: "Use modern syntax (
str | int)" for type annotations in files undersrc/**/*.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/models/common/agents/turn_accumulator.py` around lines 4 - 32, The run_result field in AgentTurnAccumulator uses the legacy Optional[...] typing; change its annotation to the modern PEP-604 union form: replace "run_result: Optional[AgentRunResult[str]] = None" with "run_result: AgentRunResult[str] | None = None" and remove the now-unused Optional import from typing; ensure references to AgentRunResult and the AgentTurnAccumulator class (and the run_result symbol) are updated accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/models/common/agents/turn_accumulator.py`:
- Around line 4-32: The run_result field in AgentTurnAccumulator uses the legacy
Optional[...] typing; change its annotation to the modern PEP-604 union form:
replace "run_result: Optional[AgentRunResult[str]] = None" with "run_result:
AgentRunResult[str] | None = None" and remove the now-unused Optional import
from typing; ensure references to AgentRunResult and the AgentTurnAccumulator
class (and the run_result symbol) are updated accordingly.
In `@src/pydantic_ai_lightspeed/llamastack/_provider.py`:
- Around line 5-57: The PR reverted PEP-604 union syntax to typing.Optional;
update the type hints back to modern syntax: change the return annotation of
LlamaStackProvider.model_profile from Optional[ModelProfile] to ModelProfile |
None, and in LlamaStackProvider.__init__ change the parameter annotations for
base_url, api_key, library_client, and http_client from Optional[...] to str |
None, str | None, AsyncLlamaStackAsLibraryClient | None, and httpx.AsyncClient |
None respectively so the file follows the "Use modern syntax" guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bdd47fc-6d24-442a-931f-fdec130d3312
📒 Files selected for processing (2)
src/models/common/agents/turn_accumulator.pysrc/pydantic_ai_lightspeed/llamastack/_provider.py
📜 Review details
⏰ 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). (18)
- GitHub Check: mypy
- GitHub Check: check_dependencies
- GitHub Check: Pylinter
- GitHub Check: spectral
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: Pyright
- GitHub Check: build-pr
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/common/agents/turn_accumulator.pysrc/pydantic_ai_lightspeed/llamastack/_provider.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/common/agents/turn_accumulator.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/common/agents/turn_accumulator.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/common/agents/turn_accumulator.py
1 participant
Description
LCORE-2492: Use optional data types consistently
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit