Refactor rlsapi v1 infer endpoint to deduplicate error handling#1249
Merged
tisnik merged 2 commits intolightspeed-core:mainfrom Mar 2, 2026
Merged
Refactor rlsapi v1 infer endpoint to deduplicate error handling#1249tisnik merged 2 commits intolightspeed-core:mainfrom
tisnik merged 2 commits intolightspeed-core:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ 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 (1)
WalkthroughCentralizes inference exception handling, adds helpers for base prompt and configured default model naming, changes model resolution (retrieve_simple_response accepts optional model_id and raises 503 when no default model), and routes inference failures through unified mapping and telemetry. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant API as API (/infer)
participant Builder as _build_instructions/_get_base_prompt
participant Retriever as retrieve_simple_response
participant Backend as ModelBackend
participant Telemetry as _queue_splunk_event
participant Recorder as _record_inference_failure
Client->>API: POST /infer (question, options)
API->>Builder: build instructions (uses _get_base_prompt)
API->>Retriever: call retrieve_simple_response(..., model_id)
Retriever->>Backend: invoke model inference
alt success
Backend-->>Retriever: model response
Retriever-->>API: return text
API->>Telemetry: enqueue event (uses _get_configured_default_model_name)
API-->>Client: 200 OK (response)
else handled error
Backend-->>Retriever: raises exception
Retriever-->>API: propagate error
API->>Recorder: _record_inference_failure(request_id, model_id, error)
API->>API: _map_inference_error_to_http_exception(error, model_id, request_id)
API-->>Client: mapped HTTPException (e.g., 429/503/502)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
128-135: Expand new helper docstrings to the project-required format.The new helper docstrings are brief but incomplete; they should include full Google-style sections (Parameters/Returns/Raises as applicable).
As per coding guidelines: "All functions must have complete docstrings with brief descriptions" and "Follow Google Python docstring conventions for modules, classes, and functions with Parameters, Returns, Raises, and Attributes sections".
Also applies to: 220-224, 296-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 128 - 135, The _get_base_prompt helper currently has a minimal docstring; expand it to a full Google-style docstring for functions: include a one-line summary, a "Returns" section specifying it returns a str (the resolved system prompt), a "Parameters" section (explicitly state there are none) and a "Raises" section if applicable (or state none), and place it directly above def _get_base_prompt so tooling and linters pick it up; apply the same Google-style docstring expansion to the other small helper functions in this module that the reviewer flagged so all helper functions follow the project's docstring convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 302-304: The runtime error check that looks for "context_length"
is too narrow and should detect variants like "context length" (and be
case-insensitive) so prompt-too-long errors return 413 instead of 500; in the
block that inspects error (the branch using variables error, logger and
request_id in this file) replace the single-string check with a broader test
(e.g., normalize the message to lower-case and test for regex r"context[_
]length" or check both "context_length" and "context length" or replace
underscores with spaces before matching) and keep the existing logger.error call
and the code path that returns/raises the 413 HTTP response so both underscore
and spaced variants are handled.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 128-135: The _get_base_prompt helper currently has a minimal
docstring; expand it to a full Google-style docstring for functions: include a
one-line summary, a "Returns" section specifying it returns a str (the resolved
system prompt), a "Parameters" section (explicitly state there are none) and a
"Raises" section if applicable (or state none), and place it directly above def
_get_base_prompt so tooling and linters pick it up; apply the same Google-style
docstring expansion to the other small helper functions in this module that the
reviewer flagged so all helper functions follow the project's docstring
convention.
major
force-pushed
the
major/rlsapi-v1-simplify-types
branch
from
d0aa4cd to
0c8bdb9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
128-135: Add complete Google-style docstrings for the new helper functions.Both helpers currently use brief one-liners; they should include explicit
Returnssections to match project standards.📝 Suggested docstring update
def _get_base_prompt() -> str: - """Get the base system prompt with configuration fallback.""" + """Get the base system prompt. + + Returns: + Configured custom system prompt when available; otherwise the default + system prompt constant. + """ if ( configuration.customization is not None and configuration.customization.system_prompt is not None ): return configuration.customization.system_prompt return constants.DEFAULT_SYSTEM_PROMPT @@ def _get_configured_default_model_name() -> str: - """Get configured default model name for telemetry payloads.""" + """Get configured default model name for telemetry payloads. + + Returns: + The configured default model name, or an empty string when no inference + configuration/default model is present. + """ if configuration.inference is None: return "" return configuration.inference.default_model or ""As per coding guidelines, "All functions must have complete docstrings with brief descriptions" and "Follow Google Python docstring conventions for modules, classes, and functions with Parameters, Returns, Raises, and Attributes sections".
Also applies to: 220-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 128 - 135, The helper function _get_base_prompt and the other helper referenced at lines 220-225 need full Google-style docstrings: replace the one-line docstrings with multi-line docstrings that include a short description, a Returns section specifying the return type (str) and what it contains (the system prompt, falling back to constants.DEFAULT_SYSTEM_PROMPT), and a Raises section if applicable (or omit if none). Locate _get_base_prompt and the other helper, update their docstrings to follow the project convention (summary line, blank line, Returns: str: description), and ensure formatting matches existing Google-style docstrings in the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 329-333: The call to handle_known_apistatus_errors(error,
model_id) can return None, so before calling error_response.model_dump() you
must guard against a None result: call handle_known_apistatus_errors in the
APIStatusError/OpenAIAPIStatusError branch, check if error_response is not None
and only then return HTTPException(**error_response.model_dump()); if it is
None, re-raise the original error or return a generic HTTPException (e.g., with
a 502/500 status and the original error message) so that an AttributeError is
not raised—update the code around logger.exception, request_id, error,
handle_known_apistatus_errors, and model_dump accordingly.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 128-135: The helper function _get_base_prompt and the other helper
referenced at lines 220-225 need full Google-style docstrings: replace the
one-line docstrings with multi-line docstrings that include a short description,
a Returns section specifying the return type (str) and what it contains (the
system prompt, falling back to constants.DEFAULT_SYSTEM_PROMPT), and a Raises
section if applicable (or omit if none). Locate _get_base_prompt and the other
helper, update their docstrings to follow the project convention (summary line,
blank line, Returns: str: description), and ensure formatting matches existing
Google-style docstrings in the repo.
Contributor
|
@major please rebase first |
Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
major/rlsapi-v1-simplify-types
branch
from
0c8bdb9 to
27894f8
Compare
Contributor
Author
|
@tisnik Done! ✅ |
major
force-pushed
the
major/rlsapi-v1-simplify-types
branch
from
27894f8 to
9241e25
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/rlsapi_v1.py (1)
203-216:⚠️ Potential issue | 🔴 CriticalUse
resolved_model_idfor usage extraction to avoidNoneand fix CI failure.At Line 215,
extract_token_usagereceivesmodel_id(str | None) instead of the resolved non-optional value. This matches the reported Pyright failure and can also produce incorrect telemetry whenmodel_idis omitted.🐛 Proposed fix
- extract_token_usage(response.usage, model_id) + extract_token_usage(response.usage, resolved_model_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 203 - 216, The code calls extract_token_usage(response.usage, model_id) with the optional model_id which can be None; update the call to pass the resolved non-optional resolved_model_id instead so usage extraction always gets the actual model string. Locate the block where resolved_model_id is set and the client.responses.create call (symbols: resolved_model_id, model_id, extract_token_usage) and replace the second argument to extract_token_usage with resolved_model_id.
🧹 Nitpick comments (2)
src/app/endpoints/rlsapi_v1.py (2)
181-182: UseOptional[...]for optional annotations to match repo typing convention.The changed signatures use
str | None/HTTPException | None, while the repo guideline asks forOptional[Type]for optionals.🔧 Proposed typing alignment
-from typing import Annotated, Any, cast +from typing import Annotated, Any, Optional, cast ... - model_id: str | None = None, + model_id: Optional[str] = None, ... -) -> HTTPException | None: +) -> Optional[HTTPException]:As per coding guidelines: "Use Optional[Type] for optional type annotations."
Also applies to: 300-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 181 - 182, Change all occurrences of union-style optional annotations like "model_id: str | None = None" and "some_exc: HTTPException | None = None" to use typing.Optional (e.g., "model_id: Optional[str] = None", "some_exc: Optional[HTTPException] = None"); add "from typing import Optional" to the imports if missing; update any other signatures in the same module that use "Type | None" (per the review note also the occurrences around the HTTPException usage) to follow this Optional[...] convention and run type checks to ensure no further edits are needed.
132-139: Align new helper docstrings with the repository’s Google-style format.The new helper docstrings are brief, but they omit required sections (e.g.,
Args/Returns/Raises) expected by the project style.📝 Suggested docstring shape
def _get_base_prompt() -> str: - """Get the base system prompt with configuration fallback.""" + """Get the base system prompt. + + Returns: + The configured system prompt when available; otherwise the default + prompt constant. + """As per coding guidelines: "Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with Parameters, Returns, Raises, and Attributes sections."
Also applies to: 225-229, 300-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 132 - 139, The _get_base_prompt helper uses a brief docstring that doesn't follow the project's Google-style conventions; update its docstring to include a short summary plus explicit Args (if any), Returns (str), and Raises (if applicable) sections per the Google Python docstring guide, and apply the same structured format to the other new helper functions in this module so all helper docstrings include Parameters/Returns/Raises as required by project style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 203-216: The code calls extract_token_usage(response.usage,
model_id) with the optional model_id which can be None; update the call to pass
the resolved non-optional resolved_model_id instead so usage extraction always
gets the actual model string. Locate the block where resolved_model_id is set
and the client.responses.create call (symbols: resolved_model_id, model_id,
extract_token_usage) and replace the second argument to extract_token_usage with
resolved_model_id.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 181-182: Change all occurrences of union-style optional
annotations like "model_id: str | None = None" and "some_exc: HTTPException |
None = None" to use typing.Optional (e.g., "model_id: Optional[str] = None",
"some_exc: Optional[HTTPException] = None"); add "from typing import Optional"
to the imports if missing; update any other signatures in the same module that
use "Type | None" (per the review note also the occurrences around the
HTTPException usage) to follow this Optional[...] convention and run type checks
to ensure no further edits are needed.
- Around line 132-139: The _get_base_prompt helper uses a brief docstring that
doesn't follow the project's Google-style conventions; update its docstring to
include a short summary plus explicit Args (if any), Returns (str), and Raises
(if applicable) sections per the Google Python docstring guide, and apply the
same structured format to the other new helper functions in this module so all
helper docstrings include Parameters/Returns/Raises as required by project
style.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/endpoints/rlsapi_v1.py (2)
53-61: Add a module-level type alias for handled exception tuple.This constant is clear, but it should use an explicit type alias for readability and consistency with the project standard.
As per coding guidelines: `Define type aliases at module level for clarity`.♻️ Proposed refactor
+InferenceHandledExceptionTypes = tuple[type[Exception], ...] + -_INFER_HANDLED_EXCEPTIONS = ( +_INFER_HANDLED_EXCEPTIONS: InferenceHandledExceptionTypes = ( RuntimeError, APIConnectionError, RateLimitError, APIStatusError, OpenAIAPIStatusError, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 53 - 61, Create a module-level type alias (e.g., HandledExceptions) for the exception-tuple type and annotate _INFER_HANDLED_EXCEPTIONS with it; add the necessary typing imports (Type and Tuple) and declare HandledExceptions = Tuple[Type[BaseException], ...] (or similar) at top-level, then change the _INFER_HANDLED_EXCEPTIONS declaration to use that alias to improve readability and follow project convention.
132-139: Upgrade new helper docstrings to full Google style sections.These new functions have docstrings, but they don’t follow the required Google format (
Parameters/Returns/Raiseswhere applicable).As per coding guidelines:
All functions require docstrings with brief descriptionsandFollow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with Parameters, Returns, Raises, and Attributes sections.Also applies to: 225-229, 300-308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/endpoints/rlsapi_v1.py` around lines 132 - 139, The _get_base_prompt function's docstring must be converted to Google-style: keep the one-line summary, add a "Returns" section specifying it returns a str (the selected system prompt), add a "Raises" section only if the function can raise exceptions (otherwise explicitly state "None"), and include a "Parameters" section indicating there are no parameters (or omit if style allows but prefer explicit "None"); apply the same Google-style docstring changes to the other new helper functions in this file referenced by the review so each has a brief description plus Parameters/Returns/Raises sections and any relevant type information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 177-182: The function signature for retrieve_simple_response (and
other places in this file where the union-with-None syntax is used, e.g., the
parameter at the later occurrence around line 302) should use typing.Optional
instead of the `| None` syntax to follow project conventions; update signatures
such as `tools: list[Any] | None` to `tools: Optional[list[Any]]` and `model_id:
str | None` to `model_id: Optional[str]`, import Optional from typing at the top
if not already present, and apply the same replacement for any other parameters
in this file that use `| None`.
---
Nitpick comments:
In `@src/app/endpoints/rlsapi_v1.py`:
- Around line 53-61: Create a module-level type alias (e.g., HandledExceptions)
for the exception-tuple type and annotate _INFER_HANDLED_EXCEPTIONS with it; add
the necessary typing imports (Type and Tuple) and declare HandledExceptions =
Tuple[Type[BaseException], ...] (or similar) at top-level, then change the
_INFER_HANDLED_EXCEPTIONS declaration to use that alias to improve readability
and follow project convention.
- Around line 132-139: The _get_base_prompt function's docstring must be
converted to Google-style: keep the one-line summary, add a "Returns" section
specifying it returns a str (the selected system prompt), add a "Raises" section
only if the function can raise exceptions (otherwise explicitly state "None"),
and include a "Parameters" section indicating there are no parameters (or omit
if style allows but prefer explicit "None"); apply the same Google-style
docstring changes to the other new helper functions in this file referenced by
the review so each has a brief description plus Parameters/Returns/Raises
sections and any relevant type information.
Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
major/rlsapi-v1-simplify-types
branch
from
9241e25 to
50cc05d
Compare
Merged
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
src/app/endpoints/rlsapi_v1.pyso/inferis easier to follow._map_inference_error_to_http_exception) instead of repeating nearly identicalexceptblocks.context_length, connection issues, rate limits, API status errors).RuntimeErrors still get re-raised (same behavior), just through a cleaner flow.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run pytest tests/unit/app/endpoints/test_rlsapi_v1.pyuv run pyright src/app/endpoints/rlsapi_v1.pyuv run make verifySummary by CodeRabbit
New Features
Bug Fixes
Telemetry
Public API