Merge pull request #74 from umago/feedback-endpoint

Add /feedback endpoint

Author: tisnik

lightspeed-stack.yaml

@@ -15,3 +15,6 @@ llama_stack:
   # library_client_config_path: <path-to-llama-stack-run.yaml-file>
   url: http://localhost:8321
   api_key: xyzzy
+user_data_collection:
+  feedback_disabled: false
+  feedback_storage: "/tmp/data/feedback"

pdm.lock

@@ -23,11 +23,13 @@ dev = [
     "pytest>=8.3.2",
     "pytest-cov>=5.0.0",
     "pytest-mock>=3.14.0",
+    "pytest-asyncio>=1.0.0",
     "pyright>=1.1.401",
     "pylint>=3.3.7",
 ]
 
 [tool.pytest.ini_options]
+asyncio_mode = "auto"
 pythonpath = [
     "src"
 ]

pyproject.toml

@@ -0,0 +1,153 @@
+"""Handler for REST API call to provide info."""
+
+import logging
+from typing import Any
+from pathlib import Path
+import json
+from datetime import datetime, UTC
+
+from fastapi import APIRouter, Request, HTTPException, Depends, status
+
+from configuration import configuration
+from models.responses import FeedbackResponse, StatusResponse
+from models.requests import FeedbackRequest
+from utils.suid import get_suid
+
+logger = logging.getLogger(__name__)
+router = APIRouter(prefix="/feedback", tags=["feedback"])
+
+# Response for the feedback endpoint
+feedback_response: dict[int | str, dict[str, Any]] = {
+    200: {"response": "Feedback received and stored"},
+}
+
+
+def is_feedback_enabled() -> bool:
+    """Check if feedback is enabled.
+
+    Returns:
+        bool: True if feedback is enabled, False otherwise.
+    """
+    return not configuration.user_data_collection_configuration.feedback_disabled
+
+
+# TODO(lucasagomes): implement this function to retrieve user ID from auth
+def retrieve_user_id(auth: Any) -> str:  # pylint: disable=unused-argument
+    """Retrieve the user ID from the authentication handler.
+
+    Args:
+        auth: The Authentication handler (FastAPI Depends) that will
+            handle authentication Logic.
+
+    Returns:
+        str: The user ID.
+    """
+    return "user_id_placeholder"
+
+
+# TODO(lucasagomes): implement this function to handle authentication
+async def auth_dependency(_request: Request) -> bool:
+    """Authenticate dependency to ensure the user is authenticated.
+
+    Args:
+        request (Request): The FastAPI request object.
+
+    Raises:
+        HTTPException: If the user is not authenticated.
+    """
+    return True
+
+
+async def assert_feedback_enabled(_request: Request) -> None:
+    """Check if feedback is enabled.
+
+    Args:
+        request (Request): The FastAPI request object.
+
+    Raises:
+        HTTPException: If feedback is disabled.
+    """
+    feedback_enabled = is_feedback_enabled()
+    if not feedback_enabled:
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN,
+            detail="Forbidden: Feedback is disabled",
+        )
+
+
+@router.post("", responses=feedback_response)
+def feedback_endpoint_handler(
+    _request: Request,
+    feedback_request: FeedbackRequest,
+    _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled),
+    auth: Any = Depends(auth_dependency),
+) -> FeedbackResponse:
+    """Handle feedback requests.
+
+    Args:
+        feedback_request: The request containing feedback information.
+        ensure_feedback_enabled: The feedback handler (FastAPI Depends) that
+            will handle feedback status checks.
+        auth: The Authentication handler (FastAPI Depends) that will
+            handle authentication Logic.
+
+    Returns:
+        Response indicating the status of the feedback storage request.
+    """
+    logger.debug("Feedback received %s", str(feedback_request))
+
+    user_id = retrieve_user_id(auth)
+    try:
+        store_feedback(user_id, feedback_request.model_dump(exclude={"model_config"}))
+    except Exception as e:
+        logger.error("Error storing user feedback: %s", e)
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail={
+                "response": "Error storing user feedback",
+                "cause": str(e),
+            },
+        ) from e
+
+    return FeedbackResponse(response="feedback received")
+
+
+def store_feedback(user_id: str, feedback: dict) -> None:
+    """Store feedback in the local filesystem.
+
+    Args:
+        user_id: The user ID (UUID).
+        feedback: The feedback to store.
+    """
+    logger.debug("Storing feedback for user %s", user_id)
+    # Creates storage path only if it doesn't exist. The `exist_ok=True` prevents
+    # race conditions in case of multiple server instances trying to set up storage
+    # at the same location.
+    storage_path = Path(
+        configuration.user_data_collection_configuration.feedback_storage or ""
+    )
+    storage_path.mkdir(parents=True, exist_ok=True)
+
+    current_time = str(datetime.now(UTC))
+    data_to_store = {"user_id": user_id, "timestamp": current_time, **feedback}
+
+    # stores feedback in a file under unique uuid
+    feedback_file_path = storage_path / f"{get_suid()}.json"
+    with open(feedback_file_path, "w", encoding="utf-8") as feedback_file:
+        json.dump(data_to_store, feedback_file)
+
+    logger.info("Feedback stored sucessfully at %s", feedback_file_path)
+
+
+@router.get("/status")
+def feedback_status() -> StatusResponse:
+    """Handle feedback status requests.
+
+    Returns:
+        Response indicating the status of the feedback.
+    """
+    logger.debug("Feedback status requested")
+    feedback_status_enabled = is_feedback_enabled()
+    return StatusResponse(
+        functionality="feedback", status={"enabled": feedback_status_enabled}
+    )

