-
Notifications
You must be signed in to change notification settings - Fork 84
Privileged _ debugging #281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Status |
---|---|---|
Silent Failure in Privileged Action Processing ▹ view | ||
Unused function without clear purpose ▹ view | ||
Inefficient Row-wise DataFrame Operation ▹ view | ||
Redundant State Change Before Exception ▹ view |
Files scanned
File Path | Reviewed |
---|---|
src/agentlab/agents/generic_agent/generic_agent.py | ✅ |
src/agentlab/agents/privaleged_info_agent/privaleged_agent_prompt.py | ✅ |
src/agentlab/agents/privaleged_info_agent/privaleged_agent.py | ✅ |
src/agentlab/analyze/json_xray.py | ✅ |
src/agentlab/experiments/loop.py | ✅ |
src/agentlab/analyze/agent_xray.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
def extract_section(text: str, start_marker: str, end_marker: str) -> str | None: | ||
if not isinstance(text, str): | ||
return None | ||
try: | ||
start = text.find(start_marker) | ||
if start == -1: | ||
return None | ||
start += len(start_marker) | ||
end = text.find(end_marker, start) | ||
if end == -1: | ||
return text[start:].strip() | ||
return text[start:end].strip() | ||
except Exception: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function without clear purpose 
Tell me more
What is the issue?
The extract_section function appears unused in the code and its purpose is unclear without context.
Why this matters
Dead code increases cognitive load as readers try to understand its purpose and relationship to other code. If unused, it should be removed.
Suggested change ∙ Feature Preview
Either remove the unused function or if it's needed for future use, add a comment explaining its purpose and mark it as reserved for future use:
# Reserved for future use: Extracts text between start_marker and end_marker from conversation logs
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
""" | ||
) | ||
except Exception as e: | ||
# raise RuntimeError(f"No privilaged action will for goal {self.goal}.") from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent Failure in Privileged Action Processing 
Tell me more
What is the issue?
The code suppresses a potential exception when handling privileged actions by commenting out the error raising line.
Why this matters
If an error occurs while processing the trajectory for privileged actions, the code silently fails. This can lead to unexpected behavior where the agent continues without the required trajectory information.
Suggested change ∙ Feature Preview
The error should be properly handled to ensure the agent behaves correctly when trajectory information is missing:
try:
# trajectory processing code
except Exception as e:
raise RuntimeError(f"Failed to process privileged action trajectory for goal {self.goal}.") from e
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
@@ -1003,14 +1021,17 @@ def get_seeds_df(result_df: pd.DataFrame, task_name: str): | |||
def extract_columns(row: pd.Series): | |||
return pd.Series( | |||
{ | |||
"seed": row[TASK_SEED_KEY], | |||
"index": row.get("_row_index", None), | |||
"seed": row.get(TASK_SEED_KEY, None), | |||
"reward": row.get("cum_reward", None), | |||
"err": bool(row.get("err_msg", None)), | |||
"n_steps": row.get("n_steps", None), | |||
} | |||
) | |||
|
|||
seed_df = result_df.apply(extract_columns, axis=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inefficient Row-wise DataFrame Operation 
Tell me more
What is the issue?
Using pandas apply() with axis=1 which is computationally expensive as it iterates row by row.
Why this matters
Row-wise operations in pandas are much slower than vectorized operations, impacting performance especially with large dataframes.
Suggested change ∙ Feature Preview
Use vectorized operations instead:
seed_df = pd.DataFrame({
'index': result_df.get('_row_index', None),
'seed': result_df.get(TASK_SEED_KEY, None),
'reward': result_df.get('cum_reward', None),
'err': result_df.get('err_msg', None).astype(bool),
'n_steps': result_df.get('n_steps', None)
})
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
sub_df = tmp_df[tmp_df["_row_index"] == episode_id.row_index] | ||
if len(sub_df) == 0: | ||
self.exp_result = None | ||
raise ValueError(f"Could not find episode for row_index: {episode_id.row_index}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant State Change Before Exception 
Tell me more
What is the issue?
The code sets exp_result to None before raising the ValueError, which is unnecessary since the exception will interrupt execution.
Why this matters
Setting state before raising an exception is redundant and could be misleading to maintainers about the object's state after an exception.
Suggested change ∙ Feature Preview
Remove redundant state change:
if len(sub_df) == 0:
raise ValueError(f"Could not find episode for row_index: {episode_id.row_index}")
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Description by Korbit AI
What change is being made?
Introduce the
PrivalegedAgent
and its components for enhanced privileged information handling, integrate new privileged action pathways, and add row indexing to various analysis modules.Why are these changes being made?
These changes are made to enable the
GenericAgent
to utilize privileged action pathways, offering more advanced debugging and tracking capabilities, which improves the contextually aware decision-making process in complex tasks. Additionally, the row indexing enhancements in analysis modules likeagent_xray.py
andjson_xray.py
provide better traceability and data inspection during experiments.