-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Parse API response errors when max_completions_tokens used and provide helpful messages to the user #482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
max_completions_tokens used and provide helpful messages to the user56e4f5e to
dca9e1d
Compare
WalkthroughThis pull request introduces API error detection and console reporting capabilities for AIPerf. It adds a new enum member to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
|
@CodeRabbit start review |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@lkomali: I'll start reviewing the changes in this PR now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/exporters/test_console_api_error_exporter.py (1)
75-122: Consider moving asyncio imports to the top.The tests provide excellent coverage of the exporter's panel rendering and skip behavior. The inline
import asynciostatements (lines 88, 118) could be moved to the top of the file for consistency with standard Python practices.Apply this diff to move the import to the top:
import json from unittest.mock import MagicMock +import asyncio import pytest from rich.console import ConsoleThen remove the inline imports:
- import asyncio - asyncio.run(exporter.export(mock_console))src/aiperf/exporters/console_api_error_exporter.py (2)
19-35: Consider using @DataClass for ErrorInsight.The current implementation is functional, but using
@dataclasswould provide automatic__repr__,__eq__, and reduce boilerplate.Apply this diff to refactor as a dataclass:
+from dataclasses import dataclass + +@dataclass class ErrorInsight: """Model to describe a detected API error insight.""" - - def __init__( - self, - title: str, - problem: str, - causes: list[str], - investigation: list[str], - fixes: list[str], - ): - self.title = title - self.problem = problem - self.causes = causes - self.investigation = investigation - self.fixes = fixes + + title: str + problem: str + causes: list[str] + investigation: list[str] + fixes: list[str]
49-58: Consider more specific exception handling.Lines 51-52 use
contextlib.suppress(Exception)which silently catches all exceptions during JSON parsing. While this prevents crashes, it could hide legitimate errors like encoding issues or malformed JSON that might indicate problems upstream.Consider catching specific exceptions:
- parsed = None - with contextlib.suppress(Exception): - parsed = json.loads(raw_msg) + parsed = None + try: + parsed = json.loads(raw_msg) + except (json.JSONDecodeError, TypeError): + # Expected if raw_msg isn't valid JSON + passAdditionally, line 58's
str()conversion is redundant sinceraw_msgis already a string from line 49 (err.message or "").
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/aiperf/common/enums/data_exporter_enums.py(1 hunks)src/aiperf/exporters/__init__.py(2 hunks)src/aiperf/exporters/console_api_error_exporter.py(1 hunks)tests/unit/exporters/test_console_api_error_exporter.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/exporters/test_console_api_error_exporter.py (2)
src/aiperf/exporters/console_api_error_exporter.py (4)
ConsoleApiErrorExporter(91-138)MaxCompletionTokensDetector(37-86)detect(39-86)export(102-118)tests/unit/exporters/test_metrics_base_exporter.py (1)
exporter_config(67-76)
src/aiperf/exporters/__init__.py (1)
src/aiperf/exporters/console_api_error_exporter.py (3)
ConsoleApiErrorExporter(91-138)ErrorInsight(19-34)MaxCompletionTokensDetector(37-86)
src/aiperf/exporters/console_api_error_exporter.py (4)
src/aiperf/common/enums/data_exporter_enums.py (1)
ConsoleExporterType(7-14)src/aiperf/common/factories.py (1)
ConsoleExporterFactory(402-418)src/aiperf/common/protocols.py (1)
ConsoleExporterProtocol(312-320)tests/unit/exporters/test_metrics_base_exporter.py (1)
exporter_config(67-76)
🪛 Ruff (0.14.5)
src/aiperf/exporters/console_api_error_exporter.py
94-96: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (11)
- GitHub Check: integration-tests (macos-latest, 3.11)
- GitHub Check: integration-tests (macos-latest, 3.12)
- GitHub Check: integration-tests (macos-latest, 3.13)
- GitHub Check: integration-tests (ubuntu-latest, 3.12)
- GitHub Check: integration-tests (ubuntu-latest, 3.11)
- GitHub Check: integration-tests (macos-latest, 3.10)
- GitHub Check: integration-tests (ubuntu-latest, 3.13)
- GitHub Check: integration-tests (ubuntu-latest, 3.10)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.13)
🔇 Additional comments (7)
src/aiperf/common/enums/data_exporter_enums.py (1)
8-8: LGTM!The new
API_ERRORSenum member is well-positioned and follows the existing naming conventions.src/aiperf/exporters/__init__.py (1)
11-15: LGTM!The new exports are correctly structured. Since this file is auto-generated by mkinit, the changes properly reflect the new module additions.
tests/unit/exporters/test_console_api_error_exporter.py (3)
16-44: LGTM!The mock classes and fixtures provide clear test infrastructure that matches the expected error structure from the API.
47-58: LGTM!Good coverage of the detection logic, verifying both the insight creation and key content presence.
61-72: LGTM!Excellent coverage of edge cases where detection should not trigger.
src/aiperf/exporters/console_api_error_exporter.py (2)
102-118: LGTM!The export logic is well-structured: it iterates through detectors, renders insights as Rich panels when detected, and flushes output appropriately.
120-138: LGTM!The formatting logic produces clear, well-structured diagnostic output with proper Rich markup for emphasis and readability.
ajcasagrande
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! A few nits, but it looks great, thanks!
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@1bcb3dd985cd5bb5472eedd85ef19d6d8edfd8c6Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@1bcb3dd985cd5bb5472eedd85ef19d6d8edfd8c6Last updated for commit: |
dca9e1d to
3a15e03
Compare
Signed-off-by: lkomali <[email protected]>
Signed-off-by: lkomali <[email protected]>
Signed-off-by: lkomali <[email protected]>
3a15e03 to
1bcb3dd
Compare
This PR improves UX in case of API response errors when

max_completions_tokensused.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.