added an optional environment parameter to agent.run_batch#205
Conversation
|
I see the linter fails due to a type error for Environment. The issue is that importing Environment causes a circular dependency. I can just omit this type check entirely if this is preferred. |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Non-deterministic LLM Selection ▹ view | |
|
Missing Stop Check After Environment React ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| tapeagents/agent.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| if environment is not None and (not hasattr(environment, "react") or not callable(environment.react)): | ||
| raise ValueError("Environment must have a callable react method") | ||
|
|
||
| original_tapes = list(tapes) |
There was a problem hiding this comment.
Non-deterministic LLM Selection 
Tell me more
What is the issue?
The LLM selection logic is not deterministic when there's no 'default' LLM. Using next(iter()) on a dictionary can return any key-value pair.
Why this matters
This non-deterministic behavior could lead to inconsistent results across different runs, especially when batch processing multiple tapes.
Suggested change ∙ Feature Preview
Replace with explicit LLM selection logic that guarantees consistent behavior:
if 'default' in self.llms:
llm = self.llms['default']
elif len(self.llms) == 1:
llm = next(iter(self.llms.values()))
else:
raise ValueError("No default LLM specified and multiple LLMs present")Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
This is false because I check if the length of the llm array is more than 1, so this is unnecessary. this is deterministic.
|
|
||
| # Update metadata for all tapes |
There was a problem hiding this comment.
Missing Stop Check After Environment React
Tell me more
What is the issue?
The environment reaction is applied after each step generation, but it's unclear if steps after the environment reaction should lead to agent termination.
Why this matters
If the environment's reaction creates a state that should stop the agent, this isn't currently checked, potentially leading to unnecessary continued processing.
Suggested change ∙ Feature Preview
Add a stop check after environment reaction:
# Apply environment reactions if environment is provided
if environment is not None:
tapes[tape_idx] = environment.react(tapes[tape_idx])
# Check if agent should stop after environment reaction
if subagent.should_stop(tapes[tape_idx]):
pending_tape_indices.remove(tape_idx)
continueProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
I'm not sure if this is really relevant, is it possible for the agent to stop immediately after an environment reaction?
There was a problem hiding this comment.
You bring up a good point, we may need to consider cases where the agent should stop immediately after an environment reaction, but doesn't. I would suggest adding this logic as @korbit-ai advised. Although the conditions under which the agent would need to stop after an environment reaction are still ambiguous, I believe adding this validation check increases the robustness of our code. What do you think?
NicolasAG
left a comment
NicolasAG
left a comment
There was a problem hiding this comment.
This looks like it is really useful! I never used batch_run() in my agents yet so I can't test if it breaks anything the previous batch_run() function did.
The main weak point I see is that for this to work, the Environments need to be state-LESS. Otherwise executing multiple tapes in parallel may conflict the internal state of the environment.
Potential solution: take as input a list of environments, 1 per tape. If not, have a warning message saying that we will assume the environment is StateLESS.
Don't know if it's worth doing in this PR or in another one. @AlexPiche @rafapi
|
I think we are moving to using async Python for many-agent orchestration, folks. See e.g. #225 . |
4 participants
Fixed the following things in this patch:
agent.run_batchto take an environment parameterset()issue so that now tapes can end prematurely and not mess up the indicesTrainableLLMto just ensuring the passed LLM has abatch_generatemethodDescription by Korbit AI
What change is being made?
Add an optional
environmentparameter to therun_batchmethod in theAgentclass to allow environment integration during parallel batch processing of tapes.Why are these changes being made?
This change enhances the flexibility of the
run_batchmethod by enabling it to interact with a provided environment, which can modify tapes through itsreactmethod. This is necessary for scenarios where the processing of tapes requires interaction with dynamic external conditions or systems.