add support for async methods#15
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces asynchronous support across the Basalt SDK, adding async methods to the DatasetSDK, MonitorSDK, and PromptSDK classes and updating the underlying API and network layers to support async operations using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK
participant Api
participant Networker
User->>SDK: Call async_method(...)
SDK->>Api: await async_invoke(endpoint, dto)
Api->>Networker: await async_fetch(url, method, body, headers, params)
Networker-->>Api: (error, response)
Api-->>SDK: (error, decoded_response)
SDK-->>User: (error, result)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
0xthc
force-pushed
the
implement-async-methods
branch
3 times, most recently
from
cedd078 to
21965ab
Compare
0xthc
force-pushed
the
implement-async-methods
branch
from
1a9676f to
b3658fc
Compare
0xthc
force-pushed
the
implement-async-methods
branch
from
39172fd to
41f46e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
basalt/sdk/monitorsdk.py (2)
63-105: Refactor to reduce code duplication.The
async_create_tracemethod is nearly identical tocreate_traceexcept for theawaitkeyword. This pattern repeats throughout the file, creating significant code duplication.Consider extracting common logic into a shared private method:
+ def _prepare_trace_params(self, params: Optional[TraceParams]) -> TraceParams: + """Common logic for preparing trace parameters.""" + if params is None: + params = {} + return TraceParams(**params) + + def _build_trace(self, slug: str, params: TraceParams) -> Trace: + """Common logic for building a trace object.""" + flusher = Flusher(self._api, self._logger) + params_dict = { + "input": params.input, + "output": params.output, + "name": params.name, + "start_time": params.start_time, + "end_time": params.end_time, + "user": params.user, + "organization": params.organization, + "metadata": params.metadata, + "experiment": params.experiment, + "evaluators": params.evaluators, + "evaluationConfig": params.evaluation_config + } + return Trace(slug, params_dict, flusher, self._logger) def create_trace(self, slug: str, params: Optional[TraceParams] = None) -> Trace: - if params is None: - params = {} - trace_params = TraceParams(**params) - return self._create_trace(slug, trace_params) + trace_params = self._prepare_trace_params(params) + return self._build_trace(slug, trace_params) async def async_create_trace(self, slug: str, params: Optional[TraceParams] = None) -> Trace: - if params is None: - params = {} - trace_params = TraceParams(**params) - return await self._async_create_trace(slug, trace_params) + trace_params = self._prepare_trace_params(params) + return self._build_trace(slug, trace_params)
28-60: Fix return type inconsistency.The
create_experimentmethod returnsTuple[Optional[Exception], Optional[Experiment]]but the docstring mentions it returns justExperiment. The async version should maintain the same return type pattern.def create_experiment( self, feature_slug: str, params: ExperimentParams ) -> Tuple[Optional[Exception], Optional[Experiment]]: """ Creates a new experiment for monitoring. Args: feature_slug (str): The feature slug for the experiment. - params (Dict[str, Any]): Parameters for the experiment. + params (ExperimentParams): Parameters for the experiment. Returns: - Experiment: A new Experiment instance. + Tuple[Optional[Exception], Optional[Experiment]]: A tuple containing an optional exception and an optional Experiment instance. """
🧹 Nitpick comments (11)
basalt/basaltsdk.py (1)
8-8: Question the necessity of this whitespace addition.This appears to be an unnecessary whitespace addition to the docstring. Consider removing it unless it serves a specific formatting purpose.
basalt/utils/networker.py (1)
93-100: Optimize nested context managers.The nested
withstatements can be combined into a single statement for better readability.- async with aiohttp.ClientSession() as session: - async with session.request( - method, - url, - params=params, - json=body, - headers=headers - ) as response: + async with aiohttp.ClientSession() as session, \ + session.request(method, url, params=params, json=body, headers=headers) as response:tests/test_datasetsdk_async.py (1)
215-215: Remove unused variable.The
runnervariable is assigned but never used.- runner = unittest.TextTestRunner()tests/test_monitorsdk_async.py (1)
226-226: Remove unused variable assignment.The
runnervariable is assigned but never used.- runner = unittest.TextTestRunner()tests/test_promptsdk_async.py (3)
3-3: Remove unused import.The
patchimport is not used in this test file.-from unittest.mock import MagicMock, AsyncMock, patch +from unittest.mock import MagicMock, AsyncMock
7-7: Remove unused import.The
GetPromptDTOimport is not used in this test file.-from basalt.utils.dtos import PromptDTO, PromptVersionDTO, PromptVariablesDTO, GetPromptDTO +from basalt.utils.dtos import PromptDTO, PromptVersionDTO, PromptVariablesDTO
198-198: Remove unused variable assignment.The
runnervariable is assigned but never used.- runner = unittest.TextTestRunner()basalt/sdk/monitorsdk.py (1)
2-2: Remove unused import.The
asyncioimport is not used in this file.-import asynciobasalt/sdk/promptsdk.py (3)
1-1: Remove unused import.The
Awaitableimport is not used in this file.-from typing import Optional, Dict, Tuple, Any, Awaitable +from typing import Optional, Dict, Tuple, Any
14-14: Remove unused import.The
asyncioimport is not used in this file.-import asyncio
197-243: Consider reducing code duplication with sync version.The
_async_prepare_monitoringmethod is identical to_prepare_monitoringexcept for the function signature. Since neither method actually performs async operations (they just create objects), you could reuse the synchronous version.- async def _async_prepare_monitoring( - self, - prompt: PromptResponse, - slug: str, - version: Optional[str] = None, - tag: Optional[str] = None, - variables: Optional[Dict[str, Any]] = None, - original_prompt_text: Optional[str] = None - ) -> Generation: - """ - Asynchronously prepare monitoring by creating a trace and generation object. - - Args: - prompt (PromptResponse): The prompt response. - slug (str): The slug identifier for the prompt. - version (Optional[str]): The version of the prompt. - tag (Optional[str]): The tag associated with the prompt. - variables (Optional[Dict[str, Any]]): Variables used in the prompt. - original_prompt_text (Optional[str]): The original prompt text. - - Returns: - Generation: The generation object. - """ - # Create a flusher - flusher = Flusher(self._api, self._logger) - - # Create a trace - trace = Trace(slug, { - "input": original_prompt_text or prompt.text, - "start_time": datetime.now() - }, flusher, self._logger) - - # Create a generation - generation = Generation({ - "name": slug, - "trace": trace, - "prompt": { - "slug": slug, - "version": version, - "tag": tag - }, - "input": original_prompt_text or prompt.text, - "variables": variables, - "options": {"type": "single"} - }) - - return generation + async def _async_prepare_monitoring( + self, + prompt: PromptResponse, + slug: str, + version: Optional[str] = None, + tag: Optional[str] = None, + variables: Optional[Dict[str, Any]] = None, + original_prompt_text: Optional[str] = None + ) -> Generation: + """ + Asynchronously prepare monitoring by creating a trace and generation object. + """ + return self._prepare_monitoring(prompt, slug, version, tag, variables, original_prompt_text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
basalt/basaltsdk.py(1 hunks)basalt/objects/dataset.py(1 hunks)basalt/sdk/datasetsdk.py(4 hunks)basalt/sdk/monitorsdk.py(9 hunks)basalt/sdk/promptsdk.py(6 hunks)basalt/utils/api.py(2 hunks)basalt/utils/networker.py(2 hunks)examples/dataset_sdk_async_demo.ipynb(1 hunks)examples/dataset_sdk_demo.ipynb(1 hunks)examples/monitor_sdk_async_demo.ipynb(1 hunks)examples/prompt_sdk_async_demo.ipynb(1 hunks)setup.py(1 hunks)tests/test_datasetsdk_async.py(1 hunks)tests/test_monitorsdk_async.py(1 hunks)tests/test_promptsdk_async.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
basalt/sdk/datasetsdk.py (9)
basalt/sdk/promptsdk.py (1)
async_list(343-367)basalt/utils/dtos.py (4)
ListDatasetsDTO(173-175)DatasetDTO(137-151)GetDatasetDTO(179-181)CreateDatasetItemDTO(185-191)basalt/utils/api.py (1)
async_invoke(88-121)basalt/endpoints/list_datasets.py (1)
ListDatasetsEndpoint(32-64)basalt/basaltsdk.py (1)
datasets(27-29)basalt/basalt_facade.py (1)
datasets(63-67)basalt/utils/protocols.py (2)
datasets(46-46)error(51-51)basalt/endpoints/get_dataset.py (1)
GetDatasetEndpoint(37-72)basalt/endpoints/create_dataset_item.py (1)
CreateDatasetItemEndpoint(39-87)
basalt/sdk/promptsdk.py (8)
basalt/objects/generation.py (3)
variables(69-71)Generation(6-151)prompt(39-41)basalt/utils/dtos.py (6)
PromptResponse(52-65)GetPromptDTO(68-71)DescribePromptDTO(99-102)DescribePromptResponse(77-96)PromptListDTO(128-129)PromptListResponse(108-125)basalt/utils/api.py (1)
async_invoke(88-121)basalt/endpoints/get_prompt.py (1)
GetPromptEndpoint(27-65)basalt/utils/flusher.py (1)
Flusher(11-114)basalt/objects/trace.py (3)
Trace(13-344)name(50-52)feature_slug(95-97)basalt/endpoints/describe_prompt.py (1)
DescribePromptEndpoint(27-65)basalt/endpoints/list_prompts.py (1)
ListPromptsEndpoint(27-61)
🪛 Flake8 (7.2.0)
basalt/objects/dataset.py
[error] 24-24: indentation contains mixed spaces and tabs
(E101)
[error] 24-24: continuation line unaligned for hanging indent
(E131)
[error] 25-25: indentation contains mixed spaces and tabs
(E101)
setup.py
[error] 30-30: indentation contains mixed spaces and tabs
(E101)
basalt/utils/api.py
[error] 4-4: 'asyncio' imported but unused
(F401)
tests/test_datasetsdk_async.py
[error] 215-215: local variable 'runner' is assigned to but never used
(F841)
tests/test_monitorsdk_async.py
[error] 226-226: local variable 'runner' is assigned to but never used
(F841)
tests/test_promptsdk_async.py
[error] 3-3: 'unittest.mock.patch' imported but unused
(F401)
[error] 7-7: 'basalt.utils.dtos.GetPromptDTO' imported but unused
(F401)
[error] 198-198: local variable 'runner' is assigned to but never used
(F841)
basalt/sdk/datasetsdk.py
[error] 5-5: 'asyncio' imported but unused
(F401)
basalt/sdk/promptsdk.py
[error] 1-1: 'typing.Awaitable' imported but unused
(F401)
[error] 14-14: 'asyncio' imported but unused
(F401)
basalt/utils/networker.py
[error] 73-73: unexpected spaces around keyword / parameter equals
(E251)
[error] 73-73: unexpected spaces around keyword / parameter equals
(E251)
[error] 74-74: unexpected spaces around keyword / parameter equals
(E251)
[error] 74-74: unexpected spaces around keyword / parameter equals
(E251)
[error] 75-75: unexpected spaces around keyword / parameter equals
(E251)
[error] 75-75: unexpected spaces around keyword / parameter equals
(E251)
[error] 76-76: continuation line with same indent as next logical line
(E125)
basalt/sdk/monitorsdk.py
[error] 2-2: 'asyncio' imported but unused
(F401)
🪛 Ruff (0.11.9)
basalt/utils/api.py
4-4: asyncio imported but unused
Remove unused import: asyncio
(F401)
tests/test_datasetsdk_async.py
215-215: Local variable runner is assigned to but never used
Remove assignment to unused variable runner
(F841)
tests/test_monitorsdk_async.py
226-226: Local variable runner is assigned to but never used
Remove assignment to unused variable runner
(F841)
tests/test_promptsdk_async.py
3-3: unittest.mock.patch imported but unused
Remove unused import: unittest.mock.patch
(F401)
7-7: basalt.utils.dtos.GetPromptDTO imported but unused
Remove unused import: basalt.utils.dtos.GetPromptDTO
(F401)
198-198: Local variable runner is assigned to but never used
Remove assignment to unused variable runner
(F841)
basalt/sdk/datasetsdk.py
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
basalt/sdk/promptsdk.py
1-1: typing.Awaitable imported but unused
Remove unused import: typing.Awaitable
(F401)
14-14: asyncio imported but unused
Remove unused import: asyncio
(F401)
97-97: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
basalt/utils/networker.py
93-100: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
basalt/sdk/monitorsdk.py
2-2: asyncio imported but unused
Remove unused import: asyncio
(F401)
🪛 Pylint (3.3.7)
basalt/sdk/datasetsdk.py
[refactor] 155-155: Too many arguments (6/5)
(R0913)
[refactor] 155-155: Too many positional arguments (6/5)
(R0917)
basalt/sdk/promptsdk.py
[refactor] 92-92: Too many arguments (6/5)
(R0913)
[refactor] 92-92: Too many positional arguments (6/5)
(R0917)
[refactor] 197-197: Too many arguments (7/5)
(R0913)
[refactor] 197-197: Too many positional arguments (7/5)
(R0917)
basalt/utils/networker.py
[refactor] 69-69: Too many arguments (6/5)
(R0913)
[refactor] 69-69: Too many positional arguments (6/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (3.12)
🔇 Additional comments (18)
basalt/objects/dataset.py (1)
24-25: Fix indentation in DatasetRow.to_dict() and verify None handlingPlease address two concerns in
basalt/objects/dataset.py:
Indentation
The mixed tabs/spaces before the"name"and"idealOutput"lines will fail linting. Convert to spaces to match the surrounding block:- "name": self.name, - "idealOutput": self.ideal_output + "name": self.name, + "idealOutput": self.ideal_outputLogic change
Previously these keys were only included when non-None. Now they’re always present, which could sendnullin API payloads. Please verify whether the downstream API acceptsnullfornameandidealOutputor if the conditional inclusion should be retained.Can you confirm via API docs/tests that
nullvalues are supported, or update the code to keep the original conditional logic?examples/dataset_sdk_demo.ipynb (1)
1-232: Comprehensive synchronous examples with good practices.The notebook provides excellent examples of Dataset SDK usage with proper error handling, including safely handling missing datasets and incomplete values. The examples complement the new async functionality by serving as the synchronous reference implementation.
basalt/utils/api.py (1)
88-121: Well-implemented async method that mirrors synchronous counterpart.The
async_invokemethod correctly implements the asynchronous pattern, properly usingawaitfor network calls and maintaining the same interface and error handling as the synchronousinvokemethod. The implementation is consistent and follows good async/await practices.basalt/utils/networker.py (2)
2-2: LGTM: Adding aiohttp import for async support.The import addition aligns with the PR objective to add async HTTP capabilities.
92-120: Excellent async implementation with proper error handling.The async_fetch method correctly mirrors the synchronous version while using aiohttp appropriately. The status code handling correctly uses
response.statusinstead ofresponse.status_codefor aiohttp, and error handling maintains consistency with the sync implementation.basalt/sdk/datasetsdk.py (3)
51-69: Excellent async implementation of list method.The async_list method properly mirrors the synchronous version while using
awaitwithasync_invoke. The error handling and return type are consistent.
93-113: Excellent async implementation of get method.The async_get method correctly implements the asynchronous pattern with proper error handling for both API errors and result errors.
155-193: Excellent async implementation of addRow method.The async_addRow method maintains consistency with the synchronous version while properly implementing async patterns. The parameter signature and error handling are correct.
tests/test_datasetsdk_async.py (1)
1-224: Excellent comprehensive async test coverage.The test suite provides thorough coverage of all async DatasetSDK methods with proper use of AsyncMock and async test patterns. The test structure and assertions are well-designed.
examples/monitor_sdk_async_demo.ipynb (1)
1-268: Excellent async demo notebook with comprehensive examples.This notebook provides outstanding demonstration of the MonitorSDK async capabilities with:
- Clear progression from basic to advanced usage patterns
- Proper async/await implementation throughout
- Effective demonstration of concurrent operations using
asyncio.gather- Well-structured examples with appropriate error handling
- Educational value for users learning async patterns
The concurrent execution example particularly showcases the performance benefits of async operations.
examples/prompt_sdk_async_demo.ipynb (1)
1-242: Excellent async demo notebook with comprehensive PromptSDK coverage.This notebook effectively demonstrates the PromptSDK async capabilities with:
- Comprehensive coverage of async methods (list, get, describe)
- Practical examples including variable substitution
- Clear demonstration of concurrent operations using
asyncio.gather- Well-structured progression from basic to advanced usage
- Proper error handling and result validation throughout
The variable substitution example and concurrent operations showcase real-world usage patterns effectively.
tests/test_monitorsdk_async.py (1)
75-215: LGTM! Comprehensive async test coverage.The test methods provide excellent coverage for all async MonitorSDK methods with proper mocking, assertions, and error handling verification.
examples/dataset_sdk_async_demo.ipynb (1)
1-288: Excellent async SDK demonstration with best practices.This notebook provides comprehensive examples of async DatasetSDK usage, progressing from basic operations to advanced concurrent execution patterns. The error handling, clear documentation, and practical examples make this a valuable resource for users learning async SDK operations.
tests/test_promptsdk_async.py (1)
74-187: LGTM! Comprehensive async test coverage.The test methods provide excellent coverage for all async PromptSDK methods with proper variable substitution testing, error handling, and endpoint verification.
basalt/sdk/monitorsdk.py (1)
199-225: LGTM! Async experiment creation implementation.The async implementation correctly mirrors the synchronous version and properly handles error cases and object creation.
basalt/sdk/promptsdk.py (3)
92-147: LGTM! Well-implemented async get method.The async implementation correctly handles caching, API calls, and fallback scenarios while maintaining the same logic flow as the synchronous version.
286-324: LGTM! Clean async describe implementation.The async describe method correctly mirrors the synchronous version with proper error handling and response construction.
343-367: LGTM! Efficient async list implementation.The async list method properly handles optional filtering and constructs the response list correctly.
| packages=find_packages(), | ||
| install_requires=[ | ||
| "requests>=2.32", | ||
| "aiohttp>=3.8.0", |
There was a problem hiding this comment.
Fix indentation and approve the dependency addition.
The aiohttp dependency is correctly added to support async functionality, but there's a mixed spaces/tabs indentation issue.
Fix the indentation:
- "aiohttp>=3.8.0",
+ "aiohttp>=3.8.0",📝 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.
| "aiohttp>=3.8.0", | |
| "aiohttp>=3.8.0", |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 30-30: indentation contains mixed spaces and tabs
(E101)
🤖 Prompt for AI Agents
In setup.py at line 30, the added dependency "aiohttp>=3.8.0" has inconsistent
indentation using mixed spaces and tabs. Fix this by ensuring the line uses
consistent indentation matching the surrounding lines, preferably spaces only,
to maintain code style consistency.
| from typing import Dict, TypeVar, Optional, Tuple | ||
|
|
||
| from .protocols import IEndpoint, INetworker, ILogger | ||
| import asyncio |
There was a problem hiding this comment.
Remove unused asyncio import.
The asyncio import is not used in this file and should be removed.
-import asyncio📝 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.
| import asyncio |
🧰 Tools
🪛 Ruff (0.11.9)
4-4: asyncio imported but unused
Remove unused import: asyncio
(F401)
🪛 Flake8 (7.2.0)
[error] 4-4: 'asyncio' imported but unused
(F401)
🤖 Prompt for AI Agents
In basalt/utils/api.py at line 4, the asyncio import is unused and should be
removed to clean up the code and avoid unnecessary imports.
| async def async_fetch( | ||
| self, | ||
| url: str, | ||
| method: str, | ||
| body = None, | ||
| headers = None, | ||
| params = None | ||
| ) -> Tuple[Optional[FetchError], Optional[Dict[str, Any]]]: |
There was a problem hiding this comment.
Fix parameter formatting issues.
The parameter definitions have incorrect spacing around the default value assignments.
- async def async_fetch(
- self,
- url: str,
- method: str,
- body = None,
- headers = None,
- params = None
- ) -> Tuple[Optional[FetchError], Optional[Dict[str, Any]]]:
+ async def async_fetch(
+ self,
+ url: str,
+ method: str,
+ body=None,
+ headers=None,
+ params=None
+ ) -> Tuple[Optional[FetchError], Optional[Dict[str, Any]]]:📝 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.
| async def async_fetch( | |
| self, | |
| url: str, | |
| method: str, | |
| body = None, | |
| headers = None, | |
| params = None | |
| ) -> Tuple[Optional[FetchError], Optional[Dict[str, Any]]]: | |
| async def async_fetch( | |
| self, | |
| url: str, | |
| method: str, | |
| body=None, | |
| headers=None, | |
| params=None | |
| ) -> Tuple[Optional[FetchError], Optional[Dict[str, Any]]]: |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 73-73: unexpected spaces around keyword / parameter equals
(E251)
[error] 73-73: unexpected spaces around keyword / parameter equals
(E251)
[error] 74-74: unexpected spaces around keyword / parameter equals
(E251)
[error] 74-74: unexpected spaces around keyword / parameter equals
(E251)
[error] 75-75: unexpected spaces around keyword / parameter equals
(E251)
[error] 75-75: unexpected spaces around keyword / parameter equals
(E251)
[error] 76-76: continuation line with same indent as next logical line
(E125)
🪛 Pylint (3.3.7)
[refactor] 69-69: Too many arguments (6/5)
(R0913)
[refactor] 69-69: Too many positional arguments (6/5)
(R0917)
🤖 Prompt for AI Agents
In basalt/utils/networker.py around lines 69 to 76, the parameter definitions in
the async_fetch method have inconsistent spacing around the equal signs for
default values. Fix this by removing extra spaces so that default assignments
use no spaces before or after the equals sign, e.g., change "body = None" to
"body=None" for all parameters with default values.
| SDK for interacting with Basalt datasets | ||
| """ | ||
| from typing import Dict, List, Optional, Tuple, Any | ||
| import asyncio |
There was a problem hiding this comment.
Remove unused asyncio import.
The asyncio import is not used in this file and should be removed.
-import asyncio📝 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.
| import asyncio |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 5-5: 'asyncio' imported but unused
(F401)
🪛 Ruff (0.11.9)
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
🤖 Prompt for AI Agents
In basalt/sdk/datasetsdk.py at line 5, the asyncio module is imported but not
used anywhere in the file. Remove the line importing asyncio to clean up unused
imports and improve code clarity.
| def run_async_tests(): | ||
| """ | ||
| Helper function to run async tests | ||
| """ | ||
| loop = asyncio.get_event_loop() | ||
|
|
||
| # Create and run the test suite | ||
| suite = unittest.TestLoader().loadTestsFromTestCase(TestMonitorSDKAsync) | ||
| runner = unittest.TextTestRunner() | ||
|
|
||
| for test in suite: | ||
| if test._testMethodName.startswith('test_async_'): | ||
| coro = getattr(test, test._testMethodName)() | ||
| loop.run_until_complete(coro) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the async test runner implementation.
The current approach manually executes test methods, bypassing unittest's proper test execution model. This can miss setUp/tearDown methods and doesn't provide proper test isolation.
Consider using unittest.IsolatedAsyncioTestCase for cleaner async test execution:
-def run_async_tests():
- """
- Helper function to run async tests
- """
- loop = asyncio.get_event_loop()
-
- # Create and run the test suite
- suite = unittest.TestLoader().loadTestsFromTestCase(TestMonitorSDKAsync)
- runner = unittest.TextTestRunner()
-
- for test in suite:
- if test._testMethodName.startswith('test_async_'):
- coro = getattr(test, test._testMethodName)()
- loop.run_until_complete(coro)
+class TestMonitorSDKAsync(unittest.IsolatedAsyncioTestCase):
+ # ... existing test methods ...
+
+if __name__ == "__main__":
+ unittest.main()Or use asyncio.run() with proper test discovery:
-def run_async_tests():
- """
- Helper function to run async tests
- """
- loop = asyncio.get_event_loop()
-
- # Create and run the test suite
- suite = unittest.TestLoader().loadTestsFromTestCase(TestMonitorSDKAsync)
- runner = unittest.TextTestRunner()
-
- for test in suite:
- if test._testMethodName.startswith('test_async_'):
- coro = getattr(test, test._testMethodName)()
- loop.run_until_complete(coro)
+if __name__ == "__main__":
+ unittest.main()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
226-226: Local variable runner is assigned to but never used
Remove assignment to unused variable runner
(F841)
🪛 Flake8 (7.2.0)
[error] 226-226: local variable 'runner' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In tests/test_monitorsdk_async.py around lines 218 to 231, the current async
test runner manually executes async test methods, bypassing unittest's standard
execution flow and missing setUp/tearDown calls and proper isolation. Refactor
the tests to inherit from unittest.IsolatedAsyncioTestCase, which natively
supports async test methods with proper setup and teardown. Alternatively,
modify the runner to use asyncio.run() for each async test method while ensuring
full unittest test discovery and execution flow is preserved.
| def run_async_tests(): | ||
| """ | ||
| Helper function to run async tests | ||
| """ | ||
| loop = asyncio.get_event_loop() | ||
|
|
||
| # Create and run the test suite | ||
| suite = unittest.TestLoader().loadTestsFromTestCase(TestPromptSDKAsync) | ||
| runner = unittest.TextTestRunner() | ||
|
|
||
| for test in suite: | ||
| if test._testMethodName.startswith('test_async_'): | ||
| coro = getattr(test, test._testMethodName)() | ||
| loop.run_until_complete(coro) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the async test runner implementation.
Same issue as in the MonitorSDK tests - consider using unittest.IsolatedAsyncioTestCase or proper asyncio test execution instead of manually running test methods.
-def run_async_tests():
- """
- Helper function to run async tests
- """
- loop = asyncio.get_event_loop()
-
- # Create and run the test suite
- suite = unittest.TestLoader().loadTestsFromTestCase(TestPromptSDKAsync)
- runner = unittest.TextTestRunner()
-
- for test in suite:
- if test._testMethodName.startswith('test_async_'):
- coro = getattr(test, test._testMethodName)()
- loop.run_until_complete(coro)
+if __name__ == "__main__":
+ unittest.main()📝 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.
| def run_async_tests(): | |
| """ | |
| Helper function to run async tests | |
| """ | |
| loop = asyncio.get_event_loop() | |
| # Create and run the test suite | |
| suite = unittest.TestLoader().loadTestsFromTestCase(TestPromptSDKAsync) | |
| runner = unittest.TextTestRunner() | |
| for test in suite: | |
| if test._testMethodName.startswith('test_async_'): | |
| coro = getattr(test, test._testMethodName)() | |
| loop.run_until_complete(coro) | |
| if __name__ == "__main__": | |
| unittest.main() |
🧰 Tools
🪛 Ruff (0.11.9)
198-198: Local variable runner is assigned to but never used
Remove assignment to unused variable runner
(F841)
🪛 Flake8 (7.2.0)
[error] 198-198: local variable 'runner' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In tests/test_promptsdk_async.py around lines 190 to 203, the current async test
runner manually runs async test methods using the event loop, which is not
ideal. Refactor the test class to inherit from unittest.IsolatedAsyncioTestCase
and define async test methods directly; this allows unittest to handle the event
loop properly and run async tests natively without manual loop management.
| slug: str, | ||
| version: Optional[str] = None, | ||
| tag: Optional[str] = None, | ||
| variables: Dict[str, str] = {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix mutable default argument.
Using a mutable dictionary as a default argument can lead to unexpected behavior when the same dictionary instance is shared across function calls.
- variables: Dict[str, str] = {},
+ variables: Optional[Dict[str, str]] = None,Then initialize within the function:
+ if variables is None:
+ variables = {}📝 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.
| variables: Dict[str, str] = {}, | |
| def your_function( | |
| ..., | |
| - variables: Dict[str, str] = {}, | |
| + variables: Optional[Dict[str, str]] = None, | |
| ) -> ReturnType: | |
| + if variables is None: | |
| + variables = {} | |
| # rest of function body... |
🧰 Tools
🪛 Ruff (0.11.9)
97-97: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In basalt/sdk/promptsdk.py at line 97, the function uses a mutable dictionary as
a default argument which can cause shared state issues across calls. Change the
default value of the variables parameter from an empty dictionary to None, then
inside the function initialize variables to an empty dictionary if it is None.
| self._fallback_cache.put(dto, result.prompt) | ||
|
|
There was a problem hiding this comment.
Fix cache TTL parameter inconsistency.
The async version is missing the ttl parameter when putting to cache, unlike the synchronous version.
- self._cache.put(dto, result.prompt, self._cache_duration)
+ self._cache.put(dto, result.prompt, ttl=self._cache_duration)📝 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.
| self._cache.put(dto, result.prompt, self._cache_duration) | |
| self._fallback_cache.put(dto, result.prompt) | |
| self._cache.put(dto, result.prompt, ttl=self._cache_duration) | |
| self._fallback_cache.put(dto, result.prompt) |
🤖 Prompt for AI Agents
In basalt/sdk/promptsdk.py around lines 132 to 133, the call to put an item in
the fallback cache is missing the ttl parameter, causing inconsistency with the
synchronous version. Add the ttl parameter (likely self._cache_duration) to the
self._fallback_cache.put call to ensure the cache entry respects the intended
time-to-live duration.
41f46e4 to
8cf2136
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_monitorsdk_async.py (1)
144-183: Use unittest.IsolatedAsyncioTestCase instead of custom async test runner.The custom
AsyncTestRunnerreimplements functionality that's already available in the standard library and was flagged in previous reviews.Replace the custom runner with the standard async test support:
-class AsyncTestRunner: - """Helper class to run async tests properly""" - - def __init__(self): - self.loop = asyncio.new_event_loop() - asyncio.set_event_loop(self.loop) - - def run_test_case(self, test_case_class): - """Run all async test methods in a test case""" - suite = unittest.TestLoader().loadTestsFromTestCase(test_case_class) - - for test in suite: - test_method = getattr(test, test._testMethodName) - if asyncio.iscoroutinefunction(test_method): - try: - test.setUp() - self.loop.run_until_complete(test_method()) - print(f"✓ {test._testMethodName}") - except Exception as e: - print(f"✗ {test._testMethodName}: {e}") - else: - # Run sync tests normally - try: - test.setUp() - test_method() - print(f"✓ {test._testMethodName}") - except Exception as e: - print(f"✗ {test._testMethodName}: {e}") - - def close(self): - self.loop.close() - - -if __name__ == "__main__": - runner = AsyncTestRunner() - try: - runner.run_test_case(TestMonitorSDKAsync) - finally: - runner.close() +if __name__ == "__main__": + unittest.main()And update the test class to inherit from
IsolatedAsyncioTestCase:-class TestMonitorSDKAsync(unittest.TestCase): +class TestMonitorSDKAsync(unittest.IsolatedAsyncioTestCase):
🧹 Nitpick comments (2)
tests/test_monitorsdk_async.py (2)
7-11: Remove unused imports.Static analysis correctly identifies several unused imports that can be removed to clean up the code.
-from basalt.ressources.monitor.experiment_types import ExperimentParams -from basalt.ressources.monitor.trace_types import TraceParams -from basalt.ressources.monitor.generation_types import GenerationParams -from basalt.ressources.monitor.log_types import LogParams -from basalt.objects.experiment import Experiment
30-38: Improve mock experiment object creation.The current approach using
type()constructor and manual attribute assignment is unconventional and harder to maintain.-experiment_output = ExperimentOutput( - experiment=type('Experiment', (), experiment_data)() -) - -# Set experiment attributes properly -experiment_output.experiment.id = experiment_data["id"] -experiment_output.experiment.feature_slug = experiment_data["featureSlug"] -experiment_output.experiment.name = experiment_data["name"] -experiment_output.experiment.created_at = experiment_data["createdAt"] +# Create a proper mock experiment object +mock_experiment = MagicMock() +mock_experiment.id = experiment_data["id"] +mock_experiment.feature_slug = experiment_data["featureSlug"] +mock_experiment.name = experiment_data["name"] +mock_experiment.created_at = experiment_data["createdAt"] + +experiment_output = ExperimentOutput(experiment=mock_experiment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
basalt/objects/dataset.py(1 hunks)basalt/ressources/monitor/generation_types.py(2 hunks)examples/dataset_sdk_demo.ipynb(1 hunks)tests/test_monitorsdk_async.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- basalt/ressources/monitor/generation_types.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/dataset_sdk_demo.ipynb
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_monitorsdk_async.py (8)
basalt/sdk/monitorsdk.py (6)
MonitorSDK(16-402)create_experiment(28-43)async_create_experiment(45-60)async_create_trace(85-105)async_create_generation(123-137)async_create_log(155-169)basalt/utils/logger.py (1)
Logger(3-26)basalt/ressources/monitor/experiment_types.py (1)
ExperimentParams(5-7)basalt/ressources/monitor/trace_types.py (1)
TraceParams(26-38)basalt/ressources/monitor/generation_types.py (2)
GenerationParams(26-61)Generation(64-158)basalt/ressources/monitor/log_types.py (1)
LogParams(11-25)basalt/endpoints/monitor/create_experiment.py (2)
CreateExperimentEndpoint(32-55)Output(29-30)basalt/utils/api.py (1)
async_invoke(88-121)
🪛 Ruff (0.11.9)
tests/test_monitorsdk_async.py
7-7: basalt.ressources.monitor.experiment_types.ExperimentParams imported but unused
Remove unused import: basalt.ressources.monitor.experiment_types.ExperimentParams
(F401)
8-8: basalt.ressources.monitor.trace_types.TraceParams imported but unused
Remove unused import: basalt.ressources.monitor.trace_types.TraceParams
(F401)
9-9: basalt.ressources.monitor.generation_types.GenerationParams imported but unused
Remove unused import: basalt.ressources.monitor.generation_types.GenerationParams
(F401)
10-10: basalt.ressources.monitor.log_types.LogParams imported but unused
Remove unused import: basalt.ressources.monitor.log_types.LogParams
(F401)
11-11: basalt.objects.experiment.Experiment imported but unused
Remove unused import: basalt.objects.experiment.Experiment
(F401)
🪛 Flake8 (7.2.0)
tests/test_monitorsdk_async.py
[error] 7-7: 'basalt.ressources.monitor.experiment_types.ExperimentParams' imported but unused
(F401)
[error] 8-8: 'basalt.ressources.monitor.trace_types.TraceParams' imported but unused
(F401)
[error] 9-9: 'basalt.ressources.monitor.generation_types.GenerationParams' imported but unused
(F401)
[error] 10-10: 'basalt.ressources.monitor.log_types.LogParams' imported but unused
(F401)
[error] 11-11: 'basalt.objects.experiment.Experiment' imported but unused
(F401)
basalt/objects/dataset.py
[error] 24-24: indentation contains mixed spaces and tabs
(E101)
[error] 24-24: continuation line unaligned for hanging indent
(E131)
[error] 25-25: indentation contains mixed spaces and tabs
(E101)
🔇 Additional comments (4)
basalt/objects/dataset.py (1)
24-25: Verify backend handling ofnullforname&idealOutputNo references to these fields’ expected behavior were found in API docs, schemas or tests. Please manually confirm that the backend accepts and correctly processes
nullvalues for both fields:
- Review your OpenAPI/Swagger spec or any API documentation under
docs/orapi/to ensurenameandidealOutputare defined as nullable or optional.- Inspect the serialization/validation logic in the backend (e.g. dataset serializers, controllers or model validators) to verify it allows
null.- Update or add API integration tests (e.g.
tests/api/test_dataset*.py) to cover payloads wherenameandidealOutputarenull.- Ensure downstream client code gracefully handles
nullfor these fields.tests/test_monitorsdk_async.py (3)
50-74: Well-structured async experiment creation test.The test properly verifies the async API call, checks the returned experiment attributes, validates the correct endpoint usage, and ensures the DTO is constructed correctly.
75-126: Comprehensive coverage of direct object creation methods.The tests for async trace, generation, and log creation properly verify that these methods create the expected object types with correct attributes, even though they don't make API calls.
127-142: Good error handling test coverage.The error handling test properly simulates API failures and verifies error propagation, ensuring robust async error handling.
| "name": self.name, | ||
| "idealOutput": self.ideal_output |
There was a problem hiding this comment.
Fix indentation issues with mixed spaces and tabs.
The indentation on these lines contains mixed spaces and tabs, and the continuation line is misaligned. This violates Python coding standards and will cause formatting issues.
Apply this diff to fix the indentation:
- "name": self.name,
- "idealOutput": self.ideal_output
+ "name": self.name,
+ "idealOutput": self.ideal_output📝 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.
| "name": self.name, | |
| "idealOutput": self.ideal_output | |
| "name": self.name, | |
| "idealOutput": self.ideal_output |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 24-24: indentation contains mixed spaces and tabs
(E101)
[error] 24-24: continuation line unaligned for hanging indent
(E131)
[error] 25-25: indentation contains mixed spaces and tabs
(E101)
🤖 Prompt for AI Agents
In basalt/objects/dataset.py around lines 24 to 25, the indentation uses mixed
spaces and tabs causing misalignment and formatting issues. Replace all tabs
with spaces and ensure the continuation line is properly aligned with the
opening brace or previous line according to Python indentation standards.
8cf2136 to
d44a11a
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
aiohttpas a dependency for async HTTP requests.Tests
Documentation