tests: tool runner #327
tests: tool runner #327
Conversation
maciejmajek
changed the base branch from
feat/debugging-tools
to
development
maciejmajek
changed the base branch from
development
to
feat/debugging-tools
|
@CodeRabbit full review |
WalkthroughThe changes in this pull request include modifications to the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
pyproject.toml (1)
90-91: Consider environment-specific logging configuration.While enabling CLI logging with DEBUG level is helpful for local development and debugging, it might generate excessive output in CI/CD environments.
Consider making the logging level configurable via environment variables:
-log_cli = true -log_cli_level = "DEBUG" +log_cli = "true" +log_cli_level = "${LOG_LEVEL:DEBUG}"This way, you can adjust the logging level (e.g., to INFO or WARNING) in CI/CD environments without modifying the configuration file.
tests/core/test_tool_runner.py (1)
26-31: Consider improving test data management and error handlingThe
get_imagetool function has several areas for improvement:
- Move test image to a dedicated test fixtures directory
- Add error handling for missing image file
- Enhance docstring with parameters and return type information
Consider refactoring like this:
@tool(response_format="content_and_artifact") def get_image(): - """Get an image from the camera""" + """Simulate getting an image from a camera for testing purposes. + + Returns: + tuple: A tuple containing: + - str: Status message + - dict: Dictionary with 'images' key containing preprocessed image data + """ + test_image_path = "tests/fixtures/test_image.png" + try: + return "Image retrieved", { + "images": [preprocess_image(test_image_path)] + } + except FileNotFoundError: + raise FileNotFoundError(f"Test image not found at {test_image_path}")src/rai/rai/agents/tool_runner.py (2)
Line range hint
76-94: Standardize error logging levelsWhile the error handling is comprehensive, consider standardizing the logging levels:
- Use ERROR level for unexpected exceptions
- Use WARNING level for validation errors
- Use INFO level for expected operational events
- Use DEBUG level for detailed output
- self.logger.info(error_message) + self.logger.warning(error_message) - self.logger.info(f'Error in "{call["name"]}", error: {e}') + self.logger.error(f'Error in "{call["name"]}", error: {e}')
Line range hint
47-49: Consider implementing a proper task queueing mechanismThe current implementation forces
max_concurrency=1with a TODO comment. This could become a bottleneck as the system scales. Consider:
- Implementing a proper task queue (e.g., using asyncio or a dedicated queue system)
- Adding configurable concurrency limits
- Implementing priority queuing for different types of tools
Would you like help designing a more scalable task queueing solution?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
pyproject.toml(1 hunks)src/rai/rai/agents/tool_runner.py(2 hunks)tests/core/test_tool_runner.py(1 hunks)
🔇 Additional comments (3)
pyproject.toml (1)
90-91: Verify impact on CI pipeline logs.
The DEBUG level logging might significantly increase the size of CI pipeline logs.
✅ Verification successful
Setting DEBUG level logging is acceptable
The CI workflow uses poetry run pytest without any specific logging configuration flags. The DEBUG level logging set in pyproject.toml will only affect the local test output and won't impact CI logs, as GitHub Actions automatically manages and truncates large logs. Additionally, detailed logging during CI can be beneficial for debugging test failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the pytest configuration in CI workflow files
# Look for pytest configuration in CI workflow files
fd -e yml -e yaml . .github/workflows --exec grep -l 'pytest'
# Look for existing log level configurations in CI
fd -e yml -e yaml . .github/workflows --exec grep -i 'log.*level'
Length of output: 163
Script:
#!/bin/bash
# Check the content of the CI workflow file to understand how pytest is configured
cat .github/workflows/poetry-test.yml
Length of output: 1538
src/rai/rai/agents/tool_runner.py (2)
18-18: LGTM: Import placement follows PEP 8
The addition of the time module import is correctly placed in the import section, following the standard library imports.
69-72: LGTM: Proper implementation of performance timing
The implementation uses time.perf_counter() which is the recommended way to measure performance in Python as it provides monotonic timing with the highest available resolution.
maciejmajek
force-pushed
the
feat/debugging-tools
branch
4 times, most recently
from
d1e9a9b to
eb4cb6c
Compare
maciejmajek
force-pushed
the
test/tool-runner
branch
from
bcb9e0c to
980ccd3
Compare
maciejmajek
force-pushed
the
test/tool-runner
branch
from
980ccd3 to
500d338
Compare
Purpose
Tests are good.
Proposed Changes
Issues
Testing
Summary by CodeRabbit
New Features
get_image, for retrieving images from a camera.Bug Fixes
Tests
ToolRunnerclass to validate tool interactions and outputs.