added bedrock llm connection - #23
Conversation
📝 WalkthroughWalkthroughThe PR adds a Bedrock Converse caller with Redis-backed token and error telemetry, and includes ChangesBedrock invocation telemetry
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant call_bedrock
participant BedrockRuntime
participant RedisClient
Caller->>call_bedrock: model_id, messages, inference_config
call_bedrock->>BedrockRuntime: converse()
BedrockRuntime-->>call_bedrock: response and token usage
call_bedrock->>RedisClient: persist future telemetry
call_bedrock-->>Caller: response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ventis/llm/bedrock.py`:
- Line 12: Update the Redis port configuration in the Bedrock initialization to
use a string default when calling os.environ.get, then convert the retrieved
value to an integer for the port argument.
- Around line 3-8: Update the import block in bedrock.py to use the canonical
package import consistently, while retaining the fallback imports for
environments where the package import is unavailable. Format both try and except
branches according to Ruff’s import-order and formatting rules so I001 and
PLR0402 are resolved.
- Around line 35-50: Make the Redis telemetry calls in the exception and finally
paths best-effort: wrap _redis.client.incr and _redis.hset_multiple in separate
protection that catches and logs telemetry failures, while preserving the
original Bedrock exception or result for the caller. Keep the existing telemetry
fields and agent/future conditions unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1c89367-f1b9-412f-af85-9e5dea7234f1
📒 Files selected for processing (2)
ventis/llm/__init__.pyventis/llm/bedrock.py
| try: | ||
| from ventis.utils.redis_client import RedisClient | ||
| import ventis.ventis_context as ventis_context | ||
| except ImportError: | ||
| from redis_client import RedisClient | ||
| import ventis_context |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the import block so CI passes.
Ruff reports I001 and PLR0402 here. Use the canonical package import and format both branches while preserving the intended fallback.
Proposed fix
try:
- from ventis.utils.redis_client import RedisClient
- import ventis.ventis_context as ventis_context
+ from ventis import ventis_context
+ from ventis.utils.redis_client import RedisClient
except ImportError:
- from redis_client import RedisClient
import ventis_context
+ from redis_client import RedisClient📝 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.
| try: | |
| from ventis.utils.redis_client import RedisClient | |
| import ventis.ventis_context as ventis_context | |
| except ImportError: | |
| from redis_client import RedisClient | |
| import ventis_context | |
| try: | |
| from ventis import ventis_context | |
| from ventis.utils.redis_client import RedisClient | |
| except ImportError: | |
| import ventis_context | |
| from redis_client import RedisClient |
🧰 Tools
🪛 GitHub Actions: CI / 0_test.txt
[error] 5-7: Ruff (PLR0402): Use from ventis import ventis_context in lieu of alias.
🪛 GitHub Actions: CI / test
[error] 4-6: Ruff I001: Import block is un-sorted or un-formatted.
[error] 5-5: Ruff PLR0402: Use from ventis import ventis_context in lieu of alias.
[error] 7-8: Ruff I001: Import block is un-sorted or un-formatted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/llm/bedrock.py` around lines 3 - 8, Update the import block in
bedrock.py to use the canonical package import consistently, while retaining the
fallback imports for environments where the package import is unavailable.
Format both try and except branches according to Ruff’s import-order and
formatting rules so I001 and PLR0402 are resolved.
Source: Pipeline failures
|
|
||
| _redis = RedisClient( | ||
| host=os.environ.get("VENTIS_REDIS_HOST", "localhost"), | ||
| port=int(os.environ.get("VENTIS_REDIS_PORT", 6379)), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a string environment default.
Ruff PLW1508 flags the integer default passed to os.environ.get; convert the environment string after retrieval.
Proposed fix
- port=int(os.environ.get("VENTIS_REDIS_PORT", 6379)),
+ port=int(os.environ.get("VENTIS_REDIS_PORT", "6379")),📝 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.
| port=int(os.environ.get("VENTIS_REDIS_PORT", 6379)), | |
| port=int(os.environ.get("VENTIS_REDIS_PORT", "6379")), |
🧰 Tools
🪛 GitHub Actions: CI / test
[error] 12-12: Ruff PLW1508: Invalid type for environment variable default; expected str or None.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/llm/bedrock.py` at line 12, Update the Redis port configuration in the
Bedrock initialization to use a string default when calling os.environ.get, then
convert the retrieved value to an integer for the port argument.
Source: Pipeline failures
| agent_id = ventis_context.get_current_agent_id() | ||
| if agent_id: | ||
| _redis.client.incr(f"{agent_id}:llm_errors") | ||
| raise | ||
| finally: | ||
| if future_id: | ||
| usage = (response or {}).get("usage", {}) | ||
| _redis.hset_multiple(f"future:{future_id}", { | ||
| "model": model_id, | ||
| "input_token_count": str(usage.get("inputTokens", "")), | ||
| "output_token_count": str(usage.get("outputTokens", "")), | ||
| "token_count": str(usage.get("totalTokens", "")), | ||
| "errors": str(error_count), | ||
| "input_cache_tokens": str(usage.get("cacheReadInputTokens", "")), | ||
| "input_cache_write_tokens": str(usage.get("cacheWriteInputTokens", "")), | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols"
fd -a 'bedrock.py|redis.py|context.py' . | sed 's#^\./##' | head -100
echo
echo "Relevant source outline"
ast-grep outline ventis/llm/bedrock.py --view expanded || true
echo
echo "Relevant bedrock.py"
cat -n ventis/llm/bedrock.py | sed -n '1,140p'
echo
echo "Search for _redis usage"
rg -n "_redis\\.(incr|hset_multiple|get_client|client)|hset_multiple\\(" ventis -SRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 3622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ventis/utils/redis_client.py"
cat -n ventis/utils/redis_client.py | sed -n '1,180p'
echo
echo "Search import and exception handling around call_bedrock"
rg -n "call_bedrock\\(|Bedrock|bedrock" ventis -S
echo
echo "Python exception semantics probe: exception in except/finally blocks and finally exception suppresses original"
python3 - <<'PY'
def original_error():
try:
raise RuntimeError("bedrock")
except RuntimeError as e:
raise RuntimeError("redis")
finally:
try:
original_error()
except RuntimeError:
...
try:
original_error()
except Exception as exc:
print(type(exc).__name__, str(exc))
print("has __cause__:", hasattr(exc, "__cause__"), getattr(exc, "__cause__", None))
PYRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 4042
Make Redis telemetry best-effort and preserve Bedrock errors.
_redis.client.incr(...) and hset_multiple(...) currently run without protection, so Redis failures can suppress or replace the original Bedrock result/exception. Catch and log Redis telemetry errors separately so the caller still receives the primary Bedrock outcome.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/llm/bedrock.py` around lines 35 - 50, Make the Redis telemetry calls
in the exception and finally paths best-effort: wrap _redis.client.incr and
_redis.hset_multiple in separate protection that catches and logs telemetry
failures, while preserving the original Bedrock exception or result for the
caller. Keep the existing telemetry fields and agent/future conditions
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ventis/stub_generator.py`:
- Line 316: Update the destination filename in the stub generator’s
source-to-destination mapping to remove the trailing comma, ensuring the Bedrock
module is generated as bedrock.py so it remains importable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 391113ba-cfbc-4d27-acce-b3f6c7682953
📒 Files selected for processing (1)
ventis/stub_generator.py
| "local_controller_frontend.py", | ||
| ), | ||
| (os.path.join(script_dir, "utils", "redis_client.py"), "redis_client.py"), | ||
| (os.path.join(script_dir, "llm", "bedrock.py"), "bedrock.py,"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the trailing comma from the destination filename.
This copies the module as bedrock.py,, so import bedrock will fail in the generated Docker context.
🐛 Proposed fix
- (os.path.join(script_dir, "llm", "bedrock.py"), "bedrock.py,"),
+ (os.path.join(script_dir, "llm", "bedrock.py"), "bedrock.py"),📝 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.
| (os.path.join(script_dir, "llm", "bedrock.py"), "bedrock.py,"), | |
| (os.path.join(script_dir, "llm", "bedrock.py"), "bedrock.py"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ventis/stub_generator.py` at line 316, Update the destination filename in the
stub generator’s source-to-destination mapping to remove the trailing comma,
ensuring the Bedrock module is generated as bedrock.py so it remains importable.
1 participant
Added a folder in ventis/ called llm/, which can be imported to run the bedrock client with extra logging.
The ingestion of these metrics into a db is not implemented in this PR though. This PR is a pure addition of a import to replace calling the bedrock client directly.
Summary by CodeRabbit