[Improvement] Putting all the future data into one object - #35
Open
Saaketh0 wants to merge 9 commits into
Open
Conversation
…e Redis key Fold execution metrics (cpu/gpu, timings, LLM usage, failure details) into the future's own hash instead of a separate :metrics key, so a single snapshot travels between origin and executor in both request and completion callbacks. Also fixes the completion callback firing before final metrics (finished_at/cpu_resource/gpu_resource/agent/queue_time) were written. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves conflicts between the future:{future_id}/:metrics consolidation
and origin/main's changes by keeping the consolidated-key implementation
(and layering it on top of main's telemetry_logging.py/session_logging.py
renames, stub_generator.py updates, etc.).
Also finishes a merge origin/main had left broken: portfolio/agents/
intent_agent.py had unresolved <<<<<<< HEAD markers (SyntaxError),
global_controller.yaml had leftover duplicate/incomplete agent entries,
and advisor_agent.py had dead code referencing unset self.model_id/
self.region. Resolved all three in favor of the direct
ventis.llm.bedrock.call_bedrock pattern (matching vllm_agent.py), dropping
the newly-added but unwired shared LLMAgent stub and its policy/config
references so Bedrock calls keep flowing through per-future telemetry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
slimmed down useless function removed useless file removed some useless comments
📝 WalkthroughWalkthroughThe PR unifies future execution state, results, errors, and telemetry in ChangesUnified future state and execution flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Future
participant LocalController
participant Redis
participant OriginController
Future->>Redis: Store request and created_at
Future->>LocalController: Submit execution request
LocalController->>Redis: Store status and final metrics
LocalController->>OriginController: Send complete future snapshot
OriginController->>Redis: Merge callback state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ventis/llm/bedrock.py (1)
41-52: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve telemetry per future across multiple Bedrock calls.
error_countresets each invocation, andhset_multipleoverwrites future hash fields. If one future makes more than one Bedrock call, a later response can reseterrorsand replace earlier token usage. Accumulate these fields atomically, or enforce and test a one-call-per-future contract.🤖 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 41 - 52, The finally block’s future telemetry update must preserve accumulated usage and errors across multiple Bedrock calls instead of resetting or overwriting prior values. Update the logic around error_count and _redis.hset_multiple to atomically increment or merge existing future:{future_id} fields, including token counts and errors, while retaining current behavior for single calls.
🤖 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 `@examples/portfolio/agents/advisor_agent.py`:
- Around line 11-12: Update summarize to extract the first Bedrock text value,
validate that it is non-empty, and return _fallback_summary when the response is
empty instead of returning it directly. Apply the same handling to both affected
return paths while preserving the existing deterministic fallback behavior.
In `@ventis/controller/local_controller.py`:
- Around line 627-639: Update the pre-execution failure handling around
`_mark_future_failed` so the `self.agent is None` and missing-method paths do
not return with `origin` callbacks immediately. Call `_mark_future_failed`
without `origin`, then route both failures through the existing finalization
callback path so all final metrics and agent data are written before the single
callback; add remote tests covering both failure cases.
---
Outside diff comments:
In `@ventis/llm/bedrock.py`:
- Around line 41-52: The finally block’s future telemetry update must preserve
accumulated usage and errors across multiple Bedrock calls instead of resetting
or overwriting prior values. Update the logic around error_count and
_redis.hset_multiple to atomically increment or merge existing
future:{future_id} fields, including token counts and errors, while retaining
current behavior for single calls.
🪄 Autofix
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: 87fb73d5-2c5b-43c1-885e-65fe35de464e
📒 Files selected for processing (17)
examples/portfolio/agents/advisor_agent.pyexamples/portfolio/agents/intent_agent.pyexamples/portfolio/agents/llm_agent.pyexamples/portfolio/agents/llm_agent.yamlexamples/portfolio/config/global_controller.yamlexamples/portfolio/config/policy.yamlexamples/text2sql/agents/vllm_agent.pytests/test_error_propagation.pytests/test_future.pytests/test_local_controller_metrics.pytests/test_telemetry_logging.pyventis/FUTURE_SCHEMA.mdventis/controller/local_controller.pyventis/controller/local_controller_frontend.pyventis/controller/utils/telemetry_logging.pyventis/future.pyventis/llm/bedrock.py
💤 Files with no reviewable changes (4)
- examples/portfolio/agents/llm_agent.yaml
- examples/portfolio/config/policy.yaml
- examples/portfolio/agents/llm_agent.py
- examples/portfolio/config/global_controller.yaml
Comment on lines
11
to
12
| # If the LLM is unavailable (returns an empty string), it falls back to a | ||
| # deterministic templated summary so the pipeline still returns. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle empty Bedrock output before returning.
The module documentation promises a deterministic fallback when Bedrock returns an empty string. summarize returns the first text value directly, so an empty response reaches the caller instead of _fallback_summary.
Extract and validate the text before returning it.
Proposed fix
- return response["output"]["message"]["content"][0]["text"]
+ text = response["output"]["message"]["content"][0].get("text", "").strip()
+ if not text:
+ return self._fallback_summary(metrics, risk)
+ return textAlso applies to: 42-45
🤖 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 `@examples/portfolio/agents/advisor_agent.py` around lines 11 - 12, Update
summarize to extract the first Bedrock text value, validate that it is
non-empty, and return _fallback_summary when the response is empty instead of
returning it directly. Apply the same handling to both affected return paths
while preserving the existing deterministic fallback behavior.
Comment on lines
+627
to
+639
| # Send the completion callback only now that every final metric | ||
| # has been written, so the snapshot sent to origin is complete. | ||
| if origin and origin != self._my_endpoint: | ||
| if succeeded: | ||
| self._send_result_callback( | ||
| origin, future_id, result=serialized, failed=0, error_message="" | ||
| ) | ||
| else: | ||
| error_message = self.redis.hget(f"future:{future_id}", "error") | ||
| self._send_result_callback( | ||
| origin, future_id, failed=1, error_message=error_message or "" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Defer callbacks for pre-execution failures.
When self.agent is None or the method is missing, Lines 554-565 call _mark_future_failed(..., origin) and return before this finalization path. _mark_future_failed then sends a callback without finished_at, cpu_resource, gpu_resource, or agent.
Route these failures through this finally block. Call _mark_future_failed without origin before the single deferred callback. Add remote tests for both failure paths.
🤖 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/controller/local_controller.py` around lines 627 - 639, Update the
pre-execution failure handling around `_mark_future_failed` so the `self.agent
is None` and missing-method paths do not return with `origin` callbacks
immediately. Call `_mark_future_failed` without `origin`, then route both
failures through the existing finalization callback path so all final metrics
and agent data are written before the single callback; add remote tests covering
both failure cases.
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
1 participant
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.
In the past, metrics for each future was created by both the executor and origin nodes, with disparate data being sent back and forth, but now, we just send the entire future object back and forth, leading to one source of truth.
The executor still writes the data to its own Redis cache to be picked up by the polling global controller.
Fixing Issue #29
In addition,
Summary by CodeRabbit
New Features
Bug Fixes
Documentation