user feeback validation changes LCORE-197 #93

omkarjoshi0304 · 2025-06-11T21:52:37Z

## Description

Type of change

Related Tickets & Documents

Related Issue #
Closes #

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.

lpiwowar

Really solid start!:) 💪 You were really quick with proposing the PR. Let's iterate on this:).

Also, two tips:

How to write a good commit message -> https://cbea.ms/git-commit/
You should add a proof of testing comment that showcases how you tested your change. I've learnt this from Chris. He writes nice proofs of testing comments [1]. It is a requirement for this repo:

[1] openstack-lightspeed/rag-content#35 (comment)

lpiwowar · 2025-06-12T10:59:38Z

data/feedback/14c22031-eb3d-47c9-ad78-0a4658763cbc.json

@@ -0,0 +1 @@
+{"user_id": "user_id_placeholder", "timestamp": "2025-06-09 22:19:56.810345+00:00", "conversation_id": "12345678-abcd-0000-0123-456789abcdef", "user_question": "sasddfghlljhkgfdghjkl;kjhgfdfghjklkjhiugyftdsfghjkliuyftdrsadtfyuiopiuytrsdfghj", "llm_response": "bar", "sentiment": 1, "user_feedback": "Great service!"}


We probably do not want to include the /data folder:).

Indeed, needs to be removed from the commit

lpiwowar · 2025-06-12T11:00:29Z

tests/unit/models/test_requests.py

+    with pytest.raises(ValidationError) as exc_info:
+        FeedbackRequest(
+            conversation_id="123e4567-e89b-12d3-a456-426614174000",
+            user_question="What is OpenStack?",
+            llm_response="OpenStack is a cloud computing platform.",
+            user_feedback="x" * 4097,
+        )
+    assert "should have at most 4096 characters" in str(exc_info.value)


Indentation needs to be fixed:). Also, you can probably use match in pytest.rases() . You can check out the test above [1].

[1] https://github.com/lightspeed-core/lightspeed-stack/pull/93/files#diff-49fd4c68eb1f80b46e87791cc49551a3138b7106dc9b3ed416dcbf21fbcbaabcR170

lpiwowar · 2025-06-12T11:05:01Z

src/app/main.py

 async def startup_event() -> None:
    """Perform logger setup on service startup."""
-    get_logger("app.endpoints.handlers")
+get_logger("app.endpoints.handlers")


Indent should be fixed and new line at the end of the file is missing.

lpiwowar · 2025-06-12T11:17:43Z

src/models/requests.py

+        min_length=1,
+        max_length=4096,
+        description="Maximum length of user feedback is 4096 characters long.",
+        examples=["How to Set Up Kubernetes?"],


The example field should most likely contain something like this:

Suggested change

examples=["How to Set Up Kubernetes?"],

examples=["I'm not satisfied with the response because it is too vague."],

You can of course come up with something else. Just an idea:)

lpiwowar · 2025-06-12T11:18:51Z

src/models/requests.py

+        default=None,
+        min_length=1,
+        max_length=4096,
+        description="Maximum length of user feedback is 4096 characters long.",


I think the descriptions field is mostly meant to be used for describing the purpose of the field. I think we can drop the information about the maximum length here. What do you think?

Suggested change

description="Maximum length of user feedback is 4096 characters long.",

description="Feedback on the LLM response.",

lpiwowar · 2025-06-12T11:19:19Z

src/models/requests.py

+    # Optional user feedback limited to 1–4096 characters to prevent abuse.
+    user_feedback: Optional[str] = Field(
+        default=None,
+        min_length=1,


From where I stand, I think we shouldn't be enforcing a min_length on the user_feedback. Maybe the lightspeed-core team has a different opinion on this but for now I would probably remove it.

Suggested change

min_length=1,

lpiwowar · 2025-06-12T11:21:09Z

tests/unit/models/test_requests.py

@@ -1,5 +1,5 @@
 import pytest
-
+from pydantic import ValidationError


I would keep the empty line here:).

Suggested change

from pydantic import ValidationError

import pytest

from pydantic import ValidationError

from models.requests import QueryRequest, Attachment, FeedbackRequest

From PEP-8:

