-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: add configurable timeout for evals (5-10 min) #5865
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
Conversation
- Add timeout field to CreateRun schema with min 5, max 10, default 5 - Add timeout slider UI component to /runs/new page - Update database schema to include timeout column in runs table - Create migration to add timeout column with default value of 5 - Update runTask.ts to use configurable timeout from run settings - Pass timeout parameter through the createRun action
@roomote-agent can you fix that failing test? |
I can see the failing test - there appears to be a compilation failure. Let me check out the branch and investigate the issue. |
I can see the request to fix the failing test. Let me check out the branch and investigate the current status of the tests. |
- Added timeout: 5 to both createRun calls in copyRun.spec.ts - This fixes the test failure caused by the new required timeout field in the runs schema - The timeout field was added in the configurable timeout feature but the test was not updated
✅ Fixed the failing test! Issue identified: The test in Solution applied:
Results:
The PR is ready for review and merge! |
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.
LGTM
@roomote-agent - There's a critical issue with the Redis key expiration that needs to be fixed. Problem: The Impact: When users set a timeout longer than 5 minutes, the Redis runner keys will expire before the task completes, potentially causing issues with task tracking. Solution: Update the // In redis.ts
export const registerRunner = async ({ runId, taskId, timeoutSeconds }: { runId: number; taskId: number; timeoutSeconds: number }) => {
const redis = await redisClient()
const runnersKey = getRunnersKey(runId)
await redis.sAdd(runnersKey, `task-${taskId}:${process.env.HOSTNAME ?? process.pid}`)
await redis.expire(runnersKey, timeoutSeconds)
} // In runTask.ts line 38
await registerRunner({ runId: run.id, taskId, timeoutSeconds: (run.timeout || 5) * 60 }) This ensures the Redis keys remain valid for the entire duration of the task execution. |
👋 I've seen your comment about the Redis key expiration issue. You're absolutely right - this is a critical problem that needs to be fixed. I'll check out the branch and implement the solution to make the registerRunner function use the configurable timeout instead of the hardcoded EVALS_TIMEOUT. Working on it now! |
- Updated registerRunner function to accept timeoutSeconds parameter - Modified call in runTask.ts to pass configurable timeout instead of hardcoded EVALS_TIMEOUT - Removed unused EVALS_TIMEOUT import from redis.ts - Ensures Redis keys remain valid for the entire duration of task execution (up to 10 minutes)
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.
LGTM
* feat: add configurable timeout for evals (5-10 min) - Add timeout field to CreateRun schema with min 5, max 10, default 5 - Add timeout slider UI component to /runs/new page - Update database schema to include timeout column in runs table - Create migration to add timeout column with default value of 5 - Update runTask.ts to use configurable timeout from run settings - Pass timeout parameter through the createRun action * fix: remove unused EVALS_TIMEOUT import * fix: add timeout field to createRun calls in copyRun test - Added timeout: 5 to both createRun calls in copyRun.spec.ts - This fixes the test failure caused by the new required timeout field in the runs schema - The timeout field was added in the configurable timeout feature but the test was not updated * fix: use configurable timeout for Redis key expiration in registerRunner - Updated registerRunner function to accept timeoutSeconds parameter - Modified call in runTask.ts to pass configurable timeout instead of hardcoded EVALS_TIMEOUT - Removed unused EVALS_TIMEOUT import from redis.ts - Ensures Redis keys remain valid for the entire duration of task execution (up to 10 minutes) --------- Co-authored-by: Roo Code <[email protected]> Co-authored-by: hannesrudolph <[email protected]>
This PR adds a configurable timeout setting to the evals setup (/runs/new) that allows users to change the current 5-minute timeout up to 10 minutes.
Changes Made
Frontend Changes
Backend Changes
UI/UX
Testing
Addresses the Slack request to add configurable timeout with slider interface.
Important
Adds configurable timeout (5-10 min) for evals with frontend slider and backend support, updating database schema and tests.
createRun
function inruns.ts
./runs/new
page innew-run.tsx
.createRun
function to accepttimeout
parameter.runTask.ts
to use the configurable timeout instead of hardcoded value.redis.ts
to usetimeoutSeconds
for runner registration.timeout
column toruns
table inschema.ts
and migration file0001_add_timeout_to_runs.sql
.copyRun.spec.ts
to includetimeout
in test cases.This description was created by
for 19d0f28. You can customize this summary. It will automatically update as commits are pushed.