Filter SWE-Bench Multimodal image builds to curated subset#644
Filter SWE-Bench Multimodal image builds to curated subset#644juanmichelini wants to merge 7 commits into
Conversation
Update build_images.py and BUILD_DEFAULTS to honor the same curated default subset (benchmarks/swebenchmultimodal/resolved_instances.txt) as inference, following up to PR #622. Changes: - config.py: Add DEFAULT_RESOLVED_INSTANCES_FILE and use it in BUILD_DEFAULTS - build_images.py: Use BUILD_DEFAULTS['select'] as the --select default - resolved_instances.txt: Add curated instance list (68 instances marked SOLVEABLE) - README.md: Document the curated subset defaults for both inference and build - tests: Update test_defaults and add override behavior tests To build the full dataset instead, pass --select ''. Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands I tested it here https://openhands-eval-monitor.vercel.app/?run=swebenchmultimodal%2Flitellm_proxy-claude-sonnet-4-5-20250929%2F24154684523%2F&text=swebenchm but I see https://github.com/OpenHands/benchmarks/actions/runs/24154797122 Manifest Files: 1 shouldn't it say 68 images? |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
The workflow's SELECT_FILE env var was empty, so --select was never passed to build_images.py even though the script has BUILD_DEFAULTS['select'] set. This caused all images to be built instead of just the 68-instance curated subset. Now the workflow defaults SELECT_FILE to the curated subset file path, consistent with the inference defaults. Co-authored-by: openhands <openhands@all-hands.dev>
|
Good catch! The issue was that the workflow's |
Summary of WorkI've identified and fixed the bug where only 1 image was being built instead of the expected 68 images. Root CauseThe workflow's if [ -n "${SELECT_FILE}" ]; then
CMD="$CMD --select '${SELECT_FILE}'"
fiFix AppliedUpdated
Changes Made
Checklist
|
Resolved conflict in README.md: - Keep curated subset as default (remove explicit --select from example) - Use updated description about default behavior Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands we are still building only 1 see https://github.com/OpenHands/benchmarks/actions/runs/24155990974 |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
The build-select-file action was exiting early when INSTANCE_IDS was empty, potentially interfering with the workflow-level SELECT_FILE env var. This change: 1. Updates the action's echo message to clarify it uses the workflow default 2. Adds a debug step to print SELECT_FILE value before building, which will help diagnose similar issues in the future Co-authored-by: openhands <openhands@all-hands.dev>
Summary of WorkThe issue was that the SWE-Bench Multimodal image build workflow was only building 1 image instead of the expected 68 images. Root CauseThe Changes Made (commit
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable with concerns - This PR solves a real consistency problem (inference and build using different defaults), but introduces a silent breaking change to the CI workflow that needs clearer communication. The code itself is clean and well-tested.
|
@OpenHands please address reviewers concerns, then bring changes from main |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
Final SummaryI've addressed PR #644 comment "please address reviewers concerns, then bring changes from main". Checklist
The branch is now up-to-date with main and pushed to |
|
@OpenHands address reviewers concerns, then bring changes from main, double check everything makes sense and ask for rereview |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
- Workflow: document SELECT_FILE behavior change as a deliberate breaking change to defaults and explain how to opt back into building all images. - build-select-file action: clarify the inverted empty-string semantics between this composite action and build_images.py --select. - README: state the default behavior (68 curated instances) up front. - Tests: add explicit guard that the curated subset file is non-empty, and adopt main's BUILD_DEFAULTS-based assertion in test_defaults. Co-authored-by: openhands <openhands@all-hands.dev>
Update: addressed review feedback + synced with mainHi @juanmichelini, the latest push ( 1. Addressed all 5 reviewer comments
All five review threads have been replied-to and marked resolved. 2. Brought changes from
|
Summary of Work on PR #644The PR comment requested three things: (1) address reviewer concerns, (2) bring changes from main, (3) double-check everything and ask for re-review. Checklist
ConcisenessThe new changes are tightly scoped to the four files called out by reviewers plus the test conflict resolution and merge of main. No extraneous files were touched. The merge brings in main's recent commits as required, but no benchmarks-unrelated edits were introduced in this branch beyond what was needed. The request has been completely addressed. PR: #644 |
juanmichelini
requested review from
all-hands-bot
and removed request for
all-hands-bot
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This PR cleanly completes the curated-subset story by aligning the image-build pipeline default with the inference default established in #622. The CI changes, test additions, and README update are all well-structured. Two items not yet covered by the existing review:
-
Misleading PR description: The "Changes" section lists
config.py,build_images.py, andresolved_instances.txt— but none of these files appear in this PR's actual diff (they were introduced in #622). Conversely, the files that are changed here (.github/actions/build-select-file/action.yml,.github/workflows/build-swebenchmultimodal-images.yml,benchmarks/swebenchmultimodal/README.md) are absent from the description. A reviewer relying on the description to scope their review would look for files that aren't there and miss the key CI workflow changes. Consider updating the description to reflect only the actual diff. -
Debug step left in production CI — see inline comment.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| instance-ids: ${{ env.INSTANCE_IDS }} | ||
|
|
||
| # Debug: Print SELECT_FILE value before building | ||
| - name: Debug SELECT_FILE |
There was a problem hiding this comment.
🟡 Suggestion — Remove debug step before merging: This step was useful during development but adds noise to every production CI run. Consider either:
- Removing it entirely — the build script will surface file-not-found issues clearly enough without it.
- Gating it with
if: runner.debug == '1'so it only runs when someone explicitly enables runner debug mode.
The step itself is harmless, but leftover debug output creates log clutter that can distract from real failures.
3 participants
Summary
Update the SWE-Bench Multimodal image-build path to honor the same curated default subset as inference.
Follow up to #622
Changes
benchmarks/swebenchmultimodal/config.py: AddDEFAULT_RESOLVED_INSTANCES_FILEconstant and use it inBUILD_DEFAULTSwith theselectkeybenchmarks/swebenchmultimodal/build_images.py: Update--selectargument to default toBUILD_DEFAULTS.get("select")instead ofNonebenchmarks/swebenchmultimodal/resolved_instances.txt: Add curated instance list (68 instances markedSOLVEABLEinambiguity_annotations.json)benchmarks/swebenchmultimodal/README.md: Document the curated subset defaults for both inference and build commandstests/test_swebenchmultimodal.py: Add tests verifyingINFER_DEFAULTSandresolved_instances.txtconsistencytests/test_multimodal_phased_build.py: Updatetest_defaultsand add override behavior testsValidation
Behavior
swebenchmultimodal-inferandbuild_images.pynow filter to the curated 68-instance subset--select ''to override the defaultCloses #621.