Feature/unified v1 #413
Feature/unified v1 #413
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a complete LLM API feature with async job processing. Changes include: database migration adding LLM_API job type, FastAPI endpoint Changes
Sequence DiagramsequenceDiagram
participant User
participant API as FastAPI /llm/call
participant JobService as start_job
participant DB
participant Celery as execute_job (Celery)
participant Orchestrator
participant Provider as OpenAIProvider
participant OpenAI
User->>API: POST /llm/call (LLMCallRequest)
API->>JobService: start_job(db, request, project_id, org_id)
JobService->>DB: Create Job (status=CREATED)
JobService->>Celery: Enqueue execute_job task (high priority)
Celery-->>API: Job UUID response
API-->>User: {status: "processing"}
rect rgba(100, 150, 200, 0.1)
Note over Celery: Background Processing
Celery->>DB: Update Job to PROCESSING
Celery->>Orchestrator: execute_llm_call(request, client)
Orchestrator->>Provider: execute(request)
Provider->>Provider: Convert request → OpenAISpec
Provider->>OpenAI: API call (create response)
OpenAI-->>Provider: response with tokens
Provider->>Provider: Extract message + build LLMCallResponse
Provider-->>Orchestrator: (LLMCallResponse | None, error | None)
Orchestrator-->>Celery: response/error tuple
alt Success
Celery->>DB: Update Job (status=SUCCESS, response_id)
else Failure
Celery->>DB: Update Job (status=FAILED, error_message)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: New provider abstraction pattern with factory, OpenAI-specific spec validation/conversion logic, integration with existing async job framework, multiple interdependent model definitions, and state management flow across Celery tasks warrant careful review of logic correctness and API consistency. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 5
🧹 Nitpick comments (17)
backend/app/services/llm/providers/base.py (3)
27-33: Prefer Generic or Protocol overAnyfor client type.Using
Anyfor the client parameter loses type safety benefits. Consider using a TypeVar with Generic or a Protocol to maintain type information across provider implementations.Apply this diff to add a TypeVar:
+from typing import Any, TypeVar + +ClientT = TypeVar("ClientT") + -class BaseProvider(ABC): +class BaseProvider(ABC, Generic[ClientT]): """Abstract base class for LLM providers. ... """ - def __init__(self, client: Any): + def __init__(self, client: ClientT): """Initialize the provider with client. Args: - client: Provider-specific client (e.g., OpenAI, Anthropic client) + client: Provider-specific client """ self.client = client
35-60: Consider using exception-based error handling instead of tuple returns.The
tuple[LLMCallResponse | None, str | None]pattern is error-prone and requires callers to manually check both return values. Consider raising exceptions for errors and returningLLMCallResponsedirectly, or use a Result type.Example alternative:
@abstractmethod def execute(self, request: LLMCallRequest) -> LLMCallResponse: """Execute an LLM call using the provider. Raises: ProviderError: If the API call fails ValidationError: If request validation fails """
61-67: Provider name derivation relies on fragile naming convention.The
get_provider_name()method assumes all provider classes end with "Provider" suffix. If a class doesn't follow this convention, the result will be unexpected. Consider making this an abstract property or using explicit configuration.Alternative approach:
@property @abstractmethod def provider_name(self) -> str: """Get the name of the provider.""" ...backend/app/models/llm/response.py (2)
22-28: Consider adding validation constraints to numeric fields.Token count fields should have non-negative constraints to catch invalid API responses early.
Apply this diff:
+from sqlmodel import SQLModel, Field + class LLMCallResponse(SQLModel): ... - input_tokens: int - output_tokens: int - total_tokens: int + input_tokens: int = Field(ge=0) + output_tokens: int = Field(ge=0) + total_tokens: int = Field(ge=0)
22-22: Consider using Literal or Enum for status field.The
statusfield is currently a plainstr, which allows any value. Using a Literal type or Enum would provide better type safety and validation.Example:
from typing import Literal status: Literal["success", "error", "pending"]backend/app/services/llm/orchestrator.py (1)
61-64: Sanitize unexpected-error returns to avoid leaking internalsReturning
str(e)can expose internal details. Log full exception with stack, but return a generic message.- except Exception as e: - error_message = f"Unexpected error in LLM service: {str(e)}" - logger.error(f"[execute_llm_call] {error_message}", exc_info=True) - return None, error_message + except Exception as e: + logger.error("[execute_llm_call] Unexpected error in LLM service", exc_info=True) + return None, "Unexpected error in LLM service"backend/app/services/llm/providers/factory.py (1)
35-36: Normalize provider names and enable dynamic registrationMake provider matching case-insensitive and allow runtime registration for extensions.
class ProviderFactory: @@ - def create_provider(cls, provider_type: str, client: Any) -> BaseProvider: + def create_provider(cls, provider_type: str, client: Any) -> BaseProvider: """Create a provider instance based on the provider type. @@ - provider_class = cls._PROVIDERS.get(provider_type) + normalized = (provider_type or "").strip().lower() + provider_class = cls._PROVIDERS.get(normalized) @@ - logger.info(f"[ProviderFactory] Creating {provider_type} provider instance") + logger.info(f"[ProviderFactory] Creating {normalized} provider instance") return provider_class(client=client) @@ def get_supported_providers(cls) -> list[str]: """Get list of supported provider types. @@ return list(cls._PROVIDERS.keys()) + + @classmethod + def register_provider(cls, name: str, provider_cls: type[BaseProvider]) -> None: + """Register a provider at runtime (useful for plugins/tests).""" + cls._PROVIDERS[name.strip().lower()] = provider_clsAlso applies to: 48-59, 60-67
backend/app/services/llm/__init__.py (1)
12-13: Avoid side‑effect imports; re‑export explicitly if needed.Importing app.services.llm.specs for side effects is surprising. If you intend to expose OpenAISpec (or others), explicitly import and add to all, or drop this import if unused.
-# Initialize model specs on module import -import app.services.llm.specs # noqa: F401 +# from app.services.llm.specs import OpenAISpec +# __all__.append("OpenAISpec")backend/app/models/llm/config.py (2)
28-33: Constrain provider type to prevent typos.Use an Enum or Literal for provider (e.g., "openai") to fail fast on invalid values and align with ProviderFactory.get_supported_providers().
-from sqlmodel import SQLModel +from enum import Enum +from sqlmodel import SQLModel + +class Provider(str, Enum): + openai = "openai" class LLMModelSpec(SQLModel): - provider: str = "openai" + provider: Provider = Provider.openai
37-51: Optional: add basic bounds at this layer.You already enforce ranges in OpenAISpec; adding min/max for max_tokens/top_p here provides earlier feedback when the provider is swapped.
backend/app/services/llm/providers/openai.py (2)
69-87: Make message extraction resilient and collect all text parts.Current logic returns only the first text block; if output contains multiple text items or different shapes, result may be empty.
- # Find the first ResponseOutputMessage in the output - for item in output: + texts = [] + for item in output: # Check if it's a message type (has 'role' and 'content' attributes) if hasattr(item, "type") and item.type == "message": if hasattr(item, "content"): # Content is a list of content items if isinstance(item.content, list) and len(item.content) > 0: - # Get the first text content - first_content = item.content[0] - if hasattr(first_content, "text"): - return first_content.text - elif hasattr(first_content, "type") and first_content.type == "text": - return getattr(first_content, "text", "") - return "" + for c in item.content: + if hasattr(c, "text"): + texts.append(c.text) + elif hasattr(c, "type") and c.type == "text": + t = getattr(c, "text", "") + if t: + texts.append(t) + if texts: + return "\n".join(texts) logger.warning( f"[OpenAIProvider] No message found in output array with {len(output)} items" ) return ""
127-129: Guard against missing usage fields.usage can be None; default to 0s to avoid AttributeError.
- input_tokens=response.usage.input_tokens, - output_tokens=response.usage.output_tokens, - total_tokens=response.usage.total_tokens, + input_tokens=getattr(getattr(response, "usage", None), "input_tokens", 0), + output_tokens=getattr(getattr(response, "usage", None), "output_tokens", 0), + total_tokens=getattr(getattr(response, "usage", None), "total_tokens", 0),backend/app/services/llm/jobs.py (5)
28-37: Persist Celery task_id to the job record.Store task_id to correlate job↔task and aid ops.
try: task_id = start_high_priority_job( @@ ) except Exception as e: @@ ) - logger.info( + # Persist task_id for observability + job_crud.update(job_id=job.id, job_update=JobUpdate(task_id=task_id)) + + logger.info( f"[start_job] Job scheduled for LLM call | job_id={job.id}, project_id={project_id}, task_id={task_id}" )Also applies to: 42-46
62-70: Wrap request parsing/logging in try to ensure FAILED status on early errors.If model parsing/logging fails, the job never moves to FAILED.
-def execute_job( +def execute_job( request_data: dict, @@ ) -> LLMCallResponse | None: - """Celery task to process an LLM request asynchronously.""" - request = LLMCallRequest(**request_data) - job_id_uuid = UUID(job_id) - - logger.info( - f"[execute_job] Starting LLM job execution | job_id={job_id}, task_id={task_id}, " - f"provider={request.llm.provider}, model={request.llm.llm_model_spec.model}" - ) - - try: + """Celery task to process an LLM request asynchronously.""" + try: + request = LLMCallRequest(**request_data) + job_id_uuid = UUID(job_id) + logger.info( + f"[execute_job] Starting LLM job execution | job_id={job_id}, task_id={task_id}, " + f"provider={request.llm.llm_model_spec.provider}, model={request.llm.llm_model_spec.model}" + )Also applies to: 71-79
60-61: Silence Ruff ARG001 for unused Celery task arg.Prefix with underscore or remove if not required by signature.
- task_instance, + _task_instance,
86-93: Minor: avoid redundant JobCrud re-instantiation.You already have job_crud in the same context; reuse it.
- job_crud = JobCrud(session=session) - job_crud.update( + job_crud.update( job_id=job_id_uuid, job_update=JobUpdate( status=JobStatus.FAILED, error_message=error_msg ), )
79-83: Define client before conditional to satisfy analyzers.Initialize client to None before the provider branch; keeps scope clear.
- provider_type = request.llm.llm_model_spec.provider + provider_type = request.llm.llm_model_spec.provider + client = None if provider_type == "openai": client = get_openai_client(session, organization_id, project_id) else: @@ - response, error = execute_llm_call( + response, error = execute_llm_call( request=request, client=client, )Also applies to: 95-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
backend/app/alembic/versions/219033c644de_add_llm_im_jobs_table.py(1 hunks)backend/app/api/main.py(2 hunks)backend/app/api/routes/llm.py(1 hunks)backend/app/models/__init__.py(1 hunks)backend/app/models/job.py(1 hunks)backend/app/models/llm/__init__.py(1 hunks)backend/app/models/llm/config.py(1 hunks)backend/app/models/llm/request.py(1 hunks)backend/app/models/llm/response.py(1 hunks)backend/app/services/llm/__init__.py(1 hunks)backend/app/services/llm/jobs.py(1 hunks)backend/app/services/llm/orchestrator.py(1 hunks)backend/app/services/llm/providers/__init__.py(1 hunks)backend/app/services/llm/providers/base.py(1 hunks)backend/app/services/llm/providers/factory.py(1 hunks)backend/app/services/llm/providers/openai.py(1 hunks)backend/app/services/llm/specs/__init__.py(1 hunks)backend/app/services/llm/specs/openai.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use type hints in Python code (Python 3.11+ project)
Files:
backend/app/models/job.pybackend/app/services/llm/orchestrator.pybackend/app/services/llm/providers/__init__.pybackend/app/services/llm/providers/factory.pybackend/app/services/llm/__init__.pybackend/app/api/main.pybackend/app/models/__init__.pybackend/app/models/llm/response.pybackend/app/services/llm/providers/base.pybackend/app/services/llm/specs/__init__.pybackend/app/api/routes/llm.pybackend/app/services/llm/providers/openai.pybackend/app/models/llm/__init__.pybackend/app/services/llm/jobs.pybackend/app/models/llm/config.pybackend/app/services/llm/specs/openai.pybackend/app/alembic/versions/219033c644de_add_llm_im_jobs_table.pybackend/app/models/llm/request.py
backend/app/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Define SQLModel entities (database tables and domain objects) in backend/app/models/
Files:
backend/app/models/job.pybackend/app/models/__init__.pybackend/app/models/llm/response.pybackend/app/models/llm/__init__.pybackend/app/models/llm/config.pybackend/app/models/llm/request.py
backend/app/services/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement business logic services under backend/app/services/
Files:
backend/app/services/llm/orchestrator.pybackend/app/services/llm/providers/__init__.pybackend/app/services/llm/providers/factory.pybackend/app/services/llm/__init__.pybackend/app/services/llm/providers/base.pybackend/app/services/llm/specs/__init__.pybackend/app/services/llm/providers/openai.pybackend/app/services/llm/jobs.pybackend/app/services/llm/specs/openai.py
backend/app/api/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Expose FastAPI REST endpoints under backend/app/api/ organized by domain
Files:
backend/app/api/main.pybackend/app/api/routes/llm.py
🧬 Code graph analysis (13)
backend/app/services/llm/orchestrator.py (4)
backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)backend/app/services/llm/providers/factory.py (2)
ProviderFactory(16-67)create_provider(35-58)backend/app/services/llm/providers/base.py (1)
execute(36-59)
backend/app/services/llm/providers/__init__.py (3)
backend/app/services/llm/providers/base.py (1)
BaseProvider(14-67)backend/app/services/llm/providers/factory.py (1)
ProviderFactory(16-67)backend/app/services/llm/providers/openai.py (1)
OpenAIProvider(25-157)
backend/app/services/llm/providers/factory.py (2)
backend/app/services/llm/providers/base.py (1)
BaseProvider(14-67)backend/app/services/llm/providers/openai.py (1)
OpenAIProvider(25-157)
backend/app/services/llm/__init__.py (4)
backend/app/services/llm/orchestrator.py (1)
execute_llm_call(17-64)backend/app/services/llm/providers/base.py (1)
BaseProvider(14-67)backend/app/services/llm/providers/factory.py (1)
ProviderFactory(16-67)backend/app/services/llm/providers/openai.py (1)
OpenAIProvider(25-157)
backend/app/models/__init__.py (3)
backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)backend/app/models/llm/config.py (2)
LLMConfig(37-51)LLMModelSpec(12-34)
backend/app/services/llm/providers/base.py (3)
backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)backend/app/services/llm/providers/openai.py (1)
execute(89-157)
backend/app/services/llm/specs/__init__.py (1)
backend/app/services/llm/specs/openai.py (1)
OpenAISpec(15-197)
backend/app/api/routes/llm.py (4)
backend/app/models/auth.py (1)
AuthContext(18-21)backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/services/llm/jobs.py (1)
start_job(20-51)backend/app/utils.py (2)
APIResponse(29-53)success_response(36-39)
backend/app/services/llm/providers/openai.py (4)
backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)backend/app/services/llm/providers/base.py (2)
BaseProvider(14-67)execute(36-59)backend/app/services/llm/specs/openai.py (3)
OpenAISpec(15-197)from_llm_request(173-197)to_api_params(101-170)
backend/app/models/llm/__init__.py (3)
backend/app/models/llm/config.py (2)
LLMConfig(37-51)LLMModelSpec(12-34)backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)
backend/app/services/llm/jobs.py (7)
backend/app/celery/utils.py (1)
start_high_priority_job(18-43)backend/app/crud/jobs.py (1)
JobCrud(11-42)backend/app/models/job.py (3)
JobType(16-18)JobStatus(9-13)JobUpdate(47-50)backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)backend/app/models/llm/response.py (1)
LLMCallResponse(8-28)backend/app/services/llm/orchestrator.py (1)
execute_llm_call(17-64)backend/app/utils.py (1)
get_openai_client(175-205)
backend/app/services/llm/specs/openai.py (1)
backend/app/models/llm/request.py (1)
LLMCallRequest(11-23)
backend/app/models/llm/request.py (1)
backend/app/models/llm/config.py (1)
LLMConfig(37-51)
🪛 Ruff (0.14.1)
backend/app/api/routes/llm.py
25-25: Local variable job_id is assigned to but never used
Remove assignment to unused variable job_id
(F841)
backend/app/services/llm/jobs.py
60-60: Unused function argument: task_instance
(ARG001)
🔇 Additional comments (11)
backend/app/alembic/versions/219033c644de_add_llm_im_jobs_table.py (2)
19-21: LGTM! Migration safely adds enum value.The migration correctly uses
IF NOT EXISTSto make the enum addition idempotent, which is the recommended approach for Postgres enum extensions.
23-24: Empty downgrade is acceptable for enum additions.Removing enum values from Postgres is risky if any rows reference them, so leaving downgrade as a no-op is a reasonable choice. If you need to support rollback, consider checking for usage before removal.
backend/app/models/job.py (1)
16-18: LGTM! JobType enum extended correctly.The new
LLM_APIenum member aligns with the Alembic migration and follows the existing pattern.backend/app/api/main.py (1)
10-10: LGTM! LLM router wired correctly.The new LLM router follows the established pattern for including routers in the API.
Also applies to: 35-35
backend/app/services/llm/specs/__init__.py (1)
1-3: LGTM! Clean module exports.The re-export follows standard Python packaging conventions.
backend/app/services/llm/providers/__init__.py (1)
1-15: LGTM! Well-documented module exports.The provider module exports follow best practices with clear documentation and appropriate public API surface.
backend/app/models/__init__.py (1)
51-56: Public API re-export looks goodRe-exporting LLM models here improves discoverability and import ergonomics. LGTM.
backend/app/models/llm/__init__.py (1)
1-21: Clean aggregation of LLM modelsWell-scoped
__all__with explicit exports; clear and maintainable.backend/app/services/llm/__init__.py (1)
15-22: Public surface looks coherent.execute_llm_call and provider types are neatly exported. No blockers.
backend/app/services/llm/specs/openai.py (2)
172-197: from_llm_request mapping is clean.max_tokens→max_output_tokens mapping and vector store fields are correctly bridged.
152-161: Code implementation is correct per OpenAI Responses API specification.The schema matches exactly: "tools" array with file_search type, vector_store_ids as a list, optional max_num_results, and include=["file_search_call.results"] is a valid include value that returns file_search tool call results. No changes needed.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… registry for provider instantiation, and update OpenAI provider execution logic.
…y comments, and clarify provider responsibilities.
…update response handling
…m_call, reorganize imports, and enhance logging in OpenAIProvider.
1 participant
Summary
Target issue is #PLEASE_TYPE_ISSUE_NUMBER
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
Release Notes
/llm/callAPI endpoint for language model requests with configurable parameters including model selection, temperature, and token limits.