LCORE-1953: Added checks for None scores#1556
Conversation
WalkthroughBYOK RAG integration tests were updated to assert that all returned Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/endpoints/test_query_byok_integration.py`:
- Around line 993-1000: The list comprehension that builds scores unnecessarily
filters out None even though just above you assert every chunk.score is not
None; remove the redundant "if chunk.score is not None" from the comprehension
that creates scores (use [chunk.score for chunk in response.rag_chunks]) so any
unexpected None will fail the test instead of being silently dropped; apply the
same change to the parallel block that uses response.rag_chunks around lines
1084-1090 to keep the list[float] annotation accurate and tests strict.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1052a92e-cf38-4df5-9b71-44585b3fd63f
📒 Files selected for processing (1)
tests/integration/endpoints/test_query_byok_integration.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). (17)
- GitHub Check: list_outdated_dependencies
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.13)
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pyright
- GitHub Check: mypy
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Import FastAPI dependencies with:from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Use@model_validatorand@field_validatorfor Pydantic model validation
Complete type annotations for all class attributes; use specific types, notAny
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections
Files:
tests/integration/endpoints/test_query_byok_integration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Use markerpytest.mark.asynciofor async tests
Unit tests require 60% coverage, integration tests 10%
Files:
tests/integration/endpoints/test_query_byok_integration.py
🔇 Additional comments (2)
tests/integration/endpoints/test_query_byok_integration.py (2)
1084-1090: Duplicate of the pattern above.Same redundancy as lines 993-1000: the preceding loop already asserts non-None, so the filter in the comprehension is unnecessary. See the suggestion on the earlier block; apply the equivalent simplification here.
926-928: LGTM — None guards make the comparison type-safe.Adding explicit
is not Nonechecks before the>comparison prevents aTypeErrorif either score is everNoneand keeps mypy happy givenRAGChunk.score: Optional[float].
| # Check that the score is computed properly | ||
| for chunk in response.rag_chunks: | ||
| assert chunk.score is not None | ||
|
|
||
| # Verify chunks are sorted by score descending (highest first) | ||
| scores = [chunk.score for chunk in response.rag_chunks] | ||
| scores: list[float] = [ | ||
| chunk.score for chunk in response.rag_chunks if chunk.score is not None | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant None filter after asserting non-None.
Lines 994-995 already assert every chunk.score is not None, so the if chunk.score is not None filter in the list comprehension on line 999 is dead code and weakens the test: if a None slipped through the assertion loop (it can't, but hypothetically), the filter would silently drop it rather than letting sorted() comparison fail. Given the preceding assertion, the comprehension can drop the filter and keep the explicit list[float] annotation honest.
♻️ Proposed simplification
# Check that the score is computed properly
for chunk in response.rag_chunks:
assert chunk.score is not None
# Verify chunks are sorted by score descending (highest first)
- scores: list[float] = [
- chunk.score for chunk in response.rag_chunks if chunk.score is not None
- ]
+ scores: list[float] = [chunk.score for chunk in response.rag_chunks]
assert scores == sorted(scores, reverse=True)The same applies to the parallel block at lines 1084-1090.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check that the score is computed properly | |
| for chunk in response.rag_chunks: | |
| assert chunk.score is not None | |
| # Verify chunks are sorted by score descending (highest first) | |
| scores = [chunk.score for chunk in response.rag_chunks] | |
| scores: list[float] = [ | |
| chunk.score for chunk in response.rag_chunks if chunk.score is not None | |
| ] | |
| # Check that the score is computed properly | |
| for chunk in response.rag_chunks: | |
| assert chunk.score is not None | |
| # Verify chunks are sorted by score descending (highest first) | |
| scores: list[float] = [chunk.score for chunk in response.rag_chunks] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_byok_integration.py` around lines 993
- 1000, The list comprehension that builds scores unnecessarily filters out None
even though just above you assert every chunk.score is not None; remove the
redundant "if chunk.score is not None" from the comprehension that creates
scores (use [chunk.score for chunk in response.rag_chunks]) so any unexpected
None will fail the test instead of being silently dropped; apply the same change
to the parallel block that uses response.rag_chunks around lines 1084-1090 to
keep the list[float] annotation accurate and tests strict.
1 participant
Description
LCORE-1953: Added checks for
NonescoresType of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit