feat: add maximum evaluation time limit with PROMPTFOO_MAX_EVAL_TIME_MS #4322
Conversation
…MS env var and maxEvalTimeMs option
…d direct action-oriented guidance
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mldangelo - I've reviewed your changes and found some issues that need to be addressed.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
Adds a global time‐limit feature for evaluations so users can cap total run time via maxEvalTimeMs or PROMPTFOO_MAX_EVAL_TIME_MS.
- Introduces a new
maxEvalTimeMsoption and corresponding environment variable for overall evaluation timeouts - Implements global abort logic in
Evaluator.evaluate, tracks pending steps, and records timeout results - Updates types, schema, documentation, and tests to cover the new feature
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/evaluator.test.ts | Refactored mocks and added a test for aborting when maxEvalTimeMs is exceeded |
| src/types/index.ts | Added maxEvalTimeMs to EvaluateOptionsSchema and the exported type |
| src/evaluator.ts | Implemented global timeout/abort logic, tracking, and pending-result handling |
| src/envars.ts | Introduced PROMPTFOO_MAX_EVAL_TIME_MS env var and getMaxEvalTimeMs helper |
| site/static/config-schema.json | Extended JSON schema with "maxEvalTimeMs" property |
| site/docs/usage/troubleshooting.md | Updated troubleshooting guide with triage steps and examples |
| site/docs/usage/command-line.md | Documented the new PROMPTFOO_MAX_EVAL_TIME_MS and updated related tips |
| * @returns The max duration in milliseconds, or the default value if not set. | ||
| */ | ||
| export function getMaxEvalTimeMs(defaultValue: number = 0): number { | ||
| return getEnvInt('PROMPTFOO_MAX_EVAL_TIME_MS', defaultValue); |
There was a problem hiding this comment.
The implementation calls getEnvInt with a defaultValue parameter, but getEnvInt only accepts one argument. This means the defaultValue is ignored and may cause a type error; change to const val = getEnvInt('PROMPTFOO_MAX_EVAL_TIME_MS'); return val != null ? val : defaultValue;.
| return getEnvInt('PROMPTFOO_MAX_EVAL_TIME_MS', defaultValue); | |
| const val = getEnvInt('PROMPTFOO_MAX_EVAL_TIME_MS'); | |
| return val != null ? val : defaultValue; |
Copilot uses AI. Check for mistakes.
| const originalReadFileSync = fs.readFileSync; | ||
| fs.readFileSync = jest.fn().mockImplementation((path) => { | ||
| if (path.includes('test_file.txt')) { | ||
| jest.spyOn(fs, 'readFileSync').mockImplementation((path) => { | ||
| if (typeof path === 'string' && path.includes('test_file.txt')) { | ||
| return '<h1>Sample Report</h1><p>This is a test report with some data for the year 2023.</p>'; | ||
| } | ||
| return originalReadFileSync(path); |
There was a problem hiding this comment.
[nitpick] After using jest.spyOn on fs.readFileSync, the spy is never restored and could leak into other tests. Consider adding afterEach(() => jest.restoreAllMocks()); or calling .mockRestore() on the spy.
Copilot uses AI. Check for mistakes.
| logger.info( | ||
| `[${numComplete}/${serialRunEvalOptions.length}] Running ${provider} with vars: ${vars}`, | ||
| ); | ||
| try { |
There was a problem hiding this comment.
[nitpick] The large try/catch block covers both serial and concurrent evaluation logic, making the method hard to follow. Consider extracting serial and concurrent processing into separate helper functions for clarity.
Copilot uses AI. Check for mistakes.
Co-authored-by: gru-agent[bot] <185149714+gru-agent[bot]@users.noreply.github.com>
Adds a new maximum evaluation time limit feature with PROMPTFOO_MAX_EVAL_TIME_MS environment variable and maxEvalTimeMs API option. Includes comprehensive documentation and test coverage. Useful for CI/CD time limits, cost control, and preventing runaway evaluations.