Conversation
catebros
commented
Mar 28, 2026
catebros
commented
- Depends on Flowgentic Hotfix/benchmarking-redesign PR (For Academy and Autogen implementation and event model)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new BackendComparison experiment to benchmark agent orchestration frameworks, including LangGraph, AutoGen, and Academy, against backend engines like AsyncFlow and Parsl. The update adds new workload implementations, a comprehensive plotting utility, and refined dependency management. Review feedback identifies several issues: a potential crash in the Discord notification function due to missing environment variable validation, unreachable code in the notification utility, a logic error in invocation count reporting that could lead to misleading metrics, and the inclusion of an unused schema file.
| webhook_url = os.getenv("DISCORD_WEBHOOK") | ||
| data = {"content": msg} | ||
| requests.post(webhook_url, json=data) |
There was a problem hiding this comment.
The send_discord_notifaction function does not handle the case where the DISCORD_WEBHOOK environment variable is not set. If os.getenv("DISCORD_WEBHOOK") returns None, requests.post(None, ...) will raise an exception, crashing the program. Please add a check for the webhook URL.
This function also duplicates logic from data_generation/utils/io_utils.py. Consider refactoring to use a single notification utility.
webhook_url = os.getenv("DISCORD_WEBHOOK")
if webhook_url:
data = {"content": msg}
try:
requests.post(webhook_url, json=data, timeout=5).raise_for_status()
except requests.exceptions.RequestException as e:
logger.error(f"Failed to send Discord notification: {e}")| self.webhook_url = os.getenv("DISCORD_WEBHOOK") | ||
|
|
||
| def send_discord_notification(self, msg: str, image_path: str = None): | ||
| return None |
There was a problem hiding this comment.
This return None statement makes the rest of the function unreachable code. This appears to be a temporary change for debugging. Please ensure this is removed before merging to allow Discord notifications to work as intended.
| for engine_id, record in data.items(): | ||
| d = _extract_event_durations(record["events"]) | ||
|
|
||
| n_inv = len(d["d_resolve"]) or 1 |
There was a problem hiding this comment.
Using or 1 here can lead to incorrect reporting. If there are zero invocations, n_inv will be set to 1, which is misleading when this metric is displayed in the results table. It's better to correctly report 0 and handle potential division-by-zero errors where the division actually occurs, if any.
| n_inv = len(d["d_resolve"]) or 1 | |
| n_inv = len(d["d_resolve"]) |
| @@ -0,0 +1 @@ | |||
| from pydantic import BaseModel No newline at end of file | |||
There was a problem hiding this comment.
This file appears to be unused. The main.py for this experiment imports schemas from data_generation/utils/schemas. If this file is not needed, it should be removed to avoid confusion. Also, it's missing a newline at the end of the file, which is a style violation.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
1 participant