Imports should be grouped in the following order: 1. Standard library imports. 2. Related third party imports. 3. Local application/library specific imports. You should put a blank line between each group of imports.

https://peps.python.org/pep-0008/#imports

thanks @lpiwowar for review! Agree with your comments

tisnik

pls. remove those /data/* files. TYVM

umago

Thanks Omkar, comments inline

umago · 2025-06-12T13:55:56Z

data/feedback/14c22031-eb3d-47c9-ad78-0a4658763cbc.json

@@ -0,0 +1 @@
+{"user_id": "user_id_placeholder", "timestamp": "2025-06-09 22:19:56.810345+00:00", "conversation_id": "12345678-abcd-0000-0123-456789abcdef", "user_question": "sasddfghlljhkgfdghjkl;kjhgfdfghjklkjhiugyftdsfghjkliuyftdrsadtfyuiopiuytrsdfghj", "llm_response": "bar", "sentiment": 1, "user_feedback": "Great service!"}


Indeed, needs to be removed from the commit

umago · 2025-06-12T13:57:32Z

src/models/requests.py

+        default=None,
+        min_length=1,
+        max_length=4096,
+        description="Maximum length of user feedback is 4096 characters long.",


umago · 2025-06-12T13:58:54Z

tests/unit/models/test_requests.py

+
+    def test_user_feedback_too_short(self) -> None:
+        """Test that user_feedback shorter than 1 character raises ValidationError."""
+    with pytest.raises(ValidationError) as exc_info:


Identation here is off.

Btw, unit-tests can be run locally as:

$ pdm test-unit

umago · 2025-06-12T13:59:13Z

src/models/requests.py

+    # Optional user feedback limited to 1–4096 characters to prevent abuse.
+    user_feedback: Optional[str] = Field(
+        default=None,
+        min_length=1,


tisnik · 2025-06-12T18:43:07Z

now it's much better. Pls just rerun black on your sources to have them reformatted, and you are done

umago · 2025-06-12T18:49:27Z

src/app/main.py

 async def startup_event() -> None:
    """Perform logger setup on service startup."""
-    get_logger("app.endpoints.handlers")
+get_logger("app.endpoints.handlers")


This change is outside the scope of the change

umago · 2025-06-12T18:50:26Z

now it's much better. Pls just rerun black on your sources to have them reformatted, and you are done

And get rid of the data/feedback pls (-:

…ity checks

user feeback validation changes

b25af3f

lpiwowar reviewed Jun 12, 2025

View reviewed changes

Update main.py

cff6715

tisnik requested changes Jun 12, 2025

View reviewed changes

umago reviewed Jun 12, 2025

View reviewed changes

omkarjoshi0304 and others added 3 commits June 13, 2025 11:57

Fix feedback field validation, remove test data, align with code qual…

bb14bac

…ity checks

Update main.py

c74db01

Remove data/ from .gitignore

8f1ea75

tisnik merged commit 1580a29 into lightspeed-core:main Jun 13, 2025
18 checks passed

		@@ -0,0 +1 @@
		{"user_id": "user_id_placeholder", "timestamp": "2025-06-09 22:19:56.810345+00:00", "conversation_id": "12345678-abcd-0000-0123-456789abcdef", "user_question": "sasddfghlljhkgfdghjkl;kjhgfdfghjklkjhiugyftdsfghjkliuyftdrsadtfyuiopiuytrsdfghj", "llm_response": "bar", "sentiment": 1, "user_feedback": "Great service!"} No newline at end of file

	examples=["How to Set Up Kubernetes?"],
	examples=["I'm not satisfied with the response because it is too vague."],

	description="Maximum length of user feedback is 4096 characters long.",
	description="Feedback on the LLM response.",

		@@ -1,5 +1,5 @@
		import pytest

		from pydantic import ValidationError

user feeback validation changes LCORE-197 #93

user feeback validation changes LCORE-197 #93

Uh oh!

Conversation

omkarjoshi0304 commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

lpiwowar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisnik left a comment

Choose a reason for hiding this comment

Uh oh!

umago left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisnik commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umago commented Jun 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

omkarjoshi0304 commented Jun 11, 2025 •

edited

Loading

umago left a comment •

edited

Loading