src/app/endpoints/feedback.py

@@ -2,7 +2,7 @@
 
 from fastapi import FastAPI
 
-from app.endpoints import info, models, root, query, health, config
+from app.endpoints import info, models, root, query, health, config, feedback
 
 
 def include_routers(app: FastAPI) -> None:
@@ -17,3 +17,4 @@ def include_routers(app: FastAPI) -> None:
     app.include_router(query.router, prefix="/v1")
     app.include_router(health.router, prefix="/v1")
     app.include_router(config.router, prefix="/v1")
+    app.include_router(feedback.router, prefix="/v1")

src/app/routers.py

@@ -4,7 +4,7 @@
 from typing import Any, Optional
 
 import yaml
-from models.config import Configuration, LLamaStackConfiguration
+from models.config import Configuration, LLamaStackConfiguration, UserDataCollection
 
 logger = logging.getLogger(__name__)
 
@@ -51,5 +51,13 @@ def llama_stack_configuration(self) -> LLamaStackConfiguration:
         ), "logic error: configuration is not loaded"
         return self._configuration.llama_stack
 
+    @property
+    def user_data_collection_configuration(self) -> UserDataCollection:
+        """Return user data collection configuration."""
+        assert (
+            self._configuration is not None
+        ), "logic error: configuration is not loaded"
+        return self._configuration.user_data_collection
+
 
 configuration: AppConfig = AppConfig()

src/configuration.py

@@ -60,9 +60,24 @@ def check_llama_stack_model(self) -> Self:
         return self
 
 
+class UserDataCollection(BaseModel):
+    """User data collection configuration."""
+
+    feedback_disabled: bool = True
+    feedback_storage: Optional[str] = None
+
+    @model_validator(mode="after")
+    def check_storage_location_is_set_when_needed(self) -> Self:
+        """Check that storage_location is set when enabled."""
+        if not self.feedback_disabled and self.feedback_storage is None:
+            raise ValueError("feedback_storage is required when feedback is enabled")
+        return self
+
+
 class Configuration(BaseModel):
     """Global service configuration."""
 
     name: str
     service: ServiceConfiguration
     llama_stack: LLamaStackConfiguration
+    user_data_collection: UserDataCollection

src/models/config.py

@@ -2,10 +2,11 @@
 
 from typing import Optional, Self
 
-from pydantic import BaseModel, model_validator
-
+from pydantic import BaseModel, model_validator, field_validator
 from llama_stack_client.types.agents.turn_create_params import Document
 
+from utils import suid
+
 
 class Attachment(BaseModel):
     """Model representing an attachment that can be send from UI as part of query.
@@ -130,3 +131,72 @@ def validate_provider_and_model(self) -> Self:
         if self.provider and not self.model:
             raise ValueError("Model must be specified if provider is specified")
         return self
+
+
+class FeedbackRequest(BaseModel):
+    """Model representing a feedback request.
+
+    Attributes:
+        conversation_id: The required conversation ID (UUID).
+        user_question: The required user question.
+        llm_response: The required LLM response.
+        sentiment: The optional sentiment.
+        user_feedback: The optional user feedback.
+
+    Example:
+        ```python
+        feedback_request = FeedbackRequest(
+            conversation_id="12345678-abcd-0000-0123-456789abcdef",
+            user_question="what are you doing?",
+            user_feedback="Great service!",
+            llm_response="I don't know",
+            sentiment=-1,
+        )
+        ```
+    """
+
+    conversation_id: str
+    user_question: str
+    llm_response: str
+    sentiment: Optional[int] = None
+    user_feedback: Optional[str] = None
+
+    # provides examples for /docs endpoint
+    model_config = {
+        "json_schema_extra": {
+            "examples": [
+                {
+                    "conversation_id": "12345678-abcd-0000-0123-456789abcdef",
+                    "user_question": "foo",
+                    "llm_response": "bar",
+                    "user_feedback": "Great service!",
+                    "sentiment": 1,
+                }
+            ]
+        }
+    }
+
+    @field_validator("conversation_id")
+    @classmethod
+    def check_uuid(cls, value: str) -> str:
+        """Check if conversation ID has the proper format."""
+        if not suid.check_suid(value):
+            raise ValueError(f"Improper conversation ID {value}")
+        return value
+
+    @field_validator("sentiment")
+    @classmethod
+    def check_sentiment(cls, value: Optional[int]) -> Optional[int]:
+        """Check if sentiment value is as expected."""
+        if value not in {-1, 1, None}:
+            raise ValueError(
+                f"Improper sentiment value of {value}, needs to be -1 or 1"
+            )
+        return value
+
+    @model_validator(mode="after")
+    def check_sentiment_or_user_feedback_set(self) -> Self:
+        """Ensure that either 'sentiment' or 'user_feedback' is set."""
+        if self.sentiment is None and self.user_feedback is None:
+            raise ValueError("Either 'sentiment' or 'user_feedback' must be set")
+        return self