-
-
Notifications
You must be signed in to change notification settings - Fork 758
fix(providers): ensure Python worker writes error response before signaling completion #6073
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
|
⏩ No test execution environment matched (eb0f3a0) View output ↗ View check history
|
fc44afa to
5926de4
Compare
📝 WalkthroughWalkthroughThis pull request fixes an issue where Python worker errors were not being persisted to response files before signaling completion, resulting in ENOENT (file not found) errors. The fix adds error response writing to the persistent_wrapper.py exception handler that serializes error details to the response file before continuing with completion signaling. Supporting changes include a test fixture simulating non-existent function calls and corresponding test cases in the TypeScript worker test suite. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CHANGELOG.md (1)
19-20: Optional wording tweak for consistency.Consider “Python provider” instead of “Python worker” to match prior entries’ terminology.
-- fix(providers): ensure Python worker writes error response before signaling completion to prevent ENOENT errors when function calls fail (#6073) +- fix(providers): ensure Python provider writes an error response file before signaling completion to prevent ENOENT errors when function calls fail (#6073)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)src/python/persistent_wrapper.py(2 hunks)test/python/fixtures/test_nonexistent_function.py(1 hunks)test/python/worker.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
**/*.py: Use Python 3.9 or later
Follow the Google Python Style Guide
Use type hints to improve code readability and catch potential errors
Userufffor linting and formatting
Runruff check --fixfor general linting
Runruff check --select I --fixfor import sorting
Runruff formatfor formatting
Keep the Python codebase simple and minimal, without unnecessary external dependencies
When implementing custom providers, prompts, or asserts in Python, follow the promptfoo API patterns
Files:
test/python/fixtures/test_nonexistent_function.pysrc/python/persistent_wrapper.py
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/python.mdc)
Write unit tests for new Python functions using the built-in
unittestmodule
Files:
test/python/fixtures/test_nonexistent_function.py
test/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize tests to mirror src/ structure (e.g., test/providers → src/providers, test/redteam → src/redteam)
Place tests under test/
Files:
test/python/fixtures/test_nonexistent_function.pytest/python/worker.test.ts
test/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.cursor/rules/jest.mdc)
test/**/*.{test,spec}.ts: Mock as few functions as possible to keep tests realistic
Never increase the function timeout - fix the test instead
Organize tests in descriptivedescribeanditblocks
Prefer assertions on entire objects rather than individual keys when writing expectations
Clean up after tests to prevent side effects (e.g., useafterEach(() => { jest.resetAllMocks(); }))
Run tests with--randomizeflag to ensure your mocks setup and teardown don't affect other tests
Use Jest's mocking utilities rather than complex custom mocks
Prefer shallow mocking over deep mocking
Mock external dependencies but not the code being tested
Reset mocks between tests to prevent test pollution
For database tests, use in-memory instances or proper test fixtures
Test both success and error cases for each provider
Mock API responses to avoid external dependencies in tests
Validate that provider options are properly passed to the underlying service
Test error handling and edge cases (rate limits, timeouts, etc.)
Ensure provider caching behaves as expected
Always include both--coverageand--randomizeflags when running tests
Run tests in a single pass (no watch mode for CI)
Ensure all tests are independent and can run in any order
Clean up any test data or mocks after each test
Files:
test/python/worker.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Never increase Jest test timeouts; fix slow tests instead (avoid jest.setTimeout or large timeouts in tests)
Do not use .only() or .skip() in committed tests
Add afterEach(() => { jest.resetAllMocks(); }) to ensure mock cleanup
Prefer asserting entire objects (toEqual on whole result) rather than individual fields
Mock minimally: only external dependencies (APIs, databases), not code under test
Use Jest (not Vitest) APIs in this suite; avoid importing vitest
Import from @jest/globals in tests
Files:
test/python/worker.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/gh-cli-workflow.mdc)
Avoid disabling or skipping tests (e.g., .skip, .only) unless absolutely necessary and documented
Files:
test/python/worker.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/gh-cli-workflow.mdc)
Prefer not to introduce new TypeScript types; reuse existing interfaces where possible
**/*.{ts,tsx}: Maintain consistent import order (Biome handles sorting)
Use consistent curly braces for all control statements
Prefer const over let and avoid var
Use object shorthand syntax when possible
Use async/await for asynchronous code
Use consistent error handling with proper type checks
**/*.{ts,tsx}: Use TypeScript with strict type checking enabled
Follow consistent import order (Biome will sort imports)
Use consistent curly braces for all control statements
Prefer const over let; avoid var
Use object property shorthand when possible
Use async/await for asynchronous code instead of raw promises/callbacks
When logging, pass sensitive data via the logger context object so it is auto-sanitized; avoid interpolating secrets into message strings
Manually sanitize sensitive objects with sanitizeObject before storing or emitting outside logging contexts
Files:
test/python/worker.test.ts
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.{test,spec}.{ts,tsx}: Follow Jest best practices using describe/it blocks
Test both success and error cases for all functionality
Files:
test/python/worker.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow Jest best practices with describe/it blocks in tests
Files:
test/python/worker.test.ts
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/changelog.mdc)
CHANGELOG.md: Document all user-facing changes in CHANGELOG.md
Every pull request must add or update an entry in CHANGELOG.md under the [Unreleased] section
Follow Keep a Changelog structure under [Unreleased] with sections: Added, Changed, Fixed, Dependencies, Documentation, Tests, Removed
Each changelog entry must include the PR number formatted as (#1234) or temporary placeholder (#XXXX)
Each changelog entry must use a Conventional Commit prefix: feat:, fix:, chore:, docs:, test:, or refactor:
Each changelog entry must be concise and on a single line
Each changelog entry must be user-focused, describing what changed and why it matters to users
Each changelog entry must include a scope in parentheses, e.g., feat(providers): or fix(evaluator):
Use common scopes for consistency: providers, evaluator, webui or app, cli, redteam, core, assertions, config, database
Place all dependency updates under the Dependencies category
Place all test changes under the Tests category
Use categories consistently: Added for new features, Changed for modifications/refactors/CI, Fixed for bug fixes, Removed for removed features
After a PR number is assigned, replace (#XXXX) placeholders with the actual PR number
Be specific, use active voice, include context, and avoid repeating the PR title in changelog entries
Group related changes with multiple bullets in the same category when needed; use one entry per logical change
CHANGELOG.md: All user-facing changes require a CHANGELOG.md entry before creating a PR
Add entries under [Unreleased] in appropriate category (Added, Changed, Fixed, Dependencies, Documentation, Tests)
Each changelog entry must include PR number (#1234) or placeholder (#XXXX)
Use conventional commit prefixes in changelog entries (feat:, fix:, chore:, docs:, test:, refactor:)
CHANGELOG.md: Document all user-facing changes in CHANGELOG.md
Changelog entries must include the PR number in format (#1234)
Use conventional commit prefixes in changelog entries: feat:,...
Files:
CHANGELOG.md
src/**
📄 CodeRabbit inference engine (AGENTS.md)
Place core application/library logic under src/
Files:
src/python/persistent_wrapper.py
🧠 Learnings (6)
📚 Learning: 2025-07-18T17:26:23.087Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: .cursor/rules/jest.mdc:0-0
Timestamp: 2025-07-18T17:26:23.087Z
Learning: Applies to test/**/*.{test,spec}.ts : Test error handling and edge cases (rate limits, timeouts, etc.)
Applied to files:
test/python/worker.test.ts
📚 Learning: 2025-10-24T22:42:38.674Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-24T22:42:38.674Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Test both success and error cases for all functionality
Applied to files:
test/python/worker.test.ts
📚 Learning: 2025-10-24T22:41:44.088Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: .cursor/rules/gh-cli-workflow.mdc:0-0
Timestamp: 2025-10-24T22:41:44.088Z
Learning: Applies to CHANGELOG.md : Add entries under [Unreleased] in appropriate category (Added, Changed, Fixed, Dependencies, Documentation, Tests)
Applied to files:
CHANGELOG.md
📚 Learning: 2025-10-24T22:42:38.674Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-24T22:42:38.674Z
Learning: Applies to CHANGELOG.md : Add new entries under the 'Unreleased' section
Applied to files:
CHANGELOG.md
📚 Learning: 2025-10-24T22:41:09.485Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: .cursor/rules/changelog.mdc:0-0
Timestamp: 2025-10-24T22:41:09.485Z
Learning: Applies to CHANGELOG.md : Document all user-facing changes in CHANGELOG.md
Applied to files:
CHANGELOG.md
📚 Learning: 2025-10-24T22:41:09.485Z
Learnt from: CR
Repo: promptfoo/promptfoo PR: 0
File: .cursor/rules/changelog.mdc:0-0
Timestamp: 2025-10-24T22:41:09.485Z
Learning: Applies to CHANGELOG.md : Follow Keep a Changelog structure under [Unreleased] with sections: Added, Changed, Fixed, Dependencies, Documentation, Tests, Removed
Applied to files:
CHANGELOG.md
🧬 Code graph analysis (1)
test/python/worker.test.ts (1)
src/python/worker.ts (1)
PythonWorker(9-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test on Node 24.x and windows-latest
- GitHub Check: Test on Node 22.x and windows-latest
- GitHub Check: Test on Node 20.x and windows-latest
🔇 Additional comments (4)
test/python/fixtures/test_nonexistent_function.py (1)
1-8: LGTM! Clean test fixture for error scenario.The fixture appropriately simulates the non-existent function scenario by providing only
call_apiwhile omittingcall_nonexistent_api. The docstring clearly documents the intent.test/python/worker.test.ts (1)
165-180: LGTM! Properly tests the error-response fix.The test correctly verifies that calling a non-existent function now raises a meaningful AttributeError instead of ENOENT. The regex pattern
/has no attribute|AttributeError/appropriately matches the error format from Python'sgetattrfailure.Based on learnings
src/python/persistent_wrapper.py (2)
111-111: Good defensive initialization.Declaring
response_file = Nonebefore the try block ensures it's available in the outer exception handler, even if parsing fails before the file path is determined.
179-199: Excellent fix for the ENOENT race condition.The error-response writing logic correctly addresses the root cause:
- Writes error JSON to
response_filebefore signaling DONE (lines 182-190)- Uses
fsync()to ensure durability before signaling (line 190)- Handles write failures gracefully without hanging the caller (lines 191-196)
- Always signals DONE to prevent Node.js timeout (line 199)
The error format matches the inner exception handler (lines 139-143), ensuring consistent error structure across all failure paths.
| ### Fixed | ||
|
|
||
| - fix(providers): ensure Python worker writes error response before signaling completion to prevent ENOENT errors when function calls fail (#6073) | ||
|
|
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.
Add a Tests entry under Unreleased for the new regression test.
Per the changelog rules, test changes should be documented under a dedicated “Tests” category. Please add a concise Tests bullet for the regression covering the Python worker failure case.
Apply after Line 20:
+### Tests
+
+- test(providers): add regression ensuring Python worker writes error response file and returns AttributeError instead of ENOENT on nonexistent function calls (#6073)As per coding guidelines
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Fixed | |
| - fix(providers): ensure Python worker writes error response before signaling completion to prevent ENOENT errors when function calls fail (#6073) | |
| ### Fixed | |
| - fix(providers): ensure Python worker writes error response before signaling completion to prevent ENOENT errors when function calls fail (#6073) | |
| ### Tests | |
| - test(providers): add regression ensuring Python worker writes error response file and returns AttributeError instead of ENOENT on nonexistent function calls (#6073) |
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 17 to 20, the Unreleased section is missing a Tests
entry for the new regression test; add a new "### Tests" subsection immediately
after line 20 and include a single concise bullet describing the regression
test: "regression(python-worker): add test ensuring worker writes error response
before signaling completion to prevent ENOENT when function calls fail (#6073)".
Ensure formatting matches existing changelog style and place the entry under
Unreleased.
…naling completion When Python worker encounters exceptions before writing the response file (e.g., non-existent function, failed file I/O), the outer exception handler now writes an error response before sending DONE signal to Node.js. This prevents ENOENT errors when Node.js tries to read a response file that was never created. Fixes #6072
5926de4 to
b705970
Compare
…6075) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- Extract response_file path early to handle malformed CALL commands - Add detailed comments explaining ENOENT prevention logic - Sync CHANGELOG with main branch (pharmacy, insurance plugins, etc.) - Add Tests section to CHANGELOG per contribution guidelines This ensures the error response file is written even when: - CALL command format is invalid (wrong number of parts) - Function name parsing fails before response_file assignment - Any exception occurs before normal file write path All tests pass (6/6), lint and format clean.
Dramatically improve UX when users have wrong Python function names.
Before (confusing):
Python error: AttributeError: module 'llm_embedding' has no attribute 'call_api'
[unhelpful traceback in persistent_wrapper.py internals]
After (helpful):
Function 'call_embedding_api' not found in module 'test_wrong_function_name'
Available functions in your module: call_api, get_embedding_api, some_helper_function
Expected function names for promptfoo:
• call_api(prompt, options, context) - for chat/completions
• call_embedding_api(prompt, options) - for embeddings
• call_classification_api(prompt, options) - for classification
💡 Did you mean to rename 'get_embedding_api' to 'call_embedding_api'?
See https://www.promptfoo.dev/docs/providers/python/ for details.
Features:
- Lists all available functions in user's module
- Shows expected function signatures
- Fuzzy matching suggestions for common mistakes (get_ vs call_ prefix)
- Direct link to documentation
- Addresses issue #6072 where user had get_embedding_api instead of call_embedding_api
Tests:
- Added test fixture simulating common user error (get_ instead of call_ prefix)
- Added comprehensive test verifying error message content
- All 7 Python worker tests pass
Resolved CHANGELOG conflict by combining entries from both branches in PR number order.
42e86bb to
1a189b5
Compare
Removes early function validation from Python worker initialization that prevented embeddings-only and classification-only providers from working. Problem: - Worker validated call_api exists at startup (line 128) - Embeddings-only providers failed: "call_api not found" - User from #6072 had only call_embedding_api, no call_api - Early validation was legacy code from before dynamic function calling Solution: - Removed premature validation from main() initialization - Functions already validated in handle_call() when actually called - Added explanatory comment about dynamic function protocol - Better error messages already in place from previous improvements Benefits: - Embeddings-only providers work (only define call_embedding_api) - Classification-only providers work (only define call_classification_api) - Mixed providers work (define any combination) - Validation still happens at call time with helpful error messages Tests: - Added test for embeddings-only provider without call_api - Verifies worker initializes successfully - Verifies embedding calls work correctly - All 8 Python worker tests pass Fixes #6072 (user's second issue about needing call_api for embeddings)
- Split long embedding list into multiple lines for readability - Fixes Python formatting check in CI
Resolved CHANGELOG conflict by combining entries in PR number order.
Consolidated three separate changelog entries for PR #6073 into single comprehensive entries in both Fixed and Tests sections. Fixed section now includes: - Error response file writing before completion signal - Function name suggestions with fuzzy matching - Support for embeddings-only and classification-only providers Tests section covers: - ENOENT prevention regression tests - Error message improvements with function suggestions - Embeddings-only provider support verification 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Fixes ENOENT errors when Python provider fails before writing response file. The Python worker now ensures an error response file is always written before signaling completion to Node.js, preventing race conditions where Node.js tries to read a non-existent file.
Problem
Users on Windows were encountering
ENOENT: no such file or directoryerrors when using Python providers withsimilarityorllm-rubricassertions:The error occurred even after 16 retry attempts with exponential backoff (up to 18 seconds total wait time).
Root Cause
In
src/python/persistent_wrapper.py, when an exception occurred before the response file could be written (e.g., non-existent function, failed file I/O, invalid request), the outer exception handler would:This caused Node.js to receive the "DONE" signal and attempt to read a response file that never existed, resulting in ENOENT errors that persisted through all retry attempts.
Affected scenarios:
get_callable()fails (function doesn't exist in Python module)The Fix
Modified
handle_call()inpersistent_wrapper.py:response_file = Nonebefore try block (make it accessible in exception handler)response_filebefore sending "DONE"Before:
After:
Testing
Created regression test in
test/python/worker.test.ts:call_nonexistent_api)Test Results:
Files Changed
src/python/persistent_wrapper.py- Fixed error handling to write response before DONEtest/python/fixtures/test_nonexistent_function.py- Test fixturetest/python/worker.test.ts- Added regression testCHANGELOG.md- Documented the fixNotes
Fixes #6072