Extract shared load_llm_config utility#453
Conversation
Replace duplicated inline LLM config loading pattern across all 7 run_infer files with a shared load_llm_config() helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@OpenHands resolve conflicts |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Good refactoring that eliminates real duplication, but missing test coverage for a critical shared utility.
Resolve conflicts in run_infer.py files by keeping Tool import from main while preserving the removal of LLM import (since it's now loaded via the shared load_llm_config helper). Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI resolved the merge conflicts in PR #453 as requested. What was done:
Checklist:
|
Address review feedback requesting test coverage for the shared LLM config loading function used by all 7 benchmarks. Tests verify: - Valid JSON config files load correctly (minimal and full configs) - Missing files raise ValueError with appropriate message - Malformed JSON raises clear validation errors - Edge cases (empty file, permissions issues, unicode) are handled 19 test cases covering the critical code paths. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid refactoring that eliminates real duplication. Simple, focused function with good test coverage. One critical encoding bug must be fixed before merging.
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ Excellent refactoring. Previous issues (encoding, test coverage) are fully resolved. The utility is simple, well-tested with real code paths, and eliminates actual duplication across 7 files. This is what good refactoring looks like - no issues found.
Resolve conflicts in all 7 run_infer files and llm_config.py by taking main's version, since PR OpenHands#453 landed the same changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
LLM) into a sharedload_llm_config()helper inbenchmarks/utils/llm_config.pyrun_infer.pyfiles to use the shared helper, removing ~5 lines of boilerplate from eachTest plan
🤖 Generated with Claude Code