LCORE-945: optional topic summary #812
Conversation
|
Warning Rate limit exceeded@radofuchs has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughQueryRequest gained a new optional field Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryEndpoint
participant ConversationStore
participant TopicSvc as get_topic_summary
participant TranscriptStore
Client->>QueryEndpoint: send QueryRequest (generate_topic_summary: opt)
QueryEndpoint->>ConversationStore: fetch conversation by id
alt conversation exists
QueryEndpoint->>TranscriptStore: (maybe) persist transcript
QueryEndpoint->>QueryEndpoint: proceed without topic-summary generation
else new conversation
opt generate_topic_summary == true
QueryEndpoint->>TopicSvc: get_topic_summary(query, conversation context)
TopicSvc-->>QueryEndpoint: topic_summary
end
opt generate_topic_summary == false
QueryEndpoint-->>QueryEndpoint: set topic_summary = None
end
QueryEndpoint->>TranscriptStore: persist turn (includes topic_summary if any)
end
QueryEndpoint-->>Client: stream/return response (includes topic_summary field)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (1)
tests/unit/models/requests/test_query_request.py (1)
158-168: Consider adding a test for the default value.The tests properly verify explicit True and False values, but there's no test verifying that the default value is True when the field is not provided. Consider adding:
def test_generate_topic_summary_default(self) -> None: """Test that generate_topic_summary defaults to True.""" qr = QueryRequest(query="Tell me about Kubernetes") assert qr.generate_topic_summary is TrueThis would ensure the default behavior is explicitly tested and documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/endpoints/query.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/models/requests.py(3 hunks)tests/unit/app/endpoints/test_query.py(1 hunks)tests/unit/models/requests/test_query_request.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/app/endpoints/test_query.py (6)
tests/unit/app/endpoints/test_conversations.py (1)
dummy_request(34-44)src/utils/types.py (1)
TurnSummary(89-163)src/utils/token_counter.py (1)
TokenCounter(18-41)tests/unit/app/endpoints/test_streaming_query.py (1)
mock_database_operations(72-85)src/app/endpoints/query.py (1)
query_endpoint_handler(438-460)src/models/requests.py (1)
QueryRequest(73-233)
src/app/endpoints/streaming_query.py (1)
src/app/endpoints/query.py (1)
get_topic_summary(184-214)
tests/unit/models/requests/test_query_request.py (1)
src/models/requests.py (1)
QueryRequest(73-233)
⏰ 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). (3)
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (7)
src/models/requests.py (3)
84-84: LGTM!The docstring correctly documents the new
generate_topic_summaryattribute.
150-154: LGTM!The field definition is correct with appropriate type, default value, description, and examples. The default value of
Trueensures backward compatibility.
174-174: LGTM!Including
generate_topic_summaryin the model configuration examples ensures it's properly documented in the API schema.src/app/endpoints/query.py (1)
315-327: LGTM!The conditional topic summary generation logic is well-implemented:
- Properly checks the
generate_topic_summaryflag- Includes helpful debug logging for both paths
- Explicitly sets
topic_summaryto None when disabled for claritysrc/app/endpoints/streaming_query.py (1)
865-876: LGTM!The conditional topic summary generation logic is correctly implemented and consistent with the non-streaming endpoint in
query.py.tests/unit/app/endpoints/test_query.py (2)
2266-2311: LGTM!The test properly verifies that topic summary generation occurs by default for new conversations. The test setup and assertions are correct.
2314-2360: LGTM!The test properly verifies that topic summary generation is skipped when explicitly disabled. The use of
assert_not_called()correctly validates the expected behavior.
radofuchs
force-pushed
the
LCORE_945_optional_topic_summary
branch
from
a5274f0 to
ecd4ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/requests.py (1)
84-85: Clarifygenerate_topic_summarysemantics; consider non‑optional boolThe new field is nicely documented and used downstream, but its current typing gives it subtle tri‑state behavior:
- Declared as
Optional[bool] = True, but consumed via truthiness checks.- This means:
- Field omitted →
True(generate summary).- Explicit
False→ don’t generate (as intended).- Explicit
null→ also treated as “don’t generate” (becauseNoneis falsy).If you don’t need a distinct “null disables” state, simplifying to a plain
boolmakes the contract clearer and avoids accidental disabling vianull:- generate_topic_summary: Optional[bool] = Field( - True, - description="Whether to generate topic summary for new conversations", - examples=[True, False], - ) + generate_topic_summary: bool = Field( + True, + description="Whether to generate topic summary for new conversations (default: True)", + examples=[True, False], + )This keeps the default and opt-out behavior but removes the extra
Nonestate and documents the default explicitly.Also applies to: 150-154, 174-175
tests/unit/utils/test_endpoints.py (1)
1041-1140: Newcleanup_after_streamingtests accurately cover default vs explicit opt‑outThe two async tests do a good job of isolating the new behavior:
- Default construction of
QueryRequest(no flag) triggers topic-summary generation and persists the generated value.- Explicit
generate_topic_summary=Falsecleanly skips the generator and persiststopic_summary=None.The mocking of
get_session,create_referenced_documents_with_metadata, andstore_conversation_into_cachekeeps the tests focused on the gating logic and propagation intopersist_user_conversation_details_func.If you want even stronger coverage, you could also assert that
store_conversation_into_cacheis called with the expectedtopic_summary, but that’s optional given the current assertions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/app/endpoints/query.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/models/requests.py(3 hunks)src/utils/endpoints.py(1 hunks)tests/unit/app/endpoints/test_query.py(2 hunks)tests/unit/models/requests/test_query_request.py(1 hunks)tests/unit/utils/test_endpoints.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/endpoints/streaming_query.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/models/requests/test_query_request.py
- src/app/endpoints/query.py
- tests/unit/app/endpoints/test_query.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/utils/test_endpoints.py (2)
src/models/requests.py (1)
QueryRequest(73-233)src/utils/endpoints.py (1)
cleanup_after_streaming(598-717)
⏰ 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: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (1)
src/utils/endpoints.py (1)
674-684: Topic-summary gating logic incleanup_after_streaminglooks consistentThe new
generate_topic_summarygate for new conversations is wired correctly: it only runs for non-existing conversations, callsget_topic_summary_funcwhen truthy, and cleanly skips with a debug log otherwise. This aligns with the new flag and the added tests that cover defaultTrueand explicitFalsebehavior.If you expect
generate_topic_summary=None(explicit JSONnull) to behave differently fromFalse, you may want to double-check upstream usage and, if needed, special-caseNoneinstead of relying on raw truthiness.
radofuchs
force-pushed
the
LCORE_945_optional_topic_summary
branch
from
5a8f80f to
2e91d95
Compare
2 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Tests